[PATCH] x86/numa: move setting parse numa node to num_add_memblk

[PATCH] x86/numa: move setting parse numa node to num_add_memblk

[zhong jiang]

Currently, Arm64 and x86 use the common code wehn parsing numa node
in a acpi way. The arm64 will set the parsed node in numa_add_memblk,
but the x86 is not set in that , then it will result in the repeatly
setting. And the parsed node maybe is  unreasonable to the system.

we would better not set it although it also still works. because the
parsed node is unresonable. so we should skip related operate in this
node. This patch just set node in various architecture individually.
it is no functional change.

Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
 arch/x86/mm/amdtopology.c | 1 -
 arch/x86/mm/numa.c        | 3 ++-
 drivers/acpi/numa.c       | 5 ++++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/mm/amdtopology.c b/arch/x86/mm/amdtopology.c
index 91f501b..7657042 100644
--- a/arch/x86/mm/amdtopology.c
+++ b/arch/x86/mm/amdtopology.c
@@ -151,7 +151,6 @@ int __init amd_numa_init(void)
 
 		prevbase = base;
 		numa_add_memblk(nodeid, base, limit);
-		node_set(nodeid, numa_nodes_parsed);
 	}
 
 	if (!nodes_weight(numa_nodes_parsed))
diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index 25504d5..8f87f26 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -150,6 +150,8 @@ static int __init numa_add_memblk_to(int nid, u64 start, u64 end,
 	mi->blk[mi->nr_blks].end = end;
 	mi->blk[mi->nr_blks].nid = nid;
 	mi->nr_blks++;
+
+	node_set(nid, numa_nodes_parsed);
 	return 0;
 }
 
@@ -693,7 +695,6 @@ static int __init dummy_numa_init(void)
 	printk(KERN_INFO "Faking a node at [mem %#018Lx-%#018Lx]\n",
 	       0LLU, PFN_PHYS(max_pfn) - 1);
 
-	node_set(0, numa_nodes_parsed);
 	numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
 
 	return 0;
diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c
index 917f1cc..f2e33cb 100644
--- a/drivers/acpi/numa.c
+++ b/drivers/acpi/numa.c
@@ -294,7 +294,9 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
 		goto out_err_bad_srat;
 	}
 
-	node_set(node, numa_nodes_parsed);
+	/* some architecture is likely to ignore a unreasonable node */
+	if (!node_isset(node, numa_nodes_parsed))
+		goto out;
 
 	pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s%s\n",
 		node, pxm,
@@ -309,6 +311,7 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
 
 	max_possible_pfn = max(max_possible_pfn, PFN_UP(end - 1));
 
+out:
 	return 0;
 out_err_bad_srat:
 	bad_srat();
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/numa: move setting parse numa node to num_add_memblk

[Michal Hocko]

On Wed 29-11-17 17:13:27, zhong jiang wrote:
> Currently, Arm64 and x86 use the common code wehn parsing numa node
> in a acpi way. The arm64 will set the parsed node in numa_add_memblk,
> but the x86 is not set in that , then it will result in the repeatly
> setting. And the parsed node maybe is  unreasonable to the system.
> 
> we would better not set it although it also still works. because the
> parsed node is unresonable. so we should skip related operate in this
> node. This patch just set node in various architecture individually.
> it is no functional change.

I really have hard time to understand what you try to say above. Could
you start by the problem description and then how you are addressing it?

> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  arch/x86/mm/amdtopology.c | 1 -
>  arch/x86/mm/numa.c        | 3 ++-
>  drivers/acpi/numa.c       | 5 ++++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/mm/amdtopology.c b/arch/x86/mm/amdtopology.c
> index 91f501b..7657042 100644
> --- a/arch/x86/mm/amdtopology.c
> +++ b/arch/x86/mm/amdtopology.c
> @@ -151,7 +151,6 @@ int __init amd_numa_init(void)
>  
>  		prevbase = base;
>  		numa_add_memblk(nodeid, base, limit);
> -		node_set(nodeid, numa_nodes_parsed);
>  	}
>  
>  	if (!nodes_weight(numa_nodes_parsed))
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index 25504d5..8f87f26 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -150,6 +150,8 @@ static int __init numa_add_memblk_to(int nid, u64 start, u64 end,
>  	mi->blk[mi->nr_blks].end = end;
>  	mi->blk[mi->nr_blks].nid = nid;
>  	mi->nr_blks++;
> +
> +	node_set(nid, numa_nodes_parsed);
>  	return 0;
>  }
>  
> @@ -693,7 +695,6 @@ static int __init dummy_numa_init(void)
>  	printk(KERN_INFO "Faking a node at [mem %#018Lx-%#018Lx]\n",
>  	       0LLU, PFN_PHYS(max_pfn) - 1);
>  
> -	node_set(0, numa_nodes_parsed);
>  	numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
>  
>  	return 0;
> diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c
> index 917f1cc..f2e33cb 100644
> --- a/drivers/acpi/numa.c
> +++ b/drivers/acpi/numa.c
> @@ -294,7 +294,9 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
>  		goto out_err_bad_srat;
>  	}
>  
> -	node_set(node, numa_nodes_parsed);
> +	/* some architecture is likely to ignore a unreasonable node */
> +	if (!node_isset(node, numa_nodes_parsed))
> +		goto out;
>  
>  	pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s%s\n",
>  		node, pxm,
> @@ -309,6 +311,7 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
>  
>  	max_possible_pfn = max(max_possible_pfn, PFN_UP(end - 1));
>  
> +out:
>  	return 0;
>  out_err_bad_srat:
>  	bad_srat();
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/numa: move setting parse numa node to num_add_memblk

[zhong jiang]

On 2017/11/29 20:03, Michal Hocko wrote:
> On Wed 29-11-17 17:13:27, zhong jiang wrote:
>> Currently, Arm64 and x86 use the common code wehn parsing numa node
>> in a acpi way. The arm64 will set the parsed node in numa_add_memblk,
>> but the x86 is not set in that , then it will result in the repeatly
>> setting. And the parsed node maybe is  unreasonable to the system.
>>
>> we would better not set it although it also still works. because the
>> parsed node is unresonable. so we should skip related operate in this
>> node. This patch just set node in various architecture individually.
>> it is no functional change.
> I really have hard time to understand what you try to say above. Could
> you start by the problem description and then how you are addressing it?
  I am so sorry for that.  I will make the issue clear.
 
  Arm64  get numa information through acpi.  The code flow is as follows.

  arm64_acpi_numa_init
       acpi_parse_memory_affinity
          acpi_numa_memory_affinity_init
              numa_add_memblk(nid, start, end);      //it will set node to numa_nodes_parsed successfully.
              node_set(node, numa_nodes_parsed);     // numa_add_memblk had set that.  it will repeat.

  the root cause is that X86 parse numa also  go through above code.  and  arch-related
  numa_add_memblk  is not set the parsed node to numa_nodes_parsed.  it need
  additional node_set(node, numa_parsed) to handle.  therefore,  the issue will be introduced.

  menawhile,  the parsed node is meaningless when numa_add_memblk fails and return 0.
 so we should bail out in time.
 
