Re: [PATCH 2/2] mm/numa_emulation: Move the size calculation in split_nodes_interleave() to a separate function

[Pratyush Brahma <pratyush.brahma@oss.qualcomm.com>]

On Mon, Sep 29, 2025 at 12:46 PM Pratyush Brahma
<pratyush.brahma@oss.qualcomm.com> wrote:
>
>
>
> On Fri, Sep 26, 2025 at 7:50 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> >
> >
> >
> > Le 26/09/2025 à 12:34, pratyush.brahma@oss.qualcomm.com a écrit :
> > > From: Pratyush Brahma <pratyush.brahma@oss.qualcomm.com>
> > >
> > > The size calculation in split_nodes_interleave() has several nuances.
> > > Move it to a separate function to improve code modularity and
> > > simplify the readability of split_nodes_interleave().
> > >
> > > Signed-off-by: Pratyush Brahma <pratyush.brahma@oss.qualcomm.com>
> > > ---
> > >   mm/numa_emulation.c | 44 +++++++++++++++++++++++++++++---------------
> > >   1 file changed, 29 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/mm/numa_emulation.c b/mm/numa_emulation.c
> > > index 2a335b3dd46a..882c349c2a0f 100644
> > > --- a/mm/numa_emulation.c
> > > +++ b/mm/numa_emulation.c
> > > @@ -76,6 +76,34 @@ static int __init emu_setup_memblk(struct numa_meminfo *ei,
> > >       return 0;
> > >   }
> > >
> > > +static void __init __calc_split_params(u64 addr, u64 max_addr,
> > > +             int nr_nodes, u64 *psize, int *pbig)
> > > +{
> > > +     u64 size, usable_size;
> > > +     int big;
> > > +
> > > +     /* total usable memory (skip holes) */
> > > +     usable_size  = max_addr - addr - mem_hole_size(addr, max_addr);
> > > +
> > > +     /*
> > > +      * Calculate target node size.  x86_32 freaks on __udivdi3() so do
> > > +      * the division in ulong number of pages and convert back.
> > > +      */
> > > +     size = PFN_PHYS((unsigned long)(usable_size >> PAGE_SHIFT) / nr_nodes);
> > > +
> > > +     /*
> > > +      * Calculate the number of big nodes that can be allocated as a result
> > > +      * of consolidating the remainder.
> > > +      */
> > > +     big = ((size & (FAKE_NODE_MIN_SIZE - 1UL)) * nr_nodes) / FAKE_NODE_MIN_SIZE;
> > > +
> > > +     /* Align the base size down to the minimum granularity */
> > > +     size = ALIGN_DOWN(size, FAKE_NODE_MIN_SIZE);
> > > +
> > > +     *psize = size;
> > > +     *pbig  = big;
> >
> > Having to return simple type values through pointers is usually the
> > start of proplems.Whenever possible you shouldn't returning simple types
> > via pointers.
> Thanks Christophe for your comments. Can you please help me understand what kind of
> problems can we run into so I can be mindful of this going forward?
> >
> > Your function is void, it could return size instead.
> Sure, it can be done.
> >
> > And big seems independant, could be returned by another function.
> Had included big in this function as it was calculated before we align the size to
> FAKE_NODE_MIN_SIZE. If we move the calculation of big to a separate function,
> it would compute the value after the alignment of size, which would always render
> big as zero, wouldn't it?
>
> And if I move the calculation of big to a separate function which takes in the precomputed
> size value as input and call it within the new helper, then I would still have to return big
> from this new helper, won't I?
>
> Please let me know if I am missing something.
Hi Christophe

Can you please help me to understand here?
> >
> > > +}
> > > +
> > >   /*
> > >    * Sets up nr_nodes fake nodes interleaved over physical nodes ranging from addr
> > >    * to max_addr.
> > > @@ -100,21 +128,7 @@ static int __init split_nodes_interleave(struct numa_meminfo *ei,
> > >               nr_nodes = MAX_NUMNODES;
> > >       }
> > >
> > > -     /*
> > > -      * Calculate target node size.  x86_32 freaks on __udivdi3() so do
> > > -      * the division in ulong number of pages and convert back.
> > > -      */
> > > -     size = max_addr - addr - mem_hole_size(addr, max_addr);
> > > -     size = PFN_PHYS((unsigned long)(size >> PAGE_SHIFT) / nr_nodes);
> > > -
> > > -     /*
> > > -      * Calculate the number of big nodes that can be allocated as a result
> > > -      * of consolidating the remainder.
> > > -      */
> > > -     big = ((size & (FAKE_NODE_MIN_SIZE - 1UL)) * nr_nodes) /
> > > -             FAKE_NODE_MIN_SIZE;
> > > -
> > > -     size = ALIGN_DOWN(size, FAKE_NODE_MIN_SIZE);
> > > +     __calc_split_params(addr, max_addr, nr_nodes, &size, &big);
> > >       if (!size) {
> > >               pr_err("Not enough memory for each node.  "
> > >                       "NUMA emulation disabled.\n");
> > >
> >
Thanks and Regards
Pratyush