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

On Mon, Jan 26, 2026 at 10:50 PM Andreas Larsson <[email protected]> wrote:
>
> On 2026-01-11 08:44, chengkaitao wrote:
> > From: Chengkaitao <[email protected]>
> >
> > 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.

Hi Andreas Larsson,
Regarding my previous responses to the questions raised in this
email thread, I have not yet received your reply. I would like
to provide additional clarifications on these points here:

> Nit: use active language, "reimplement", not "reimplemented".

This issue has been fixed in version v6.
https://lore.kernel.org/all/[email protected]/

> > Signed-off-by: Chengkaitao <[email protected]>
> > Acked-by: Mike Rapoport (Microsoft) <[email protected]>
> > ---
> >  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?

In the implementation of vmemmap_populate_hugepages, the start
value remains unaligned when entering the for-loop for the first
time. However, there is no need to apply the "start & PMD_MASK"
operation in the vmemmap_*_populate series of functions. In
iterations after the first one, pmd_addr_end() will align the
start value; since start is already aligned, there is no longer
a need to align end.

In fact, in the original code, the presence of
"vstart = vstart & PMD_MASK" made "vend = ALIGN(vend, PMD_SIZE)"
redundant as well.

> > -     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?

Since the vmemmap_*_populate functions all use vmemmap_alloc_block_zero
when allocating page tables, (pmd_none(pmdp_get(pmd))) can be used as a
replacement at this point.

> > -                     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.

This issue has been fixed in version v6.
https://lore.kernel.org/all/[email protected]/

> 2) Trying sparse_buffer_alloc() before vmemmap_alloc_block(), which was
>    not done before.

The sparse_buffer_alloc function here is usable.
Both sparse_buffer_alloc() and vmemmap_alloc_block() essentially
call memmap_alloc(), the only difference is that sparse_buffer_alloc
performs the allocation in advance.

> 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?
>
> Cheers,
> Andreas
>

Is there still a possibility for this patch to be merged into the
mainline? Should I continue modifying and iterating on it? Could
you help with testing it?

Regardless of your answer, I look forward to your reply.

-- 
Yours,
Chengkaitao