Re: [PATCH v5 1/2] sparc: Use vmemmap_populate_hugepages for vmemmap_populate

[Andreas Larsson <andreas@gaisler.com>]

On 2026-01-28 04:25, Chengkaitao wrote:
> On Mon, Jan 26, 2026 at 10:50 PM Andreas Larsson <andreas@gaisler.com> wrote:
>>
>> On 2026-01-11 08:44, chengkaitao wrote:
>>> From: Chengkaitao <chengkaitao@kylinos.cn>
>>>
>>> 1. In the SPARC architecture, reimplemented vmemmap_populate using
>>> vmemmap_populate_hugepages.
>>> 2. Allow the SPARC arch to fallback to vmemmap_populate_basepages(),
>>> when vmemmap_alloc_block returns NULL.
>>
>> This patch seems to potentially make more functional changes than what
>> the descriptions gives impression of.
>>
>> Given the amount of changes this seems to introduce, more on that below,
>> I'd like to see more description on the changes and why they can be done
>> than this.
>>
>> Nit: use active language, "reimplement", not "reimplemented".
>>
>>
>>> Signed-off-by: Chengkaitao <chengkaitao@kylinos.cn>
>>> Acked-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
>>> ---
>>>  arch/sparc/mm/init_64.c | 47 ++++++++++++++---------------------------
>>>  1 file changed, 16 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
>>> index df9f7c444c39..858eaa6615ea 100644
>>> --- a/arch/sparc/mm/init_64.c
>>> +++ b/arch/sparc/mm/init_64.c
>>> @@ -2581,8 +2581,8 @@ unsigned long _PAGE_CACHE __read_mostly;
>>>  EXPORT_SYMBOL(_PAGE_CACHE);
>>>
>>>  #ifdef CONFIG_SPARSEMEM_VMEMMAP
>>> -int __meminit vmemmap_populate(unsigned long vstart, unsigned long vend,
>>> -                            int node, struct vmem_altmap *altmap)
>>> +void __meminit vmemmap_set_pmd(pmd_t *pmd, void *p, int node,
>>> +                            unsigned long addr, unsigned long next)
>>>  {
>>>       unsigned long pte_base;
>>>
>>> @@ -2595,39 +2595,24 @@ int __meminit vmemmap_populate(unsigned long vstart, unsigned long vend,
>>>
>>>       pte_base |= _PAGE_PMD_HUGE;
>>>
>>> -     vstart = vstart & PMD_MASK;
>>> -     vend = ALIGN(vend, PMD_SIZE);
>>
>> It seems that this patch removes alignment of both start and end. Is
>> this a functional change in practice or are these always aligned for
>> some other reason?
>>
> Whether vstart and vend are aligned with PMD_SIZE doesn't seem to
> affect the behavior pattern or output of vmemmap_populate_hugepages.
> The vmemmap_populate_hugepages function performs necessary alignment
> processing internally, such as pmd_addr_end and pmd/pte_index?
> 
>>> -     for (; vstart < vend; vstart += PMD_SIZE) {
>>> -             pgd_t *pgd = vmemmap_pgd_populate(vstart, node);
>>> -             unsigned long pte;
>>> -             p4d_t *p4d;
>>> -             pud_t *pud;
>>> -             pmd_t *pmd;
>>> -
>>> -             if (!pgd)
>>> -                     return -ENOMEM;
>>> -
>>> -             p4d = vmemmap_p4d_populate(pgd, vstart, node);
>>> -             if (!p4d)
>>> -                     return -ENOMEM;
>>> -
>>> -             pud = vmemmap_pud_populate(p4d, vstart, node);
>>> -             if (!pud)
>>> -                     return -ENOMEM;
>>> +     pmd_val(*pmd) = pte_base | __pa(p);
>>> +}
>>>
>>> -             pmd = pmd_offset(pud, vstart);
>>> -             pte = pmd_val(*pmd);
>>> -             if (!(pte & _PAGE_VALID)) {
>>
>> It is not the same thing, but is this equivalent to if
>> (pmd_none(pmdp_get(pmd))) at this point?
>>
> For PMD entries, there shouldn't be cases where pmd_none and
> _PAGE_VALID exhibit inconsistent behavior. I've observed that
> pmd_none is widely used in the SPARC architecture.
> 
>>> -                     void *block = vmemmap_alloc_block(PMD_SIZE, node);
>>> +int __meminit vmemmap_check_pmd(pmd_t *pmdp, int node,
>>> +                             unsigned long addr, unsigned long next)
>>> +{
>>> +     int large = pmd_leaf(*pmdp);
>>>
>>> -                     if (!block)
>>> -                             return -ENOMEM;
>>> +     if (large)
>>> +             vmemmap_verify((pte_t *)pmdp, node, addr, next);
>>>
>>> -                     pmd_val(*pmd) = pte_base | __pa(block);
>>> -             }
>>> -     }
>>> +     return large;
>>> +}
>>>
>>> -     return 0;
>>> +int __meminit vmemmap_populate(unsigned long vstart, unsigned long vend,
>>> +                            int node, struct vmem_altmap *altmap)
>>> +{
>>> +     return vmemmap_populate_hugepages(vstart, vend, node, altmap);
>>>  }
>>>  #endif /* CONFIG_SPARSEMEM_VMEMMAP */
>>>
>>
>>
>> This change introduces using vmemmap_alloc_block_buf() instead of
>> vmemmap_alloc_block() seems to introduce two new behaviours that was not
>> in use for sparc64 before:
>>
>> 1) Using altmap_alloc_block_buf() for a non-null altmap, that was not
>>    used before. Also the fallback to vmemmap_populate_basepages() passes
>>    on altmap.
> 
> If altmap validation isn't required, I can retain the original code
> logic by setting altmap to NULL.
> 
>> 2) Trying sparse_buffer_alloc() before vmemmap_alloc_block(), which was
>>    not done before.
> 
> In SPARC, sparse_init() is called to initialize the sparsemap_buf.
> If the SPARC architecture doesn't support using sparse_buffer_alloc,
> we can remove the sparse_init() call path.

Thank you for the details.

>> Neither the commit message nor the cover letter touches upon this. Could
>> you elaborate here?
>>
>> Given all the (at least seeming) functional changes could you share how
>> you tested this change?
> 
> My original intention was to help architectures adopt more generic
> kernel APIs to reduce maintenance costs. However, due to my lack of
> physical SPARC devices, I couldn't perform comprehensive testing,
> I've only verified compilation correctness based on code analysis.
> I sincerely apologize for this limitation. If you have access to
> physical SPARC hardware, could you kindly help with testing?

Yes, I have tested v6 on SPARC hardware.

Cheers,
Andreas