Messages in this thread | | | Subject | Re: [Patch v2 2/2] mm/page_alloc.c: define node_order with all zero | From | John Hubbard <> | Date | Fri, 27 Mar 2020 17:59:30 -0700 |
| |
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.
thanks, -- John Hubbard NVIDIA
| |