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

[Chengkaitao <pilgrimtao@gmail.com>]

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.

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

-- 
Cheers,
Chengkaitao