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

[Wei Yang <richard.weiyang@gmail.com>]

On Fri, Mar 27, 2020 at 05:59:30PM -0700, John Hubbard wrote:
>On 3/27/20 5:26 PM, Wei Yang wrote:
>> On Fri, Mar 27, 2020 at 03:37:57PM -0700, John Hubbard wrote:
>> > On 3/27/20 3:01 PM, Wei Yang wrote:
>> > > Since we always clear node_order before getting it, we can leverage
>> > > compiler to do this instead of at run time.
>> > > 
>> > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> > > ---
>> > >    mm/page_alloc.c | 3 +--
>> > >    1 file changed, 1 insertion(+), 2 deletions(-)
>> > > 
>> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> > > index dfcf2682ed40..49dd1f25c000 100644
>> > > --- a/mm/page_alloc.c
>> > > +++ b/mm/page_alloc.c
>> > > @@ -5585,7 +5585,7 @@ static void build_thisnode_zonelists(pg_data_t *pgdat)
>> > >    static void build_zonelists(pg_data_t *pgdat)
>> > >    {
>> > > -	static int node_order[MAX_NUMNODES];
>> > > +	static int node_order[MAX_NUMNODES] = {0};
>> > 
>> > 
>> > Looks wrong: now the single instance of node_order is initialized just once by
>> > the compiler. And that means that only the first caller of this function
>> > gets a zeroed node_order array...
>> > 
>> 
>> What a shame on me. You are right, I miss the static word.
>> 
>> Well, then I am curious about why we want to define it as static. Each time we
>> call this function, node_order would be set to 0 and find_next_best_node()
>> would sort a proper value into it. I don't see the reason to reserve it in a
>> global area and be used next time.
>
>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.

>
>
>thanks,
>-- 
>John Hubbard
>NVIDIA

-- 
Wei Yang
Help you, Help me