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]

Message-ID: <20200328025605.cpnbnavl27pphr7g@master>
Date:   Sat, 28 Mar 2020 02:56:05 +0000
From:   Wei Yang <richard.weiyang@...il.com>
To:     John Hubbard <jhubbard@...dia.com>
Cc:     Wei Yang <richard.weiyang@...il.com>, 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 Fri, Mar 27, 2020 at 06:28:36PM -0700, John Hubbard wrote:
>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};

This is what I added, so I would drop this one.

>+       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 for your explanation. I would keep this in mind.

Now I have one more question, hope it won't sound silly. (16KB / 256) = 64,
this means if each function call takes 256 space on stack, the max call depth
is 64. So how deep a kernel function call would be? or expected to be?

Also because of the limit space on stack, recursive function is not welcome in
kernel neither. Am I right?

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

-- 
Wei Yang
Help you, Help me