| Message ID | 20200327220121.27823-2-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] mm/page_alloc.c: use NODE_MASK_NONE define used_mask | expand |
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... > int node, load, nr_nodes = 0; > nodemask_t used_mask = NODE_MASK_NONE; > int local_node, prev_node; > @@ -5595,7 +5595,6 @@ static void build_zonelists(pg_data_t *pgdat) > load = nr_online_nodes; > prev_node = local_node; > > - memset(node_order, 0, sizeof(node_order)); ...and all subsequent callers are left with whatever debris is remaining in node_order. So this is not good. The reason that memset() was used here, is that there aren't many other ways to get node_order zeroed, given that it is a statically defined variable. > while ((node = find_next_best_node(local_node, &used_mask)) >= 0) { > /* > * We don't want to pressure a particular node. > thanks,
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 > > +++ 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... It is also redundant, all static data is 0 initialized in Linux and should not be explicitly initialized so it can remain in .bss > > @@ -5595,7 +5595,6 @@ static void build_zonelists(pg_data_t *pgdat) > > load = nr_online_nodes; > > prev_node = local_node; > > - memset(node_order, 0, sizeof(node_order)); > > ...and all subsequent callers are left with whatever debris is remaining in > node_order. So this is not good. Indeed Jason
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. My suggestion is to remove the static and define it {0} instead of memset every time. Is my understanding correct here? > >> int node, load, nr_nodes = 0; >> nodemask_t used_mask = NODE_MASK_NONE; >> int local_node, prev_node; >> @@ -5595,7 +5595,6 @@ static void build_zonelists(pg_data_t *pgdat) >> load = nr_online_nodes; >> prev_node = local_node; >> - memset(node_order, 0, sizeof(node_order)); > >...and all subsequent callers are left with whatever debris is remaining in >node_order. So this is not good. > >The reason that memset() was used here, is that there aren't many other ways >to get node_order zeroed, given that it is a statically defined variable. > > >> while ((node = find_next_best_node(local_node, &used_mask)) >= 0) { >> /* >> * We don't want to pressure a particular node. >> > > > >thanks, >-- >John Hubbard >NVIDIA
On Fri, Mar 27, 2020 at 08:18:20PM -0300, Jason Gunthorpe 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 >> > +++ 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... > >It is also redundant, all static data is 0 initialized in Linux and >should not be explicitly initialized so it can remain in .bss > Yeah, you are right. I missed the static word. >> > @@ -5595,7 +5595,6 @@ static void build_zonelists(pg_data_t *pgdat) >> > load = nr_online_nodes; >> > prev_node = local_node; >> > - memset(node_order, 0, sizeof(node_order)); >> >> ...and all subsequent callers are left with whatever debris is remaining in >> node_order. So this is not good. > >Indeed > >Jason
On 03/28/20 at 12:26am, 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. > > My suggestion is to remove the static and define it {0} instead of memset > every time. Is my understanding correct here? Removing static looks good, the node_order is calculated on the basis of each node, it's useless for other node. It may be inherited from the old code where it's a static global variable.
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,
On Fri, Mar 27, 2020 at 05:59:30PM -0700, John Hubbard wrote: >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. > 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. > > >thanks, >-- >John Hubbard >NVIDIA
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". thanks,
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 >
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.
On 3/27/20 7:56 PM, Wei Yang wrote: ... >> 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? > 64 is quite a bit deeper call depth than we expect to see. So 256 bytes on the stack is not completely indefensible, but it's getting close. But worse, that's just an example based on my .config choices. And (as Baoquan just pointed out) it can be much bigger. (And the .config variable is an exponent, not linear, so it gets exciting fast.) In fact, you could overrun the stack directly, with say CONFIG_NODES_SHIFT = 14. > Also because of the limit space on stack, recursive function is not welcome in > kernel neither. Am I right? > Yes, that is correct. thanks,
On Sat, Mar 28, 2020 at 06:30:00PM -0700, John Hubbard wrote: >On 3/27/20 7:56 PM, Wei Yang wrote: >... >> > 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? >> > >64 is quite a bit deeper call depth than we expect to see. So 256 bytes on the stack >is not completely indefensible, but it's getting close. But worse, that's just an >example based on my .config choices. And (as Baoquan just pointed out) it can be much >bigger. (And the .config variable is an exponent, not linear, so it gets exciting fast.) >In fact, you could overrun the stack directly, with say CONFIG_NODES_SHIFT = 14. > Thanks :-) This is better not to use "big" structure on stack. >> Also because of the limit space on stack, recursive function is not welcome in >> kernel neither. Am I right? >> >Yes, that is correct. > >thanks, >-- >John Hubbard >NVIDIA
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}; int node, load, nr_nodes = 0; nodemask_t used_mask = NODE_MASK_NONE; int local_node, prev_node; @@ -5595,7 +5595,6 @@ static void build_zonelists(pg_data_t *pgdat) load = nr_online_nodes; prev_node = local_node; - memset(node_order, 0, sizeof(node_order)); while ((node = find_next_best_node(local_node, &used_mask)) >= 0) { /* * We don't want to pressure a particular node.
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(-)