Re: [PATCH v7] arm64: mm: Populate vmemmap at the page level if not section aligned

[Zhenhua Huang <quic_zhenhuah@quicinc.com>]

On 2025/2/17 22:30, David Hildenbrand wrote:
> On 17.02.25 11:34, Zhenhua Huang wrote:
>>
>>
>> On 2025/2/17 17:44, David Hildenbrand wrote:
>>> On 17.02.25 10:29, Zhenhua Huang wrote:
>>>> On the arm64 platform with 4K base page config, SECTION_SIZE_BITS is 
>>>> set
>>>> to 27, making one section 128M. The related page struct which vmemmap
>>>> points to is 2M then.
>>>> Commit c1cc1552616d ("arm64: MMU initialisation") optimizes the
>>>> vmemmap to populate at the PMD section level which was suitable
>>>> initially since hot plug granule is always one section(128M). However,
>>>> commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug")
>>>> introduced a 2M(SUBSECTION_SIZE) hot plug granule, which disrupted the
>>>> existing arm64 assumptions.
>>>>
>>>> The first problem is that if start or end is not aligned to a section
>>>> boundary, such as when a subsection is hot added, populating the entire
>>>> section is wasteful.
>>>>
>>>> The Next problem is if we hotplug something that spans part of 128 MiB
>>>> section (subsections, let's call it memblock1), and then hotplug
>>>> something
>>>> that spans another part of a 128 MiB section(subsections, let's call it
>>>> memblock2), and subsequently unplug memblock1, vmemmap_free() will 
>>>> clear
>>>> the entire PMD entry which also supports memblock2 even though 
>>>> memblock2
>>>> is still active.
>>>>
>>>> Assuming hotplug/unplug sizes are guaranteed to be symmetric. Do the
>>>> fix similar to x86-64: populate to pages levels if start/end is not
>>>> aligned
>>>> with section boundary.
>>>>
>>>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
>>>> ---
>>>>    arch/arm64/mm/mmu.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>>> index b4df5bc5b1b8..eec1666da368 100644
>>>> --- a/arch/arm64/mm/mmu.c
>>>> +++ b/arch/arm64/mm/mmu.c
>>>> @@ -1178,7 +1178,8 @@ int __meminit vmemmap_populate(unsigned long
>>>> start, unsigned long end, int node,
>>>>    {
>>>>        WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
>>>> -    if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
>>>> +    if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES) ||
>>>> +        (end - start < PAGES_PER_SECTION * sizeof(struct page)))
>>>>            return vmemmap_populate_basepages(start, end, node, altmap);
>>>>        else
>>>>            return vmemmap_populate_hugepages(start, end, node, altmap);
>>>
>>> Yes, this does mimic what x86 does. That handling does look weird,
>>> because it
>>> doesn't care about any address alignments, only about the size, which is
>>> odd.
>>>
>>> I wonder if we could do better and move this handling
>>> into vmemmap_populate_hugepages(), where we already have a fallback
>>> to vmemmap_populate_basepages().
>>
>> Hi David,
>>
>> I had the same doubt initially.
>> After going through the codes, I noticed for vmemmap_populate(), the
>> arguments "start" and "end" passed down should already be within one
>> section.
>> early path:
>> for_each_present_section_nr
>>     __populate_section_memmap
>>         ..
>>         vmemmap_populate()
>>
>> hotplug path:
>> __add_pages
>>     section_activate
>>         vmemmap_populate()
>>
>> Therefore.. focusing only on the size seems OK to me, and fall back
>> solution below appears unnecessary?
> 
> Ah, in that case it is fine. Might make sense to document/enforce that 
> somehow for the time being ...

Shall I document and WARN_ON if the size exceeds? like:
WARN_ON(end - start > PAGES_PER_SECTION * sizeof(struct page))

Since vmemmap_populate() is implemented per architecture, the change 
should apply for other architectures as well. However I have no setup to 
test it on...
Therefore, May I implement it only for arm64 now ?
Additionally, from previous discussion, the change is worth 
backporting(apologies for missing to CC stable kernel in this version). 
Keeping it for arm64 should simplify for backporting. WDYT?

> 
> 
>>> +/*
>>> + * Try to populate PMDs, but fallback to populating base pages when 
>>> ranges
>>> + * would only partially cover a PMD.
>>> + */
>>>    int __meminit vmemmap_populate_hugepages(unsigned long start, 
>>> unsigned
>>> long end,
>>>                                            int node, struct vmem_altmap
>>> *altmap)
>>>    {
>>> @@ -313,6 +317,9 @@ int __meminit vmemmap_populate_hugepages(unsigned
>>> long start, unsigned long end,
>>>           for (addr = start; addr < end; addr = next) {
>>
>> This for loop appears to be redundant for arm64 as well, as above
>> mentioned, a single call to pmd_addr_end() should suffice.
> 
> Right, that was what was confusing me in the first place.
> 
>>
>>>                   next = pmd_addr_end(addr, end);
>>>
>>> +               if (!IS_ALIGNED(addr, PMD_SIZE) || !IS_ALIGNED(next,
>>> PMD_SIZE))
>>> +                       goto fallback;
>>> +
>>>                   pgd = vmemmap_pgd_populate(addr, node);
>>>                   if (!pgd)
>>>                           return -ENOMEM;
>>> @@ -346,6 +353,7 @@ int __meminit vmemmap_populate_hugepages(unsigned
>>> long start, unsigned long end,
>>>                           }
>>>                   } else if (vmemmap_check_pmd(pmd, node, addr, next))
>>>                           continue;
>>> +fallback:
>>>                   if (vmemmap_populate_basepages(addr, next, node, 
>>> altmap))
>>>                           return -ENOMEM;
>>
>> It seems we have no chance to call populate_basepages here?
> 
> 
> Can you elaborate?

It's invoked within vmemmap_populate_hugepages(), which is called by 
vmemmap_populate(). This implies that we are always performing a whole 
section hotplug?
However, since it's common code used by other architectures like x86, 
RISC-V and LoongArch, it is still necessary to review the code for these 
architectures as well. At the very least, it's not a BUG :)

> 
>