is it a little clearer ?

 Thanks
 zhongjiang
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>>  arch/x86/mm/amdtopology.c | 1 -
>>  arch/x86/mm/numa.c        | 3 ++-
>>  drivers/acpi/numa.c       | 5 ++++-
>>  3 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/mm/amdtopology.c b/arch/x86/mm/amdtopology.c
>> index 91f501b..7657042 100644
>> --- a/arch/x86/mm/amdtopology.c
>> +++ b/arch/x86/mm/amdtopology.c
>> @@ -151,7 +151,6 @@ int __init amd_numa_init(void)
>>  
>>  		prevbase = base;
>>  		numa_add_memblk(nodeid, base, limit);
>> -		node_set(nodeid, numa_nodes_parsed);
>>  	}
>>  
>>  	if (!nodes_weight(numa_nodes_parsed))
>> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
>> index 25504d5..8f87f26 100644
>> --- a/arch/x86/mm/numa.c
>> +++ b/arch/x86/mm/numa.c
>> @@ -150,6 +150,8 @@ static int __init numa_add_memblk_to(int nid, u64 start, u64 end,
>>  	mi->blk[mi->nr_blks].end = end;
>>  	mi->blk[mi->nr_blks].nid = nid;
>>  	mi->nr_blks++;
>> +
>> +	node_set(nid, numa_nodes_parsed);
>>  	return 0;
>>  }
>>  
>> @@ -693,7 +695,6 @@ static int __init dummy_numa_init(void)
>>  	printk(KERN_INFO "Faking a node at [mem %#018Lx-%#018Lx]\n",
>>  	       0LLU, PFN_PHYS(max_pfn) - 1);
>>  
>> -	node_set(0, numa_nodes_parsed);
>>  	numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
>>  
>>  	return 0;
>> diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c
>> index 917f1cc..f2e33cb 100644
>> --- a/drivers/acpi/numa.c
>> +++ b/drivers/acpi/numa.c
>> @@ -294,7 +294,9 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
>>  		goto out_err_bad_srat;
>>  	}
>>  
>> -	node_set(node, numa_nodes_parsed);
>> +	/* some architecture is likely to ignore a unreasonable node */
>> +	if (!node_isset(node, numa_nodes_parsed))
>> +		goto out;
>>  
>>  	pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s%s\n",
>>  		node, pxm,
>> @@ -309,6 +311,7 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
>>  
>>  	max_possible_pfn = max(max_possible_pfn, PFN_UP(end - 1));
>>  
>> +out:
>>  	return 0;
>>  out_err_bad_srat:
>>  	bad_srat();
>> -- 
>> 1.8.3.1


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/numa: move setting parse numa node to num_add_memblk

[Michal Hocko]

On Wed 29-11-17 20:41:25, zhong jiang wrote:
> On 2017/11/29 20:03, Michal Hocko wrote:
> > On Wed 29-11-17 17:13:27, zhong jiang wrote:
> >> Currently, Arm64 and x86 use the common code wehn parsing numa node
> >> in a acpi way. The arm64 will set the parsed node in numa_add_memblk,
> >> but the x86 is not set in that , then it will result in the repeatly
> >> setting. And the parsed node maybe is  unreasonable to the system.
> >>
> >> we would better not set it although it also still works. because the
> >> parsed node is unresonable. so we should skip related operate in this
> >> node. This patch just set node in various architecture individually.
> >> it is no functional change.
> > I really have hard time to understand what you try to say above. Could
> > you start by the problem description and then how you are addressing it?
>   I am so sorry for that.  I will make the issue clear.
>  
>   Arm64  get numa information through acpi.  The code flow is as follows.
> 
>   arm64_acpi_numa_init
>        acpi_parse_memory_affinity
>           acpi_numa_memory_affinity_init
>               numa_add_memblk(nid, start, end);      //it will set node to numa_nodes_parsed successfully.
>               node_set(node, numa_nodes_parsed);     // numa_add_memblk had set that.  it will repeat.
> 
>  the root cause is that X86 parse numa also  go through above code.  and  arch-related
>  numa_add_memblk  is not set the parsed node to numa_nodes_parsed.  it need
>  additional node_set(node, numa_parsed) to handle.  therefore,  the issue will be introduced.
> 

No it is not much more clear. I would have to go and re-study the whole
code flow to see what you mean here. So you could simply state what _the
issue_ is? How can user observe it and what are the consequences?

Sorry for my laziness, I could go and read the code but the primary
point of the changelog is to be _clear_ about the problem and the fix.
Call paths can help reviewers but the scope should be clear even without
them.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/numa: move setting parse numa node to num_add_memblk

