Re: [PATCH v2 1/2] arm64: mm: vmemmap populate to page level if not section aligned

[Zhenhua Huang <quic_zhenhuah@quicinc.com>]

On 2024/12/30 15:48, Zhenhua Huang wrote:
> Hi Anshuman,
> 
> On 2024/12/27 15:49, Anshuman Khandual wrote:
>> On 12/24/24 19:39, Catalin Marinas wrote:
>>> On Tue, Dec 24, 2024 at 05:32:06PM +0800, Zhenhua Huang wrote:
>>>> Thanks Catalin for review!
>>>> Merry Christmas.
>>>
>>> Merry Christmas to you too!
>>>
>>>> On 2024/12/21 2:30, Catalin Marinas wrote:
>>>>> On Mon, Dec 09, 2024 at 05:42:26PM +0800, Zhenhua Huang wrote:
>>>>>> Fixes: c1cc1552616d ("arm64: MMU initialisation")
>>>>>
>>>>> I wouldn't add a fix for the first commit adding arm64 support, we did
>>>>> not even have memory hotplug at the time (added later in 5.7 by commit
>>>>> bbd6ec605c0f ("arm64/mm: Enable memory hot remove")). IIUC, this 
>>>>> hasn't
>>>>> been a problem until commit ba72b4c8cf60 ("mm/sparsemem: support
>>>>> sub-section hotplug"). That commit broke some arm64 assumptions.
>>>>
>>>> Shall we add ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug")
>>>> because it broke arm64 assumptions ?
>>>
>>> Yes, I think that would be better. And a cc stable to 5.4 (the above
>>> commit appeared in 5.3).
>>
>> Agreed. This is a problem which needs fixing but not sure if proposed 
>> patch
>> here fixes that problem.
>>
>>>
>>>>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>>>>> index e2739b69e11b..fd59ee44960e 100644
>>>>>> --- a/arch/arm64/mm/mmu.c
>>>>>> +++ b/arch/arm64/mm/mmu.c
>>>>>> @@ -1177,7 +1177,9 @@ 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) ||
>>>>>> +    !IS_ALIGNED(page_to_pfn((struct page *)start), 
>>>>>> PAGES_PER_SECTION) ||
>>>>>> +    !IS_ALIGNED(page_to_pfn((struct page *)end), PAGES_PER_SECTION))
>>>>>>            return vmemmap_populate_basepages(start, end, node, 
>>>>>> altmap);
>>>>>>        else
>>>>>>            return vmemmap_populate_hugepages(start, end, node, 
>>>>>> altmap);
>>>>>
>>>>> An alternative would be to fix unmap_hotplug_pmd_range() etc. to avoid
>>>>> nuking the whole vmemmap pmd section if it's not empty. Not sure how
>>>>> easy that is, whether we have the necessary information (I haven't
>>>>> looked in detail).
>>>>>
>>>>> A potential issue - can we hotplug 128MB of RAM and only unplug 
>>>>> 2MB? If
>>>>> that's possible, the problem isn't solved by this patch.
>>
>> Believe this is possible after sub-section hotplug and hotremove support.
>>
>>>>
>>>> Indeed, seems there is no guarantee that plug size must be equal to 
>>>> unplug
>>>> size...
>>>>
>>>> I have two ideas:
>>>> 1. Completely disable this PMD mapping optimization since there is no
>>>> guarantee we must align 128M memory for hotplug ..
>>>
>>> I'd be in favour of this, at least if CONFIG_MEMORY_HOTPLUG is enabled.
>>> I think the only advantage here is that we don't allocate a full 2MB
>>> block for vmemmap when only plugging in a sub-section.
>>
>> Agreed, that will be the right fix for the problem which can be back 
>> ported.
>> We will have to prevent PMD/PUD/CONT mappings for both linear and as 
>> well as
> 
> Thanks Anshuman, yeah.. we must handle linear mapping as well.
> 
>> vmemmap for all non-boot memory sections, that can be hot-unplugged.
>>
>> Something like the following ? [untested]
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 216519663961..56b9c6891f46 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1171,9 +1171,15 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, 
>> int node,
>>   int __meminit vmemmap_populate(unsigned long start, unsigned long 
>> end, int node,
>>                  struct vmem_altmap *altmap)
>>   {
>> +       unsigned long start_pfn;
>> +       struct mem_section *ms;
>> +
>>          WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
>> -       if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
>> +       start_pfn = page_to_pfn((struct page *)start);
>> +       ms = __pfn_to_section(start_pfn);
>> +
>> +       if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES) || !early_section(ms))
> 
> LGTM. I will follow your and Catalin's suggestion to prepare further 
> patches, Thanks!
> 
>>                  return vmemmap_populate_basepages(start, end, node, 
>> altmap);
>>          else
>>                  return vmemmap_populate_hugepages(start, end, node, 
>> altmap);
>> @@ -1334,10 +1340,15 @@ struct range arch_get_mappable_range(void)
>>   int arch_add_memory(int nid, u64 start, u64 size,
>>                      struct mhp_params *params)
>>   {
>> +       unsigned long start_pfn = page_to_pfn((struct page *)start);
>> +       struct mem_section *ms = __pfn_to_section(start_pfn);
>>          int ret, flags = NO_EXEC_MAPPINGS;
>>          VM_BUG_ON(!mhp_range_allowed(start, size, true));
>> +       if (!early_section(ms))
>> +               flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> 
> However, here comes another doubt, given that the subsection size is 2M, 
> shouldn't we have ability to support PMD SECTION MAPPING if 
> CONFIG_ARM64_4K_PAGES? This might be the optimization we want to maintain?
> 
> Should we remove NO_BLOCK_MAPPINGS and add more constraints to avoid 
> pud_set_huge if CONFIG_ARM64_4K_PAGES ?
> 

BTW, shall we remove the check for !early_section since arch_add_memory 
is only called during hotplugging case? Correct me please if I'm mistaken :)
The idea is like(not fully tested):

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index e2739b69e11b..9afeb35673a3 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -45,6 +45,7 @@
  #define NO_BLOCK_MAPPINGS      BIT(0)
  #define NO_CONT_MAPPINGS       BIT(1)
  #define NO_EXEC_MAPPINGS       BIT(2)  /* assumes FEAT_HPDS is not used */
+#define NO_PUD_BLOCK_MAPPINGS  BIT(3)  /* Hotplug case: do not want 
block mapping for PUD */

  u64 kimage_voffset __ro_after_init;
  EXPORT_SYMBOL(kimage_voffset);
@@ -356,10 +357,12 @@ static void alloc_init_pud(p4d_t *p4dp, unsigned 
long addr, unsigned long end,

                 /*
                  * For 4K granule only, attempt to put down a 1GB block
+                * Hotplug case: do not attempt 1GB block
                  */
                 if (pud_sect_supported() &&
                    ((addr | next | phys) & ~PUD_MASK) == 0 &&
-                   (flags & NO_BLOCK_MAPPINGS) == 0) {
+                   (flags & NO_BLOCK_MAPPINGS) == 0 &&
+                   (flags & NO_PUD_BLOCK_MAPPINGS) == 0) {
                         pud_set_huge(pudp, phys, prot);

                         /*
@@ -1175,9 +1178,16 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, int 
node,
  int __meminit vmemmap_populate(unsigned long start, unsigned long end, 
int node,
                 struct vmem_altmap *altmap)
  {
+       unsigned long start_pfn;
+       struct mem_section *ms;
+
         WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));

-       if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
+       start_pfn = page_to_pfn((struct page *)start);
+       ms = __pfn_to_section(start_pfn);
+
+       /* hotplugged section not support hugepages */
+       if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES) || !early_section(ms))
                 return vmemmap_populate_basepages(start, end, node, 
altmap);
         else
                 return vmemmap_populate_hugepages(start, end, node, 
altmap);
@@ -1342,6 +1352,16 @@ int arch_add_memory(int nid, u64 start, u64 size,

         VM_BUG_ON(!mhp_range_allowed(start, size, true));

+       if (IS_ENABLED(CONFIG_ARM64_4K_PAGES))
+       /*
+        * As per subsection granule is 2M, allow PMD block mapping in
+        * case 4K PAGES.
+        * Other cases forbid section mapping.
+        */
+               flags |= NO_PUD_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
+       else
+               flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
+
         if (can_set_direct_map())
                 flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;



>> +
>>          if (can_set_direct_map())
>>                  flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>>
>>>
>>>> 2. If we want to take this optimization.
>>>> I propose adding one argument to vmemmap_free to indicate if the entire
>>>> section is freed(based on subsection map). Vmemmap_free is a common 
>>>> function
>>>> and might affect other architectures... The process would be:
>>>> vmemmap_free
>>>>     unmap_hotplug_range //In unmap_hotplug_pmd_range() as you 
>>>> mentioned：if
>>>> whole section is freed, proceed as usual. Otherwise, *just clear out 
>>>> struct
>>>> page content but do not free*.
>>>>     free_empty_tables // will be called only if entire section is freed
>>>>
>>>> On the populate side,
>>>> else if (vmemmap_check_pmd(pmd, node, addr, next)) //implement this 
>>>> function
>>>>     continue;    //Buffer still exists, just abort..
>>>>
>>>> Could you please comment further whether #2 is feasible ?
>>>
>>> vmemmap_free() already gets start/end, so it could at least check the
>>> alignment and avoid freeing if it's not unplugging a full section. It
>>
>> unmap_hotplug_pmd_range()
>> {
>>     do {
>>         if (pmd_sect(pmd)) {
>>             pmd_clear(pmdp);
>>             flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>>                          if (free_mapped)
>>                                  free_hotplug_page_range(pmd_page(pmd),
>>                                                          PMD_SIZE, 
>> altmap);
>>         }
>>     } while ()
>> }
>>
>> Do you mean clearing the PMD entry but not freeing the mapped page for 
>> vmemmap ?
>> In that case should the hot-unplug fail or not ? If we free the pfns 
>> (successful
>> hot-unplug), then leaving behind entire PMD entry for covering the 
>> remaining sub
>> sections, is going to be problematic as it still maps the removed pfns 
>> as well !
> 
> Could you please help me to understand in which scenarios this might 
> cause issue? I assume we won't touch these struct page further?
> 
>>
>>> does leave a 2MB vmemmap block in place when freeing the last subsection
>>> but it's safer than freeing valid struct page entries. In addition, it
>>> could query the memory hotplug state with something like
>>> find_memory_block() and figure out whether the section is empty.
>>
>> I guess there are two potential solutions, if 
>> unmap_hotplug_pmd_range() were to
>> handle sub-section removal.
>>
>> 1) Skip pmd_clear() when entire section is not covered
>>
>> a. pmd_clear() only if all but the current subsection have been 
>> removed earlier
>>     via is_subsection_map_empty() or something similar.
>>
>> b. Skip pmd_clear() if the entire section covering that PMD is not 
>> being removed
>>     but that might be problematic, as it still maps potentially 
>> unavailable pfns,
>>     which are now hot-unplugged out.
>>
>> 2) Break PMD into base pages
>>
>> a. pmd_clear() only if all but the current subsection have been 
>> removed earlier
>>     via is_subsection_map_empty() or something similar.
>>
>> b. Break entire PMD into base page mappings and remove entries 
>> corresponding to
>>     the subsection being removed. Although the BBM sequence needs to 
>> be followed
>>     while making sure that no other part of the kernel is accessing 
>> subsections,
>>     that are mapped via the erstwhile PMD but currently not being 
>> removed.
>>
>>>
>>> Anyway, I'll be off until the new year, maybe I get other ideas by then.
>>>
> 
>