All of lore.kernel.org
 help / color / mirror / Atom feed
From: thunder.leizhen@huawei.com (Leizhen (ThunderTown))
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v7 12/14] arm64/numa: remove the limitation that cpu0 must bind to node0
Date: Mon, 29 Aug 2016 14:55:42 +0800	[thread overview]
Message-ID: <57C3DC6E.4040406@huawei.com> (raw)
In-Reply-To: <20160826154942.GK30302@arm.com>



On 2016/8/26 23:49, Will Deacon wrote:
> On Wed, Aug 24, 2016 at 03:44:51PM +0800, Zhen Lei wrote:
>> 1. Currently only cpu0 set on cpu_possible_mask and percpu areas have not
>>    been initialized.
This description refer to below:
-	for_each_possible_cpu(cpu)
-		set_cpu_numa_node(cpu, NUMA_NO_NODE);

1. When the above code is executed, only the bit of cpu0 was set on cpu_possible_mask.
   So that, only set_cpu_numa_node(0, NUMA_NO_NODE); will be executed.
2. set_cpu_numa_node will access percpu variable numa_node, but setup_per_cpu_areas is
   called after current time. Without the first problem, it will lead kernel crash.

I changed the title of this patch in v7, the original is "remove some useless code".
I think I should separate this into a new patch.



>> 2. No reason to limit cpu0 must belongs to node0.
> 
> Whilst I suspect you're using enumerated lists in order to try to make
> things clearer, I'm having a really hard time understanding the commit
> messages you have in this series. It's actually much better if you
> structure them as concise paragraphs explaining:
> 
>   - What is the problem that you're fixing?
> 
>   - How does that problem manifest?
> 
>   - How does the patch fix it?
> 
> As far as I can see, this patch just removes a bunch of code with no
> explanation as to why it's not required or any problems caused by
> keeping it around.
> 
> Will
> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>  arch/arm64/mm/numa.c | 12 ++----------
>>  1 file changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
>> index 114180f..07a1978 100644
>> --- a/arch/arm64/mm/numa.c
>> +++ b/arch/arm64/mm/numa.c
>> @@ -94,7 +94,6 @@ void numa_clear_node(unsigned int cpu)
>>   */
>>  static void __init setup_node_to_cpumask_map(void)
>>  {
>> -	unsigned int cpu;
>>  	int node;
>>
>>  	/* setup nr_node_ids if not done yet */
>> @@ -107,9 +106,6 @@ static void __init setup_node_to_cpumask_map(void)
>>  		cpumask_clear(node_to_cpumask_map[node]);
>>  	}
>>
>> -	for_each_possible_cpu(cpu)
>> -		set_cpu_numa_node(cpu, NUMA_NO_NODE);
>> -
>>  	/* cpumask_of_node() will now work */
>>  	pr_debug("Node to cpumask map for %d nodes\n", nr_node_ids);
>>  }
>> @@ -119,13 +115,13 @@ static void __init setup_node_to_cpumask_map(void)
>>   */
>>  void numa_store_cpu_info(unsigned int cpu)
>>  {
>> -	map_cpu_to_node(cpu, numa_off ? 0 : cpu_to_node_map[cpu]);
>> +	map_cpu_to_node(cpu, cpu_to_node_map[cpu]);
>>  }
>>
>>  void __init early_map_cpu_to_node(unsigned int cpu, int nid)
>>  {
>>  	/* fallback to node 0 */
>> -	if (nid < 0 || nid >= MAX_NUMNODES)
>> +	if (nid < 0 || nid >= MAX_NUMNODES || numa_off)
>>  		nid = 0;
After the below code have been removed, we should make the corresponding adjustment.
otherwise, kernel will be crashed if "numa=off" was set in bootargs.

>>
>>  	cpu_to_node_map[cpu] = nid;
>> @@ -375,10 +371,6 @@ static int __init numa_init(int (*init_func)(void))
>>
>>  	setup_node_to_cpumask_map();
>>
>> -	/* init boot processor */
>> -	cpu_to_node_map[0] = 0;
>> -	map_cpu_to_node(0, 0);
These code limit cpu0 must belong to node0, but our current implementation deesn't
have this limitation.

>> -
>>  	return 0;
>>  }
>>
>> --
>> 2.5.0
>>
>>
> 
> .
> 

WARNING: multiple messages have this Message-ID (diff)
From: "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com>
To: Will Deacon <will.deacon@arm.com>
Cc: devicetree <devicetree@vger.kernel.org>,
	Zefan Li <lizefan@huawei.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Hanjun Guo <guohanjun@huawei.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Xinwei Hu <huxinwei@huawei.com>, Rob Herring <robh+dt@kernel.org>,
	Tianhong Ding <dingtianhong@huawei.com>,
	Frank Rowand <frowand.list@gmail.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v7 12/14] arm64/numa: remove the limitation that cpu0 must bind to node0
