[PATCH V2 0/3] Refine numa_emulation

[PATCH V2 0/3] Refine numa_emulation

[Wei Yang]

My previous patch "x86/mm/numa: Remove numa_nodemask_from_meminfo()" hits a
problem in numa_emulation. The reason is numa_nodes_parsed is not set
correctly after emulation.

This patch set tries to fix this and also with two code refine.

Detailed discussions are in this thread:

    https://lkml.org/lkml/2017/3/13/1230

and test result is posted :

    https://lkml.org/lkml/2017/4/10/641

V2:
    * refresh the change log based on David comments
    * use nodes_clear()

Wei Yang (3):
  x86/numa_emulation: fix potential memory leak
  x86/numa_emulation: assign physnode_mask directly from
    numa_nodes_parsed
  x86/numa_emulation: restructures numa_nodes_parsed from emulated nodes

 arch/x86/mm/numa_emulation.c | 61 ++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

-- 
2.11.0

[PATCH V2 1/3] x86/numa_emulation: fix potential memory leak

[Wei Yang]

numa_emulation() needs to allocate a space for phys_dist[] temporarily,
while current code may miss to release this when dfl_phys_nid ==
NUMA_NO_NODE. It is observed in code review instead of in a real case.

This patch fixes this by re-order the code path.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Acked-by: David Rientjes <rientjes@google.com>
---
 arch/x86/mm/numa_emulation.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/x86/mm/numa_emulation.c b/arch/x86/mm/numa_emulation.c
index a8f90ce3dedf..eb017c816de6 100644
--- a/arch/x86/mm/numa_emulation.c
+++ b/arch/x86/mm/numa_emulation.c
@@ -353,6 +353,24 @@ void __init numa_emulation(struct numa_meminfo *numa_meminfo, int numa_dist_cnt)
 		goto no_emu;
 	}
 
+	/*
+	 * Determine the max emulated nid and the default phys nid to use
+	 * for unmapped nodes.
+	 */
+	max_emu_nid = 0;
+	dfl_phys_nid = NUMA_NO_NODE;
+	for (i = 0; i < ARRAY_SIZE(emu_nid_to_phys); i++) {
+		if (emu_nid_to_phys[i] != NUMA_NO_NODE) {
+			max_emu_nid = i;
+			if (dfl_phys_nid == NUMA_NO_NODE)
+				dfl_phys_nid = emu_nid_to_phys[i];
+		}
+	}
+	if (dfl_phys_nid == NUMA_NO_NODE) {
+		pr_warn("NUMA: Warning: can't determine default physical node, disabling emulation\n");
+		goto no_emu;
+	}
+
 	/* copy the physical distance table */
 	if (numa_dist_cnt) {
 		u64 phys;
@@ -372,24 +390,6 @@ void __init numa_emulation(struct numa_meminfo *numa_meminfo, int numa_dist_cnt)
 					node_distance(i, j);
 	}
 
-	/*
-	 * Determine the max emulated nid and the default phys nid to use
-	 * for unmapped nodes.
-	 */
-	max_emu_nid = 0;
-	dfl_phys_nid = NUMA_NO_NODE;
-	for (i = 0; i < ARRAY_SIZE(emu_nid_to_phys); i++) {
-		if (emu_nid_to_phys[i] != NUMA_NO_NODE) {
-			max_emu_nid = i;
-			if (dfl_phys_nid == NUMA_NO_NODE)
-				dfl_phys_nid = emu_nid_to_phys[i];
-		}
-	}
-	if (dfl_phys_nid == NUMA_NO_NODE) {
-		pr_warning("NUMA: Warning: can't determine default physical node, disabling emulation\n");
-		goto no_emu;
-	}
-
 	/* commit */
 	*numa_meminfo = ei;
 
-- 
2.11.0

Re: [PATCH V2 1/3] x86/numa_emulation: fix potential memory leak

[Borislav Petkov]

On Tue, May 02, 2017 at 09:04:51PM +0800, Wei Yang wrote:
> numa_emulation() needs to allocate a space for phys_dist[] temporarily,

s/a //

> while current code may miss to release this when dfl_phys_nid ==
> NUMA_NO_NODE.

And when is "dfl_phys_nid == NUMA_NO_NODE"? What does it mean actually?