[zhong jiang]

On 2017/11/29 21:01, Michal Hocko wrote:
> On Wed 29-11-17 20:41:25, zhong jiang wrote:
>> On 2017/11/29 20:03, Michal Hocko wrote:
>>> On Wed 29-11-17 17:13:27, zhong jiang wrote:
>>>> Currently, Arm64 and x86 use the common code wehn parsing numa node
>>>> in a acpi way. The arm64 will set the parsed node in numa_add_memblk,
>>>> but the x86 is not set in that , then it will result in the repeatly
>>>> setting. And the parsed node maybe is  unreasonable to the system.
>>>>
>>>> we would better not set it although it also still works. because the
>>>> parsed node is unresonable. so we should skip related operate in this
>>>> node. This patch just set node in various architecture individually.
>>>> it is no functional change.
>>> I really have hard time to understand what you try to say above. Could
>>> you start by the problem description and then how you are addressing it?
>>   I am so sorry for that.  I will make the issue clear.
>>  
>>   Arm64  get numa information through acpi.  The code flow is as follows.
>>
>>   arm64_acpi_numa_init
>>        acpi_parse_memory_affinity
>>           acpi_numa_memory_affinity_init
>>               numa_add_memblk(nid, start, end);      //it will set node to numa_nodes_parsed successfully.
>>               node_set(node, numa_nodes_parsed);     // numa_add_memblk had set that.  it will repeat.
>>
>>  the root cause is that X86 parse numa also  go through above code.  and  arch-related
>>  numa_add_memblk  is not set the parsed node to numa_nodes_parsed.  it need
>>  additional node_set(node, numa_parsed) to handle.  therefore,  the issue will be introduced.
>>
> No it is not much more clear. I would have to go and re-study the whole
> code flow to see what you mean here. So you could simply state what _the
> issue_ is? How can user observe it and what are the consequences?
  The patch do not fix a real issue.  it is a cleanup.
  because the acpi code  is public,  I find they are messy between
  Arch64 and X86 when parsing numa message .  therefore,  I try to
  make the code more clear between them.

  Thanks
  zhongjiang
> Sorry for my laziness, I could go and read the code but the primary
> point of the changelog is to be _clear_ about the problem and the fix.
> Call paths can help reviewers but the scope should be clear even without
> them.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/numa: move setting parse numa node to num_add_memblk

[Michal Hocko]

On Wed 29-11-17 21:26:19, zhong jiang wrote:
> On 2017/11/29 21:01, Michal Hocko wrote:
> > On Wed 29-11-17 20:41:25, zhong jiang wrote:
> >> On 2017/11/29 20:03, Michal Hocko wrote:
> >>> On Wed 29-11-17 17:13:27, zhong jiang wrote:
> >>>> Currently, Arm64 and x86 use the common code wehn parsing numa node
> >>>> in a acpi way. The arm64 will set the parsed node in numa_add_memblk,
> >>>> but the x86 is not set in that , then it will result in the repeatly
> >>>> setting. And the parsed node maybe is  unreasonable to the system.
> >>>>
> >>>> we would better not set it although it also still works. because the
> >>>> parsed node is unresonable. so we should skip related operate in this
> >>>> node. This patch just set node in various architecture individually.
> >>>> it is no functional change.
> >>> I really have hard time to understand what you try to say above. Could
> >>> you start by the problem description and then how you are addressing it?
> >>   I am so sorry for that.  I will make the issue clear.
> >>  
> >>   Arm64  get numa information through acpi.  The code flow is as follows.
> >>
> >>   arm64_acpi_numa_init
> >>        acpi_parse_memory_affinity
> >>           acpi_numa_memory_affinity_init
> >>               numa_add_memblk(nid, start, end);      //it will set node to numa_nodes_parsed successfully.
> >>               node_set(node, numa_nodes_parsed);     // numa_add_memblk had set that.  it will repeat.
> >>
> >>  the root cause is that X86 parse numa also  go through above code.  and  arch-related
> >>  numa_add_memblk  is not set the parsed node to numa_nodes_parsed.  it need
> >>  additional node_set(node, numa_parsed) to handle.  therefore,  the issue will be introduced.
> >>
> > No it is not much more clear. I would have to go and re-study the whole
> > code flow to see what you mean here. So you could simply state what _the
> > issue_ is? How can user observe it and what are the consequences?
>   The patch do not fix a real issue.  it is a cleanup.
>   because the acpi code  is public,  I find they are messy between
>   Arch64 and X86 when parsing numa message .  therefore,  I try to
>   make the code more clear between them.