Date: Mon, 29 Aug 2016 14:55:42 +0800	[thread overview]
Message-ID: <57C3DC6E.4040406@huawei.com> (raw)
In-Reply-To: <20160826154942.GK30302@arm.com>



On 2016/8/26 23:49, Will Deacon wrote:
> On Wed, Aug 24, 2016 at 03:44:51PM +0800, Zhen Lei wrote:
>> 1. Currently only cpu0 set on cpu_possible_mask and percpu areas have not
>>    been initialized.
This description refer to below:
-	for_each_possible_cpu(cpu)
-		set_cpu_numa_node(cpu, NUMA_NO_NODE);

1. When the above code is executed, only the bit of cpu0 was set on cpu_possible_mask.
   So that, only set_cpu_numa_node(0, NUMA_NO_NODE); will be executed.
2. set_cpu_numa_node will access percpu variable numa_node, but setup_per_cpu_areas is
   called after current time. Without the first problem, it will lead kernel crash.

I changed the title of this patch in v7, the original is "remove some useless code".
I think I should separate this into a new patch.



>> 2. No reason to limit cpu0 must belongs to node0.
> 
> Whilst I suspect you're using enumerated lists in order to try to make
> things clearer, I'm having a really hard time understanding the commit
> messages you have in this series. It's actually much better if you
> structure them as concise paragraphs explaining:
> 
>   - What is the problem that you're fixing?
> 
>   - How does that problem manifest?
> 
>   - How does the patch fix it?
> 
> As far as I can see, this patch just removes a bunch of code with no
> explanation as to why it's not required or any problems caused by
> keeping it around.
> 
> Will
> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>  arch/arm64/mm/numa.c | 12 ++----------
>>  1 file changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
>> index 114180f..07a1978 100644
>> --- a/arch/arm64/mm/numa.c
>> +++ b/arch/arm64/mm/numa.c
>> @@ -94,7 +94,6 @@ void numa_clear_node(unsigned int cpu)
>>   */
>>  static void __init setup_node_to_cpumask_map(void)
>>  {
>> -	unsigned int cpu;
>>  	int node;
>>
>>  	/* setup nr_node_ids if not done yet */
>> @@ -107,9 +106,6 @@ static void __init setup_node_to_cpumask_map(void)
>>  		cpumask_clear(node_to_cpumask_map[node]);
>>  	}
>>
>> -	for_each_possible_cpu(cpu)
>> -		set_cpu_numa_node(cpu, NUMA_NO_NODE);
>> -
>>  	/* cpumask_of_node() will now work */
>>  	pr_debug("Node to cpumask map for %d nodes\n", nr_node_ids);
>>  }
>> @@ -119,13 +115,13 @@ static void __init setup_node_to_cpumask_map(void)
>>   */
>>  void numa_store_cpu_info(unsigned int cpu)
>>  {
>> -	map_cpu_to_node(cpu, numa_off ? 0 : cpu_to_node_map[cpu]);
>> +	map_cpu_to_node(cpu, cpu_to_node_map[cpu]);
>>  }
>>
>>  void __init early_map_cpu_to_node(unsigned int cpu, int nid)
>>  {
>>  	/* fallback to node 0 */
>> -	if (nid < 0 || nid >= MAX_NUMNODES)
>> +	if (nid < 0 || nid >= MAX_NUMNODES || numa_off)
>>  		nid = 0;
After the below code have been removed, we should make the corresponding adjustment.
otherwise, kernel will be crashed if "numa=off" was set in bootargs.

>>
>>  	cpu_to_node_map[cpu] = nid;
>> @@ -375,10 +371,6 @@ static int __init numa_init(int (*init_func)(void))
>>
>>  	setup_node_to_cpumask_map();
>>
>> -	/* init boot processor */
>> -	cpu_to_node_map[0] = 0;
>> -	map_cpu_to_node(0, 0);
These code limit cpu0 must belong to node0, but our current implementation deesn't
have this limitation.

>> -
>>  	return 0;
>>  }
>>
>> --
>> 2.5.0
>>
>>
> 
> .
> 

WARNING: multiple messages have this Message-ID (diff)
From: "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com>
To: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	"Frank Rowand" <frowand.list@gmail.com>,
	devicetree <devicetree@vger.kernel.org>,
	Zefan Li <lizefan@huawei.com>, Xinwei Hu <huxinwei@huawei.com>,
	Tianhong Ding <dingtianhong@huawei.com>,
	Hanjun Guo <guohanjun@huawei.com>