> It is observed in code review instead of in a real case.
> This patch fixes this by re-order the code path.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Acked-by: David Rientjes <rientjes@google.com>
> ---
>  arch/x86/mm/numa_emulation.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/mm/numa_emulation.c b/arch/x86/mm/numa_emulation.c
> index a8f90ce3dedf..eb017c816de6 100644
> --- a/arch/x86/mm/numa_emulation.c
> +++ b/arch/x86/mm/numa_emulation.c
> @@ -353,6 +353,24 @@ void __init numa_emulation(struct numa_meminfo *numa_meminfo, int numa_dist_cnt)
>  		goto no_emu;
>  	}
>  
> +	/*
> +	 * Determine the max emulated nid and the default phys nid to use
> +	 * for unmapped nodes.
> +	 */
> +	max_emu_nid = 0;
> +	dfl_phys_nid = NUMA_NO_NODE;
> +	for (i = 0; i < ARRAY_SIZE(emu_nid_to_phys); i++) {
> +		if (emu_nid_to_phys[i] != NUMA_NO_NODE) {
> +			max_emu_nid = i;
> +			if (dfl_phys_nid == NUMA_NO_NODE)
> +				dfl_phys_nid = emu_nid_to_phys[i];
> +		}
> +	}
> +	if (dfl_phys_nid == NUMA_NO_NODE) {
> +		pr_warn("NUMA: Warning: can't determine default physical node, disabling emulation\n");
> +		goto no_emu;
> +	}
> +

Well, that function numa_emulation() does a looot of things and could
very well be split into subfunctions, which should make the whole path
more readable.

And this chunk you're moving is kinda begging to be a separate
function...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 1/3] x86/numa_emulation: fix potential memory leak

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 2678 bytes --]

On Mon, Jun 26, 2017 at 05:31:49PM +0200, Borislav Petkov wrote:
>On Tue, May 02, 2017 at 09:04:51PM +0800, Wei Yang wrote:
>> numa_emulation() needs to allocate a space for phys_dist[] temporarily,
>
>s/a //
>
>> while current code may miss to release this when dfl_phys_nid ==
>> NUMA_NO_NODE.
>
>And when is "dfl_phys_nid == NUMA_NO_NODE"? What does it mean actually?
>

It means numa emulation is not properly configured.

>> It is observed in code review instead of in a real case.
>> This patch fixes this by re-order the code path.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> Acked-by: David Rientjes <rientjes@google.com>
>> ---
>>  arch/x86/mm/numa_emulation.c | 36 ++++++++++++++++++------------------
>>  1 file changed, 18 insertions(+), 18 deletions(-)
>> 
>> diff --git a/arch/x86/mm/numa_emulation.c b/arch/x86/mm/numa_emulation.c
>> index a8f90ce3dedf..eb017c816de6 100644
>> --- a/arch/x86/mm/numa_emulation.c
>> +++ b/arch/x86/mm/numa_emulation.c
>> @@ -353,6 +353,24 @@ void __init numa_emulation(struct numa_meminfo *numa_meminfo, int numa_dist_cnt)
>>  		goto no_emu;
>>  	}
>>  
>> +	/*
>> +	 * Determine the max emulated nid and the default phys nid to use
>> +	 * for unmapped nodes.
>> +	 */
>> +	max_emu_nid = 0;
>> +	dfl_phys_nid = NUMA_NO_NODE;
>> +	for (i = 0; i < ARRAY_SIZE(emu_nid_to_phys); i++) {
>> +		if (emu_nid_to_phys[i] != NUMA_NO_NODE) {
>> +			max_emu_nid = i;
>> +			if (dfl_phys_nid == NUMA_NO_NODE)
>> +				dfl_phys_nid = emu_nid_to_phys[i];
>> +		}
>> +	}
>> +	if (dfl_phys_nid == NUMA_NO_NODE) {
>> +		pr_warn("NUMA: Warning: can't determine default physical node, disabling emulation\n");
>> +		goto no_emu;
>> +	}
>> +
>
>Well, that function numa_emulation() does a looot of things and could
>very well be split into subfunctions, which should make the whole path
>more readable.
>

You are right. The whole function contains several blocks which could be
split. While this patch focus on the memory leak issue. For readable code, we
could come up with a separate patch to refine it.

>And this chunk you're moving is kinda begging to be a separate
>function...