So make this explicit in the changelog. Your previous wording sounded
like there is a _problem_ in the code.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/numa: move setting parse numa node to num_add_memblk

[zhong jiang]

On 2017/11/29 21:33, Michal Hocko wrote:
> On Wed 29-11-17 21:26:19, zhong jiang wrote:
>> On 2017/11/29 21:01, Michal Hocko wrote:
>>> On Wed 29-11-17 20:41:25, zhong jiang wrote:
>>>> On 2017/11/29 20:03, Michal Hocko wrote:
>>>>> On Wed 29-11-17 17:13:27, zhong jiang wrote:
>>>>>> Currently, Arm64 and x86 use the common code wehn parsing numa node
>>>>>> in a acpi way. The arm64 will set the parsed node in numa_add_memblk,
>>>>>> but the x86 is not set in that , then it will result in the repeatly
>>>>>> setting. And the parsed node maybe is  unreasonable to the system.
>>>>>>
>>>>>> we would better not set it although it also still works. because the
>>>>>> parsed node is unresonable. so we should skip related operate in this
>>>>>> node. This patch just set node in various architecture individually.
>>>>>> it is no functional change.
>>>>> I really have hard time to understand what you try to say above. Could
>>>>> you start by the problem description and then how you are addressing it?
>>>>   I am so sorry for that.  I will make the issue clear.
>>>>  
>>>>   Arm64  get numa information through acpi.  The code flow is as follows.
>>>>
>>>>   arm64_acpi_numa_init
>>>>        acpi_parse_memory_affinity
>>>>           acpi_numa_memory_affinity_init
>>>>               numa_add_memblk(nid, start, end);      //it will set node to numa_nodes_parsed successfully.
>>>>               node_set(node, numa_nodes_parsed);     // numa_add_memblk had set that.  it will repeat.
>>>>
>>>>  the root cause is that X86 parse numa also  go through above code.  and  arch-related
>>>>  numa_add_memblk  is not set the parsed node to numa_nodes_parsed.  it need
>>>>  additional node_set(node, numa_parsed) to handle.  therefore,  the issue will be introduced.
>>>>
>>> No it is not much more clear. I would have to go and re-study the whole
>>> code flow to see what you mean here. So you could simply state what _the
>>> issue_ is? How can user observe it and what are the consequences?
>>   The patch do not fix a real issue.  it is a cleanup.
>>   because the acpi code  is public,  I find they are messy between
>>   Arch64 and X86 when parsing numa message .  therefore,  I try to
>>   make the code more clear between them.
> So make this explicit in the changelog. Your previous wording sounded
> like there is a _problem_ in the code.
>
:-[       please take some time to check.  if it works.  I will resend v2 with detailed changelog.

Thanks
zhongjiang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/numa: move setting parse numa node to num_add_memblk

[Dou Liyang]

Hi Jiang,

At 11/29/2017 09:44 PM, zhong jiang wrote:
> On 2017/11/29 21:33, Michal Hocko wrote:
>> On Wed 29-11-17 21:26:19, zhong jiang wrote:
>>> On 2017/11/29 21:01, Michal Hocko wrote:
>>>> On Wed 29-11-17 20:41:25, zhong jiang wrote:
>>>>> On 2017/11/29 20:03, Michal Hocko wrote:
>>>>>> On Wed 29-11-17 17:13:27, zhong jiang wrote:
>>>>>>> Currently, Arm64 and x86 use the common code wehn parsing numa node
>>>>>>> in a acpi way. The arm64 will set the parsed node in numa_add_memblk,
>>>>>>> but the x86 is not set in that , then it will result in the repeatly
>>>>>>> setting. And the parsed node maybe is  unreasonable to the system.
>>>>>>>
>>>>>>> we would better not set it although it also still works. because the
>>>>>>> parsed node is unresonable. so we should skip related operate in this
>>>>>>> node. This patch just set node in various architecture individually.
>>>>>>> it is no functional change.
>>>>>> I really have hard time to understand what you try to say above. Could
>>>>>> you start by the problem description and then how you are addressing it?
>>>>>   I am so sorry for that.  I will make the issue clear.
>>>>>
>>>>>   Arm64  get numa information through acpi.  The code flow is as follows.
>>>>>
>>>>>   arm64_acpi_numa_init
>>>>>        acpi_parse_memory_affinity
>>>>>           acpi_numa_memory_affinity_init
>>>>>               numa_add_memblk(nid, start, end);      //it will set node to numa_nodes_parsed successfully.
>>>>>               node_set(node, numa_nodes_parsed);     // numa_add_memblk had set that.  it will repeat.
>>>>>
>>>>>  the root cause is that X86 parse numa also  go through above code.  and  arch-related
>>>>>  numa_add_memblk  is not set the parsed node to numa_nodes_parsed.  it need
>>>>>  additional node_set(node, numa_parsed) to handle.  therefore,  the issue will be introduced.
>>>>>
>>>> No it is not much more clear. I would have to go and re-study the whole
>>>> code flow to see what you mean here. So you could simply state what _the
>>>> issue_ is? How can user observe it and what are the consequences?
>>>   The patch do not fix a real issue.  it is a cleanup.

 > @@ -294,7 +294,9 @@ void __init acpi_numa_slit_init(struct 
acpi_table_slit *slit)
 >  		goto out_err_bad_srat;
 >  	}
 >
 > -	node_set(node, numa_nodes_parsed);
 > +	/* some architecture is likely to ignore a unreasonable node */
 > +	if (!node_isset(node, numa_nodes_parsed))
 > +		goto out;
 >

