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: <20200328112517.GR3039@MiWiFi-R3L-srv>
Date:   Sat, 28 Mar 2020 19:25:17 +0800
From:   Baoquan He <bhe@...hat.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 03/27/20 at 06:28pm, 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};
> +       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".

Oh, so Michal was deliberate to do so. I have CONFIG_NODES_SHIFT as 10
in my laptop config. That truly will cost much kernel stack. Thanks for
telling this.