mm/page_alloc.c: use NODE_MASK_NONE define used_mask

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 <[email protected]>

---
  mm/page_alloc.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

mm/page_alloc.c +0 -0

index dfcf2682ed40..49dd1f25c000 100644

Quoted 7 lines · Wei Yang

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 <[email protected]>

  mm/page_alloc.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

mm/page_alloc.c static void build_thisnode_zonelists(pg_data_t *pgdat) · line 5587

Quoted 8 lines

mm/page_alloc.c +2 -1

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...

mm/page_alloc.c static void build_zonelists(pg_data_t *pgdat) · line 5597

Quoted 4 lines

mm/page_alloc.c +0 -1

@@ -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.

Quoted 8 lines · John Hubbard

Quoted 7 lines · Wei Yang

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 <[email protected]>

  mm/page_alloc.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

Quoted 13 lines

mm/page_alloc.c static void build_thisnode_zonelists(pg_data_t *pgdat) · line 5587

Quoted 8 lines

mm/page_alloc.c +2 -1

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

Quoted 7 lines

mm/page_alloc.c static void build_zonelists(pg_data_t *pgdat) · line 5597

Quoted 4 lines

mm/page_alloc.c +0 -1

@@ -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

Quoted 8 lines · Wei Yang

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 <[email protected]>

---
  mm/page_alloc.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

mm/page_alloc.c static void build_thisnode_zonelists(pg_data_t *pgdat) · line 5587

Quoted 9 lines

mm/page_alloc.c +1 -1

index dfcf2682ed40..49dd1f25c000 100644
@@ -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...

mm/page_alloc.c static void build_zonelists(pg_data_t *pgdat) · line 5597

Quoted 8 lines

mm/page_alloc.c +0 -1

  	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.

mm/page_alloc.c static void build_zonelists(pg_data_t *pgdat)

Quoted 4 lines

mm/page_alloc.c +0 -0

  	while ((node = find_next_best_node(local_node, &used_mask)) >= 0) {
  		/*
  		 * We don't want to pressure a particular node.
 

thanks,

Signature · 2 lines

John Hubbard
NVIDIA

Quoted 9 lines · John Hubbard

Quoted 8 lines · Wei Yang

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 <[email protected]>

---
  mm/page_alloc.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

Quoted 15 lines

mm/page_alloc.c static void build_thisnode_zonelists(pg_data_t *pgdat) · line 5587

Quoted 9 lines

mm/page_alloc.c +1 -1

index dfcf2682ed40..49dd1f25c000 100644
@@ -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?

Quoted 9 lines · John Hubbard

Quoted 8 lines · Wei Yang

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 <[email protected]>

---
   mm/page_alloc.c | 3 +--
   1 file changed, 1 insertion(+), 2 deletions(-)

Quoted 15 lines

mm/page_alloc.c static void build_thisnode_zonelists(pg_data_t *pgdat) · line 5587

Quoted 9 lines

mm/page_alloc.c +1 -1

index dfcf2682ed40..49dd1f25c000 100644
@@ -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?

Quoted 26 lines · Wei Yang

Quoted 9 lines · John Hubbard

Quoted 8 lines · Wei Yang

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 <[email protected]>

---
   mm/page_alloc.c | 3 +--
   1 file changed, 1 insertion(+), 2 deletions(-)

Quoted 15 lines

mm/page_alloc.c static void build_thisnode_zonelists(pg_data_t *pgdat) · line 5587

Quoted 9 lines

mm/page_alloc.c +1 -1

index dfcf2682ed40..49dd1f25c000 100644
@@ -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...

Quoted 6 lines

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.

Quoted 3 lines

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,

Signature · 2 lines

John Hubbard
NVIDIA

Quoted 35 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

On 3/27/20 6:10 PM, Wei Yang wrote:
...

Quoted 41 lines

Quoted 35 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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 :

mm/page_alloc.c +0 -0

index 4bd35eb83d34..cb4b07458249 100644

mm/page_alloc.c +1 -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,

Signature · 2 lines

John Hubbard
NVIDIA

Quoted 4 lines

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?

On 3/27/20 7:56 PM, Wei Yang wrote:
...

Quoted 11 lines

Quoted 4 lines

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.

Quoted 3 lines

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

Yes, that is correct.

thanks,

Signature · 2 lines

John Hubbard
NVIDIA

On 3/27/20 6:10 PM, Wei Yang wrote:
...

Quoted 41 lines

Quoted 35 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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 :

mm/page_alloc.c +1 -1

index 4bd35eb83d34..cb4b07458249 100644
@@ -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".