It is not just a cleanup patch,	Here you change the original logic.

With this patch, we just set the *numa_nodes_parsed* after NUMA adds a
memblk successfully and also add a check here for bypassing the invalid
memblk node.

I am not sure which arch may meet this situation? did you test this
patch?

Anyway, AFAIK, The ACPI tables are very much like user input in that
respect and they are unreasonable. So the patch is better.

Thanks,
	dou.

>>>   because the acpi code  is public,  I find they are messy between
>>>   Arch64 and X86 when parsing numa message .  therefore,  I try to
>>>   make the code more clear between them.
>> So make this explicit in the changelog. Your previous wording sounded
>> like there is a _problem_ in the code.
>>
> :-[       please take some time to check.  if it works.  I will resend v2 with detailed changelog.
>
> Thanks
> zhongjiang
>
>
>
>


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/numa: move setting parse numa node to num_add_memblk

[zhong jiang]

On 2017/11/29 22:14, Dou Liyang wrote:
> Hi Jiang,
>
> At 11/29/2017 09:44 PM, zhong jiang wrote:
>> On 2017/11/29 21:33, Michal Hocko wrote:
>>> On Wed 29-11-17 21:26:19, zhong jiang wrote:
>>>> On 2017/11/29 21:01, Michal Hocko wrote:
>>>>> On Wed 29-11-17 20:41:25, zhong jiang wrote:
>>>>>> On 2017/11/29 20:03, Michal Hocko wrote:
>>>>>>> On Wed 29-11-17 17:13:27, zhong jiang wrote:
>>>>>>>> Currently, Arm64 and x86 use the common code wehn parsing numa node
>>>>>>>> in a acpi way. The arm64 will set the parsed node in numa_add_memblk,
>>>>>>>> but the x86 is not set in that , then it will result in the repeatly
>>>>>>>> setting. And the parsed node maybe is  unreasonable to the system.
>>>>>>>>
>>>>>>>> we would better not set it although it also still works. because the
>>>>>>>> parsed node is unresonable. so we should skip related operate in this
>>>>>>>> node. This patch just set node in various architecture individually.
>>>>>>>> it is no functional change.
>>>>>>> I really have hard time to understand what you try to say above. Could
>>>>>>> you start by the problem description and then how you are addressing it?
>>>>>>   I am so sorry for that.  I will make the issue clear.
>>>>>>
>>>>>>   Arm64  get numa information through acpi.  The code flow is as follows.
>>>>>>
>>>>>>   arm64_acpi_numa_init
>>>>>>        acpi_parse_memory_affinity
>>>>>>           acpi_numa_memory_affinity_init
>>>>>>               numa_add_memblk(nid, start, end);      //it will set node to numa_nodes_parsed successfully.
>>>>>>               node_set(node, numa_nodes_parsed);     // numa_add_memblk had set that.  it will repeat.
>>>>>>
>>>>>>  the root cause is that X86 parse numa also  go through above code.  and  arch-related
>>>>>>  numa_add_memblk  is not set the parsed node to numa_nodes_parsed.  it need
>>>>>>  additional node_set(node, numa_parsed) to handle.  therefore,  the issue will be introduced.
>>>>>>
>>>>> No it is not much more clear. I would have to go and re-study the whole
>>>>> code flow to see what you mean here. So you could simply state what _the
>>>>> issue_ is? How can user observe it and what are the consequences?
>>>>   The patch do not fix a real issue.  it is a cleanup.
>
> > @@ -294,7 +294,9 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
> >          goto out_err_bad_srat;
> >      }
> >
> > -    node_set(node, numa_nodes_parsed);
> > +    /* some architecture is likely to ignore a unreasonable node */
> > +    if (!node_isset(node, numa_nodes_parsed))
> > +        goto out;
> >
>
> It is not just a cleanup patch,    Here you change the original logic.
>
  you are right.  cleanup and slightly change.