Subject: Re: [PATCH v7 12/14] arm64/numa: remove the limitation that cpu0 must bind to node0
Date: Mon, 29 Aug 2016 14:55:42 +0800	[thread overview]
Message-ID: <57C3DC6E.4040406@huawei.com> (raw)
In-Reply-To: <20160826154942.GK30302@arm.com>



On 2016/8/26 23:49, Will Deacon wrote:
> On Wed, Aug 24, 2016 at 03:44:51PM +0800, Zhen Lei wrote:
>> 1. Currently only cpu0 set on cpu_possible_mask and percpu areas have not
>>    been initialized.
This description refer to below:
-	for_each_possible_cpu(cpu)
-		set_cpu_numa_node(cpu, NUMA_NO_NODE);

1. When the above code is executed, only the bit of cpu0 was set on cpu_possible_mask.
   So that, only set_cpu_numa_node(0, NUMA_NO_NODE); will be executed.
2. set_cpu_numa_node will access percpu variable numa_node, but setup_per_cpu_areas is
   called after current time. Without the first problem, it will lead kernel crash.

I changed the title of this patch in v7, the original is "remove some useless code".
I think I should separate this into a new patch.



>> 2. No reason to limit cpu0 must belongs to node0.
> 
> Whilst I suspect you're using enumerated lists in order to try to make
> things clearer, I'm having a really hard time understanding the commit
> messages you have in this series. It's actually much better if you
> structure them as concise paragraphs explaining:
> 
>   - What is the problem that you're fixing?
> 
>   - How does that problem manifest?
> 
>   - How does the patch fix it?
> 
> As far as I can see, this patch just removes a bunch of code with no
> explanation as to why it's not required or any problems caused by
> keeping it around.
> 
> Will
> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>  arch/arm64/mm/numa.c | 12 ++----------
>>  1 file changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
>> index 114180f..07a1978 100644
>> --- a/arch/arm64/mm/numa.c
>> +++ b/arch/arm64/mm/numa.c
>> @@ -94,7 +94,6 @@ void numa_clear_node(unsigned int cpu)
>>   */
>>  static void __init setup_node_to_cpumask_map(void)
>>  {
>> -	unsigned int cpu;
>>  	int node;
>>
>>  	/* setup nr_node_ids if not done yet */
>> @@ -107,9 +106,6 @@ static void __init setup_node_to_cpumask_map(void)
>>  		cpumask_clear(node_to_cpumask_map[node]);
>>  	}
>>
>> -	for_each_possible_cpu(cpu)
>> -		set_cpu_numa_node(cpu, NUMA_NO_NODE);
>> -
>>  	/* cpumask_of_node() will now work */
>>  	pr_debug("Node to cpumask map for %d nodes\n", nr_node_ids);
>>  }
>> @@ -119,13 +115,13 @@ static void __init setup_node_to_cpumask_map(void)
>>   */
>>  void numa_store_cpu_info(unsigned int cpu)
>>  {
>> -	map_cpu_to_node(cpu, numa_off ? 0 : cpu_to_node_map[cpu]);
>> +	map_cpu_to_node(cpu, cpu_to_node_map[cpu]);
>>  }
>>
>>  void __init early_map_cpu_to_node(unsigned int cpu, int nid)
>>  {
>>  	/* fallback to node 0 */
>> -	if (nid < 0 || nid >= MAX_NUMNODES)
>> +	if (nid < 0 || nid >= MAX_NUMNODES || numa_off)
>>  		nid = 0;
After the below code have been removed, we should make the corresponding adjustment.
otherwise, kernel will be crashed if "numa=off" was set in bootargs.

>>
>>  	cpu_to_node_map[cpu] = nid;
>> @@ -375,10 +371,6 @@ static int __init numa_init(int (*init_func)(void))
>>
>>  	setup_node_to_cpumask_map();
>>
>> -	/* init boot processor */
>> -	cpu_to_node_map[0] = 0;
>> -	map_cpu_to_node(0, 0);
These code limit cpu0 must belong to node0, but our current implementation deesn't
have this limitation.

>> -
>>  	return 0;
>>  }
>>
>> --
>> 2.5.0
>>
>>
> 
> .
> 

  reply	other threads:[~2016-08-29  6:55 UTC|newest]

