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

[-- Attachment #1: Type: text/plain, Size: 4178 bytes --]

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.
>
> > +}
> > +
> >   /*
> >    * 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");
> >
>

[-- Attachment #2: Type: text/html, Size: 5334 bytes --]