linux-kernel - Re: [Patch v2 2/2] mm/page_alloc.c: define node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 27 Mar 2020 17:59:30 -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 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@...il.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.



thanks,
-- 
John Hubbard
NVIDIA