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
