Re: [Patch v2 2/2] mm/page_alloc.c: define node_order with all zero

[John Hubbard]

On 3/27/20 6:10 PM, Wei Yang wrote:
...
> > It's not just about preserving the value. Sometimes it's about stack space.
> > Here's the trade-offs for static variables within a function:
> >
> > Advantages of static variables within a function (compared to non-static
> > variables, also within a function):
> > -----------------------------------
> >
> > * Doesn't use any of the scarce kernel stack space
> > * Preserves values (not always necessarily and advantage)
> >
> > Disadvantages:
> > -----------------------------------
> >
> > * Removes basic thread safety: multiple threads can no longer independently
> >   call the function without getting interaction, and generally that means
> >   data corruption.
> >
> > So here, I suspect that the original motivation was probably to conserve stack
> > space, and the author likely observed that there was no concurrency to worry
> > about: the function was only being called by one thread at a time.  Given those
> > constraints (which I haven't confirmed just yet, btw), a static function variable
> > fits well.
> >
> > > My suggestion is to remove the static and define it {0} instead of memset
> > > every time. Is my understanding correct here?
> >
> >
> > Not completely:
> >
> > a) First of all, "instead of memset every time" is a misconception, because
> >    there is still a memset happening every time with {0}. It's just that the
> >    compiler silently writes that code for you, and you don't see it on the
> >    screen. But it's still there.
> >
> > b) Switching away from a static to an on-stack variable requires that you first
> >    verify that stack space is not an issue. Or, if you determine that this
> >    function needs the per-thread isolation that a non-static variable provides,
> >    then you can switch to either an on-stack variable, or a *alloc() function.
>
> I think you get some point. While one more question about stack and static. If
> one function is thread safe, which factor determines whether we choose on
> stack value or static? Any reference size? It looks currently we don't have a
> guide line for this.


There's not really any general guideline, but applying the points above (plus keeping
in mind that kernel stack space is quite small) to each case, you'll come to a good
answer.

In this case, if we really are only ever calling this function in one thread at a time,
then it's probably best to let the "conserve stack space" point win. Which leads to
just leaving the code nearly as-is. The only thing left to do would be to (optionally,
because this is an exceedingly minor point) delete the arguably misleading "= {0}" part.
And as Jason points out, doing so also moves node_order into .bss :

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 4bd35eb83d34..cb4b07458249 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5607,7 +5607,7 @@ static void build_thisnode_zonelists(pg_data_t *pgdat)
 
 static void build_zonelists(pg_data_t *pgdat)
 {
-       static int node_order[MAX_NUMNODES] = {0};
+       static int node_order[MAX_NUMNODES];
        int node, load, nr_nodes = 0;
        nodemask_t used_mask = NODE_MASK_NONE;
        int local_node, prev_node;



Further note: On my current testing .config, I've got MAX_NUMNODES set to 64, which makes
256 bytes required for node_order array. 256 bytes on a 16KB stack is a little bit above
my mental watermark for "that's too much in today's kernels".


thanks,
--
John Hubbard
NVIDIA