Well, to this particular piece, have a for loop within a function doesn't look
like a big deal to me. So you prefer to take every for loop in this function
out?

Last but not the least, these are two issues:

The problem this patch wants to address is the memory leak, while the concern
here you mentioned is the coding style.

>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.

-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2 1/3] x86/numa_emulation: fix potential memory leak

[Borislav Petkov]

On Tue, Jun 27, 2017 at 07:11:27AM +0800, Wei Yang wrote:
> It means numa emulation is not properly configured.

Or what the error message says: it cannot determine the default physical
node because NUMA emulation is not properly configured. What I'm trying
to say, is, explain the *why* in the commit message, not the *what*. The
*what* one can see in the code.

> Well, to this particular piece, have a for loop within a function doesn't look
> like a big deal to me. So you prefer to take every for loop in this function
> out?

As I said, I'd prefer you take this loop out and turn it into a separate
function in one go, along with fixing the potential memory leak.

> Last but not the least, these are two issues:
> 
> The problem this patch wants to address is the memory leak, while the concern
> here you mentioned is the coding style.

Let's not get too pedantic here: if you carve it out in a separate
function, it is still clear what the patch is doing.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 1/3] x86/numa_emulation: fix potential memory leak

[Wei Yang]

On Wed, Jun 28, 2017 at 2:10 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, Jun 27, 2017 at 07:11:27AM +0800, Wei Yang wrote:
>> It means numa emulation is not properly configured.
>
> Or what the error message says: it cannot determine the default physical
> node because NUMA emulation is not properly configured. What I'm trying
> to say, is, explain the *why* in the commit message, not the *what*. The
> *what* one can see in the code.
>

I didn't dig into the reason for when this could happen.

After some investigation, it looks will not happen after split_nodes_xxx()
works fine. In function split_nodes_xxx(), if it doesn't return an error code
it will set the emu_nid_to_phys[]. Which in turns be assigned to dfl_phys_nid.

So I suggest to remove the error branch.

>> Well, to this particular piece, have a for loop within a function doesn't look
>> like a big deal to me. So you prefer to take every for loop in this function
>> out?
>
> As I said, I'd prefer you take this loop out and turn it into a separate
> function in one go, along with fixing the potential memory leak.
>
>> Last but not the least, these are two issues:
>>
>> The problem this patch wants to address is the memory leak, while the concern
>> here you mentioned is the coding style.
>
> Let's not get too pedantic here: if you carve it out in a separate
> function, it is still clear what the patch is doing.
>

Ok, will do this.

> --
> Regards/Gruss,
>     Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH V2 2/3] x86/numa_emulation: assign physnode_mask directly from numa_nodes_parsed

[Wei Yang]

numa_init() has already called init_func(), which is responsible for
setting numa_nodes_parsed, so use this nodemask instead of re-finding it
when calling numa_emulation().

This patch gets the physnode_mask directly from numa_nodes_parsed. At
the same time, it corrects the comment of these two functions.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Acked-by: David Rientjes <rientjes@google.com>

---
v2: refresh the change log based on David Rientjes comment
---
 arch/x86/mm/numa_emulation.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/x86/mm/numa_emulation.c b/arch/x86/mm/numa_emulation.c