> With this patch, we just set the *numa_nodes_parsed* after NUMA adds a
> memblk successfully and also add a check here for bypassing the invalid
> memblk node.
>
> I am not sure which arch may meet this situation? did you test this
> patch?
>
  At least  X86 maybe meet the condition. we can see the following code.

static int __init numa_add_memblk_to(int nid, u64 start, u64 end,
                                     struct numa_meminfo *mi)
{
        /* ignore zero length blks */
        if (start == end)
                return 0;

        /* whine about and ignore invalid blks */
        if (start > end || nid < 0 || nid >= MAX_NUMNODES) {
                pr_warning("NUMA: Warning: invalid memblk node %d [mem %#010Lx-%#010Lx]\n",
                           nid, start, end - 1);
                return 0;
        }

        if (mi->nr_blks >= NR_NODE_MEMBLKS) {
                pr_err("NUMA: too many memblk ranges\n");
                return -EINVAL;
        }

        mi->blk[mi->nr_blks].start = start;
        mi->blk[mi->nr_blks].end = end;
        mi->blk[mi->nr_blks].nid = nid;
        mi->nr_blks++;
        return 0;
}

it is likely to fail and return 0.   e.g: start == end  etc.
In this case, we expect it should bail out in time.
> Anyway, AFAIK, The ACPI tables are very much like user input in that
> respect and they are unreasonable. So the patch is better.
>
  yes,  Totally agree. 

 Thanks
 zhong jiang