Thread overview: 92+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-24  7:44 [PATCH v7 00/14] fix some type infos and bugs for arm64/of numa Zhen Lei
2016-08-24  7:44 ` Zhen Lei
2016-08-24  7:44 ` Zhen Lei
2016-08-24  7:44 ` [PATCH v7 01/14] of/numa: remove a duplicated pr_debug information Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-24  7:44 ` [PATCH v7 02/14] of/numa: fix a memory@ node can only contains one memory block Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-24  7:44 ` [PATCH v7 03/14] arm64/numa: add nid check for " Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-26 12:39   ` Will Deacon
2016-08-26 12:39     ` Will Deacon
2016-08-26 12:39     ` Will Deacon
2016-08-27  8:02     ` Leizhen (ThunderTown)
2016-08-27  8:02       ` Leizhen (ThunderTown)
2016-08-24  7:44 ` [PATCH v7 04/14] of/numa: remove a duplicated warning Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-24  7:44 ` [PATCH v7 05/14] arm64/numa: avoid inconsistent information to be printed Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-26 12:47   ` Will Deacon
2016-08-26 12:47     ` Will Deacon
2016-08-26 12:47     ` Will Deacon
2016-08-27  8:54     ` Leizhen (ThunderTown)
2016-08-27  8:54       ` Leizhen (ThunderTown)
2016-08-27  8:54       ` Leizhen (ThunderTown)
2016-08-30 17:51       ` Will Deacon
2016-08-30 17:51         ` Will Deacon
2016-08-30 17:51         ` Will Deacon
2016-08-31  2:29         ` Leizhen (ThunderTown)
2016-08-31  2:29           ` Leizhen (ThunderTown)
2016-08-31  2:29           ` Leizhen (ThunderTown)
2016-08-24  7:44 ` [PATCH v7 06/14] of_numa: Use of_get_next_parent to simplify code Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-24  7:44 ` [PATCH v7 07/14] of_numa: Use pr_fmt() Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-24  7:44 ` [PATCH v7 08/14] arm64: numa: " Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-26 12:54   ` Will Deacon
2016-08-26 12:54     ` Will Deacon
2016-08-26 12:54     ` Will Deacon
2016-08-27  9:14     ` Leizhen (ThunderTown)
2016-08-27  9:14       ` Leizhen (ThunderTown)
2016-08-27  9:14       ` Leizhen (ThunderTown)
2016-08-24  7:44 ` [PATCH v7 09/14] arm64/numa: support HAVE_SETUP_PER_CPU_AREA Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-26 13:28   ` Will Deacon
2016-08-26 13:28     ` Will Deacon
2016-08-26 13:28     ` Will Deacon
2016-08-27 10:06     ` Leizhen (ThunderTown)
2016-08-27 10:06       ` Leizhen (ThunderTown)
2016-08-27 10:06       ` Leizhen (ThunderTown)
2016-08-24  7:44 ` [PATCH v7 10/14] arm64/numa: define numa_distance as array to simplify code Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-26 15:29   ` Will Deacon
2016-08-26 15:29     ` Will Deacon
2016-08-27 10:29     ` Leizhen (ThunderTown)
2016-08-27 10:29       ` Leizhen (ThunderTown)
2016-08-27 10:29       ` Leizhen (ThunderTown)
2016-08-24  7:44 ` [PATCH v7 11/14] arm64/numa: support HAVE_MEMORYLESS_NODES Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-26 15:43   ` Will Deacon
2016-08-26 15:43     ` Will Deacon
2016-08-27 11:05     ` Leizhen (ThunderTown)
2016-08-27 11:05       ` Leizhen (ThunderTown)
2016-08-29  3:15       ` Leizhen (ThunderTown)
2016-08-29  3:15         ` Leizhen (ThunderTown)
2016-08-29  3:15         ` Leizhen (ThunderTown)
2016-08-24  7:44 ` [PATCH v7 12/14] arm64/numa: remove the limitation that cpu0 must bind to node0 Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-26 15:49   ` Will Deacon
2016-08-26 15:49     ` Will Deacon
2016-08-26 15:49     ` Will Deacon
2016-08-29  6:55     ` Leizhen (ThunderTown) [this message]
2016-08-29  6:55       ` Leizhen (ThunderTown)
2016-08-29  6:55       ` Leizhen (ThunderTown)
2016-08-24  7:44 ` [PATCH v7 13/14] of/numa: remove the constraint on the distances of node pairs Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-24  7:44 ` [PATCH v7 14/14] Documentation: " Zhen Lei
2016-08-24  7:44   ` Zhen Lei
2016-08-26 15:35   ` Will Deacon
2016-08-26 15:35     ` Will Deacon
2016-08-27 10:44     ` Leizhen (ThunderTown)
2016-08-27 10:44       ` Leizhen (ThunderTown)
2016-08-27 10:44       ` Leizhen (ThunderTown)
2016-08-30 17:55       ` Will Deacon
2016-08-30 17:55         ` Will Deacon
2016-08-30 17:55         ` Will Deacon
2016-08-31  2:46         ` Leizhen (ThunderTown)
2016-08-31  2:46           ` Leizhen (ThunderTown)
2016-08-31  2:46           ` Leizhen (ThunderTown)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=57C3DC6E.4040406@huawei.com \
    --to=thunder.leizhen@huawei.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.