index eb017c816de6..a6d01931b9a1 100644
--- a/arch/x86/mm/numa_emulation.c
+++ b/arch/x86/mm/numa_emulation.c
@@ -75,13 +75,15 @@ static int __init emu_setup_memblk(struct numa_meminfo *ei,
 
 /*
  * Sets up nr_nodes fake nodes interleaved over physical nodes ranging from addr
- * to max_addr.  The return value is the number of nodes allocated.
+ * to max_addr.
+ *
+ * Returns zero on success or negative error code.
  */
 static int __init split_nodes_interleave(struct numa_meminfo *ei,
 					 struct numa_meminfo *pi,
 					 u64 addr, u64 max_addr, int nr_nodes)
 {
-	nodemask_t physnode_mask = NODE_MASK_NONE;
+	nodemask_t physnode_mask = numa_nodes_parsed;
 	u64 size;
 	int big;
 	int nid = 0;
@@ -116,9 +118,6 @@ static int __init split_nodes_interleave(struct numa_meminfo *ei,
 		return -1;
 	}
 
-	for (i = 0; i < pi->nr_blks; i++)
-		node_set(pi->blk[i].nid, physnode_mask);
-
 	/*
 	 * Continue to fill physical nodes with fake nodes until there is no
 	 * memory left on any of them.
@@ -200,13 +199,15 @@ static u64 __init find_end_of_node(u64 start, u64 max_addr, u64 size)
 
 /*
  * Sets up fake nodes of `size' interleaved over physical nodes ranging from
- * `addr' to `max_addr'.  The return value is the number of nodes allocated.
+ * `addr' to `max_addr'.
+ *
+ * Returns zero on success or negative error code.
  */
 static int __init split_nodes_size_interleave(struct numa_meminfo *ei,
 					      struct numa_meminfo *pi,
 					      u64 addr, u64 max_addr, u64 size)
 {
-	nodemask_t physnode_mask = NODE_MASK_NONE;
+	nodemask_t physnode_mask = numa_nodes_parsed;
 	u64 min_size;
 	int nid = 0;
 	int i, ret;
@@ -231,9 +232,6 @@ static int __init split_nodes_size_interleave(struct numa_meminfo *ei,
 	}
 	size &= FAKE_NODE_MIN_HASH_MASK;
 
-	for (i = 0; i < pi->nr_blks; i++)
-		node_set(pi->blk[i].nid, physnode_mask);
-
 	/*
 	 * Fill physical nodes with fake nodes of size until there is no memory
 	 * left on any of them.
-- 
2.11.0

Re: [PATCH V2 2/3] x86/numa_emulation: assign physnode_mask directly from numa_nodes_parsed

[Borislav Petkov]

On Tue, May 02, 2017 at 09:04:52PM +0800, Wei Yang wrote:
> numa_init() has already called init_func(), which is responsible for
> setting numa_nodes_parsed, so use this nodemask instead of re-finding it
> when calling numa_emulation().
> 
> This patch gets the physnode_mask directly from numa_nodes_parsed. At
> the same time, it corrects the comment of these two functions.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Acked-by: David Rientjes <rientjes@google.com>
> 
> ---
> v2: refresh the change log based on David Rientjes comment
> ---
>  arch/x86/mm/numa_emulation.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/mm/numa_emulation.c b/arch/x86/mm/numa_emulation.c
> index eb017c816de6..a6d01931b9a1 100644
> --- a/arch/x86/mm/numa_emulation.c
> +++ b/arch/x86/mm/numa_emulation.c
> @@ -75,13 +75,15 @@ static int __init emu_setup_memblk(struct numa_meminfo *ei,
>  
>  /*
>   * Sets up nr_nodes fake nodes interleaved over physical nodes ranging from addr
> - * to max_addr.  The return value is the number of nodes allocated.
> + * to max_addr.
> + *
> + * Returns zero on success or negative error code.

						     ... on error.

Other than that:

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 2/3] x86/numa_emulation: assign physnode_mask directly from numa_nodes_parsed

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On Mon, Jun 26, 2017 at 08:40:26PM +0200, Borislav Petkov wrote:
>On Tue, May 02, 2017 at 09:04:52PM +0800, Wei Yang wrote:
>> numa_init() has already called init_func(), which is responsible for
>> setting numa_nodes_parsed, so use this nodemask instead of re-finding it
>> when calling numa_emulation().
>> 
>> This patch gets the physnode_mask directly from numa_nodes_parsed. At
>> the same time, it corrects the comment of these two functions.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> Acked-by: David Rientjes <rientjes@google.com>
>> 
>> ---
>> v2: refresh the change log based on David Rientjes comment
>> ---
>>  arch/x86/mm/numa_emulation.c | 18 ++++++++----------
>>  1 file changed, 8 insertions(+), 10 deletions(-)
>> 
>> diff --git a/arch/x86/mm/numa_emulation.c b/arch/x86/mm/numa_emulation.c
>> index eb017c816de6..a6d01931b9a1 100644
>> --- a/arch/x86/mm/numa_emulation.c
>> +++ b/arch/x86/mm/numa_emulation.c
>> @@ -75,13 +75,15 @@ static int __init emu_setup_memblk(struct numa_meminfo *ei,
>>  
>>  /*
>>   * Sets up nr_nodes fake nodes interleaved over physical nodes ranging from addr
>> - * to max_addr.  The return value is the number of nodes allocated.
>> + * to max_addr.
>> + *
>> + * Returns zero on success or negative error code.
>
>						     ... on error.
>
>Other than that:
>
>Reviewed-by: Borislav Petkov <bp@suse.de>
>

Thanks

>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.

-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH V2 3/3] x86/numa_emulation: restructures numa_nodes_parsed from emulated nodes

[Wei Yang]

By emulating numa, it may contains more or less nodes than physical nodes.
In numa_emulation(), numa_meminfo/numa_distance/__apicid_to_node is
restructured according to emulated nodes, except numa_nodes_parsed.

This patch restructures numa_nodes_parsed from emulated nodes.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Acked-by: David Rientjes <rientjes@google.com>

---
v2: use nodes_clear(numa_nodes_parsedd);
---
 arch/x86/mm/numa_emulation.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/mm/numa_emulation.c b/arch/x86/mm/numa_emulation.c
index a6d01931b9a1..6e5ccbfc3deb 100644
--- a/arch/x86/mm/numa_emulation.c
+++ b/arch/x86/mm/numa_emulation.c
@@ -391,6 +391,13 @@ void __init numa_emulation(struct numa_meminfo *numa_meminfo, int numa_dist_cnt)
 	/* commit */
 	*numa_meminfo = ei;
 
+	/* Make sure numa_nodes_parsed only contains emulated nodes */
+	nodes_clear(numa_nodes_parsed);
+	for (i = 0; i < ARRAY_SIZE(ei.blk); i++)
+		if (ei.blk[i].start != ei.blk[i].end &&
+		    ei.blk[i].nid != NUMA_NO_NODE)
+			node_set(ei.blk[i].nid, numa_nodes_parsed);
+
 	/*
 	 * Transform __apicid_to_node table to use emulated nids by
 	 * reverse-mapping phys_nid.  The maps should always exist but fall
-- 
2.11.0

Re: [PATCH V2 0/3] Refine numa_emulation

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]

Ping~ Willing to hear some feed back :-)

On Tue, May 02, 2017 at 09:04:50PM +0800, Wei Yang wrote:
>My previous patch "x86/mm/numa: Remove numa_nodemask_from_meminfo()" hits a
>problem in numa_emulation. The reason is numa_nodes_parsed is not set
>correctly after emulation.
>
>This patch set tries to fix this and also with two code refine.
>
>Detailed discussions are in this thread:
>
>    https://lkml.org/lkml/2017/3/13/1230
>
>and test result is posted :
>
>    https://lkml.org/lkml/2017/4/10/641
>
>V2:
>    * refresh the change log based on David comments
>    * use nodes_clear()
>
>Wei Yang (3):
>  x86/numa_emulation: fix potential memory leak
>  x86/numa_emulation: assign physnode_mask directly from
>    numa_nodes_parsed
>  x86/numa_emulation: restructures numa_nodes_parsed from emulated nodes
>
> arch/x86/mm/numa_emulation.c | 61 ++++++++++++++++++++++++--------------------
> 1 file changed, 33 insertions(+), 28 deletions(-)
>
>-- 
>2.11.0

-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2 0/3] Refine numa_emulation

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

This has really been a long time, not sure we have any concern on this?

This change looks good to me which make the numa emulation more robust.
And David has acked.

Welcome any comments on this.

On Tue, May 02, 2017 at 09:04:50PM +0800, Wei Yang wrote:
>My previous patch "x86/mm/numa: Remove numa_nodemask_from_meminfo()" hits a
>problem in numa_emulation. The reason is numa_nodes_parsed is not set
>correctly after emulation.
>
>This patch set tries to fix this and also with two code refine.
>
>Detailed discussions are in this thread:
>
>    https://lkml.org/lkml/2017/3/13/1230
>
>and test result is posted :
>
>    https://lkml.org/lkml/2017/4/10/641
>
>V2:
>    * refresh the change log based on David comments
>    * use nodes_clear()
>
>Wei Yang (3):
>  x86/numa_emulation: fix potential memory leak
>  x86/numa_emulation: assign physnode_mask directly from
>    numa_nodes_parsed
>  x86/numa_emulation: restructures numa_nodes_parsed from emulated nodes
>
> arch/x86/mm/numa_emulation.c | 61 ++++++++++++++++++++++++--------------------
> 1 file changed, 33 insertions(+), 28 deletions(-)
>
>-- 
>2.11.0

-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]