Date:   Fri, 27 Mar 2020 18:28:36 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Wei Yang <richard.weiyang@...il.com>
CC:     <akpm@...ux-foundation.org>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>, <jgg@...pe.ca>, <david@...hat.com>
Subject: Re: [Patch v2 2/2] mm/page_alloc.c: define node_order with all zero

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

Powered by blists - more mailing lists