> Thanks,
>     dou.
>
>>>>   because the acpi code  is public,  I find they are messy between
>>>>   Arch64 and X86 when parsing numa message .  therefore,  I try to
>>>>   make the code more clear between them.
>>> So make this explicit in the changelog. Your previous wording sounded
>>> like there is a _problem_ in the code.
>>>
>> :-[       please take some time to check.  if it works.  I will resend v2 with detailed changelog.
>>
>> Thanks
>> zhongjiang
>>
>>
>>
>>
>
>
>
> .
>


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/numa: move setting parse numa node to num_add_memblk

[zhong jiang]

+cc more mm maintainer.

Any one has any object.  please let me know.  

Thanks
zhongjiang
On 2017/11/29 17:13, zhong jiang wrote:
> Currently, Arm64 and x86 use the common code wehn parsing numa node
> in a acpi way. The arm64 will set the parsed node in numa_add_memblk,
> but the x86 is not set in that , then it will result in the repeatly
> setting. And the parsed node maybe is  unreasonable to the system.
>
> we would better not set it although it also still works. because the
> parsed node is unresonable. so we should skip related operate in this
> node. This patch just set node in various architecture individually.
> it is no functional change.
>
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  arch/x86/mm/amdtopology.c | 1 -
>  arch/x86/mm/numa.c        | 3 ++-
>  drivers/acpi/numa.c       | 5 ++++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/mm/amdtopology.c b/arch/x86/mm/amdtopology.c
> index 91f501b..7657042 100644
> --- a/arch/x86/mm/amdtopology.c
> +++ b/arch/x86/mm/amdtopology.c
> @@ -151,7 +151,6 @@ int __init amd_numa_init(void)
>  
>  		prevbase = base;
>  		numa_add_memblk(nodeid, base, limit);
> -		node_set(nodeid, numa_nodes_parsed);
>  	}
>  
>  	if (!nodes_weight(numa_nodes_parsed))
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index 25504d5..8f87f26 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -150,6 +150,8 @@ static int __init numa_add_memblk_to(int nid, u64 start, u64 end,
>  	mi->blk[mi->nr_blks].end = end;
>  	mi->blk[mi->nr_blks].nid = nid;
>  	mi->nr_blks++;
> +
> +	node_set(nid, numa_nodes_parsed);
>  	return 0;
>  }
>  
> @@ -693,7 +695,6 @@ static int __init dummy_numa_init(void)
>  	printk(KERN_INFO "Faking a node at [mem %#018Lx-%#018Lx]\n",
>  	       0LLU, PFN_PHYS(max_pfn) - 1);
>  
> -	node_set(0, numa_nodes_parsed);
>  	numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
>  
>  	return 0;
> diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c
> index 917f1cc..f2e33cb 100644
> --- a/drivers/acpi/numa.c
> +++ b/drivers/acpi/numa.c
> @@ -294,7 +294,9 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
>  		goto out_err_bad_srat;
>  	}
>  
> -	node_set(node, numa_nodes_parsed);
> +	/* some architecture is likely to ignore a unreasonable node */
> +	if (!node_isset(node, numa_nodes_parsed))
> +		goto out;
>  
>  	pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s%s\n",
>  		node, pxm,
> @@ -309,6 +311,7 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
>  
>  	max_possible_pfn = max(max_possible_pfn, PFN_UP(end - 1));
>  
> +out:
>  	return 0;
>  out_err_bad_srat:
>  	bad_srat();


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/numa: move setting parse numa node to num_add_memblk

[Michal Hocko]

On Fri 01-12-17 16:48:25, zhong jiang wrote:
> +cc more mm maintainer.
> 
> Any one has any object.  please let me know.  

Please repost with the changelog which actually tells 1) what is the
problem 2) why do we need to address it and 3) how do we address it.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/numa: move setting parse numa node to num_add_memblk

[zhong jiang]

On 2017/12/1 16:58, Michal Hocko wrote:
> On Fri 01-12-17 16:48:25, zhong jiang wrote:
>> +cc more mm maintainer.
>>
>> Any one has any object.  please let me know.  
> Please repost with the changelog which actually tells 1) what is the
> problem 2) why do we need to address it and 3) how do we address it.
Fine,  I will repost later.

Thanks
zhong jiang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>