All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ppc64/numa: consider the max numa node for migratable LPAR
@ 2021-04-29 18:19 Laurent Dufour
  2021-04-29 19:29 ` Tyrel Datwyler
  2021-05-10 10:21   ` Srikar Dronamraju
  0 siblings, 2 replies; 7+ messages in thread
From: Laurent Dufour @ 2021-04-29 18:19 UTC (permalink / raw)
  To: mpe, benh, paulus; +Cc: nathanl, linuxppc-dev, linux-kernel, Srikar Dronamraju

When a LPAR is migratable, we should consider the maximum possible NUMA
node instead the number of NUMA node from the actual system.

The DT property 'ibm,current-associativity-domains' is defining the maximum
number of nodes the LPAR can see when running on that box. But if the LPAR
is being migrated on another box, it may seen up to the nodes defined by
'ibm,max-associativity-domains'. So if a LPAR is migratable, that value
should be used.

Unfortunately, there is no easy way to know if a LPAR is migratable or
not. The hypervisor is exporting the property 'ibm,migratable-partition' in
the case it set to migrate partition, but that would not mean that the
current partition is migratable.

Without that patch, when a LPAR is started on a 2 nodes box and then
migrated to a 3 nodes box, the hypervisor may spread the LPAR's CPUs on the
3rd node. In that case if a CPU from that 3rd node is added to the LPAR, it
will be wrongly assigned to the node because the kernel has been set to use
up to 2 nodes (the configuration of the departure node). With that patch
applies, the CPU is correctly added to the 3rd node.

Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
---
 arch/powerpc/mm/numa.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index f2bf98bdcea2..673fa6e47850 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -893,7 +893,7 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)
 static void __init find_possible_nodes(void)
 {
 	struct device_node *rtas;
-	const __be32 *domains;
+	const __be32 *domains = NULL;
 	int prop_length, max_nodes;
 	u32 i;
 
@@ -909,9 +909,14 @@ static void __init find_possible_nodes(void)
 	 * it doesn't exist, then fallback on ibm,max-associativity-domains.
 	 * Current denotes what the platform can support compared to max
 	 * which denotes what the Hypervisor can support.
+	 *
+	 * If the LPAR is migratable, new nodes might be activated after a LPM,
+	 * so we should consider the max number in that case.
 	 */
-	domains = of_get_property(rtas, "ibm,current-associativity-domains",
-					&prop_length);
+	if (!of_get_property(of_root, "ibm,migratable-partition", NULL))
+		domains = of_get_property(rtas,
+					  "ibm,current-associativity-domains",
+					  &prop_length);
 	if (!domains) {
 		domains = of_get_property(rtas, "ibm,max-associativity-domains",
 					&prop_length);
@@ -920,6 +925,9 @@ static void __init find_possible_nodes(void)
 	}
 
 	max_nodes = of_read_number(&domains[min_common_depth], 1);
+	printk(KERN_INFO "Partition configured for %d NUMA nodes.\n",
+	       max_nodes);
+
 	for (i = 0; i < max_nodes; i++) {
 		if (!node_possible(i))
 			node_set(i, node_possible_map);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] ppc64/numa: consider the max numa node for migratable LPAR
  2021-04-29 18:19 [PATCH] ppc64/numa: consider the max numa node for migratable LPAR Laurent Dufour
@ 2021-04-29 19:29 ` Tyrel Datwyler
  2021-04-30  8:08   ` Laurent Dufour
  2021-05-10 10:21   ` Srikar Dronamraju
  1 sibling, 1 reply; 7+ messages in thread
From: Tyrel Datwyler @ 2021-04-29 19:29 UTC (permalink / raw)
  To: Laurent Dufour, mpe, benh, paulus
  Cc: nathanl, linuxppc-dev, linux-kernel, Srikar Dronamraju

On 4/29/21 11:19 AM, Laurent Dufour wrote:
> When a LPAR is migratable, we should consider the maximum possible NUMA
> node instead the number of NUMA node from the actual system.
> 
> The DT property 'ibm,current-associativity-domains' is defining the maximum
> number of nodes the LPAR can see when running on that box. But if the LPAR
> is being migrated on another box, it may seen up to the nodes defined by
> 'ibm,max-associativity-domains'. So if a LPAR is migratable, that value
> should be used.
> 
> Unfortunately, there is no easy way to know if a LPAR is migratable or
> not. The hypervisor is exporting the property 'ibm,migratable-partition' in
> the case it set to migrate partition, but that would not mean that the
> current partition is migratable.

Wording is a little hard to follow for me here. From PAPR the
'ibm,migratable-partition' property presence indicates that the platform
supports the potential migration of the partition. I guess maybe the point is
that all migratable partitions define 'ibm,migratable-partition', but all
partitions that define 'ibm,migratable-partition' are not necessarily migratable.

-Tyrel

> 
> Without that patch, when a LPAR is started on a 2 nodes box and then
> migrated to a 3 nodes box, the hypervisor may spread the LPAR's CPUs on the
> 3rd node. In that case if a CPU from that 3rd node is added to the LPAR, it
> will be wrongly assigned to the node because the kernel has been set to use
> up to 2 nodes (the configuration of the departure node). With that patch
> applies, the CPU is correctly added to the 3rd node.
> 
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
>  arch/powerpc/mm/numa.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index f2bf98bdcea2..673fa6e47850 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -893,7 +893,7 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)
>  static void __init find_possible_nodes(void)
>  {
>  	struct device_node *rtas;
> -	const __be32 *domains;
> +	const __be32 *domains = NULL;
>  	int prop_length, max_nodes;
>  	u32 i;
> 
> @@ -909,9 +909,14 @@ static void __init find_possible_nodes(void)
>  	 * it doesn't exist, then fallback on ibm,max-associativity-domains.
>  	 * Current denotes what the platform can support compared to max
>  	 * which denotes what the Hypervisor can support.
> +	 *
> +	 * If the LPAR is migratable, new nodes might be activated after a LPM,
> +	 * so we should consider the max number in that case.
>  	 */
> -	domains = of_get_property(rtas, "ibm,current-associativity-domains",
> -					&prop_length);
> +	if (!of_get_property(of_root, "ibm,migratable-partition", NULL))
> +		domains = of_get_property(rtas,
> +					  "ibm,current-associativity-domains",
> +					  &prop_length);
>  	if (!domains) {
>  		domains = of_get_property(rtas, "ibm,max-associativity-domains",
>  					&prop_length);
> @@ -920,6 +925,9 @@ static void __init find_possible_nodes(void)
>  	}
> 
>  	max_nodes = of_read_number(&domains[min_common_depth], 1);
> +	printk(KERN_INFO "Partition configured for %d NUMA nodes.\n",
> +	       max_nodes);
> +
>  	for (i = 0; i < max_nodes; i++) {
>  		if (!node_possible(i))
>  			node_set(i, node_possible_map);
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ppc64/numa: consider the max numa node for migratable LPAR
  2021-04-29 19:29 ` Tyrel Datwyler
@ 2021-04-30  8:08   ` Laurent Dufour
  0 siblings, 0 replies; 7+ messages in thread
From: Laurent Dufour @ 2021-04-30  8:08 UTC (permalink / raw)
  To: Tyrel Datwyler, mpe, benh, paulus
  Cc: nathanl, linuxppc-dev, linux-kernel, Srikar Dronamraju

Le 29/04/2021 à 21:29, Tyrel Datwyler a écrit :
> On 4/29/21 11:19 AM, Laurent Dufour wrote:
>> When a LPAR is migratable, we should consider the maximum possible NUMA
>> node instead the number of NUMA node from the actual system.
>>
>> The DT property 'ibm,current-associativity-domains' is defining the maximum
>> number of nodes the LPAR can see when running on that box. But if the LPAR
>> is being migrated on another box, it may seen up to the nodes defined by
>> 'ibm,max-associativity-domains'. So if a LPAR is migratable, that value
>> should be used.
>>
>> Unfortunately, there is no easy way to know if a LPAR is migratable or
>> not. The hypervisor is exporting the property 'ibm,migratable-partition' in
>> the case it set to migrate partition, but that would not mean that the
>> current partition is migratable.
> 
> Wording is a little hard to follow for me here. From PAPR the
> 'ibm,migratable-partition' property presence indicates that the platform
> supports the potential migration of the partition. I guess maybe the point is
> that all migratable partitions define 'ibm,migratable-partition', but all
> partitions that define 'ibm,migratable-partition' are not necessarily migratable.

That's what I meant.

Laurent

> -Tyrel
> 
>>
>> Without that patch, when a LPAR is started on a 2 nodes box and then
>> migrated to a 3 nodes box, the hypervisor may spread the LPAR's CPUs on the
>> 3rd node. In that case if a CPU from that 3rd node is added to the LPAR, it
>> will be wrongly assigned to the node because the kernel has been set to use
>> up to 2 nodes (the configuration of the departure node). With that patch
>> applies, the CPU is correctly added to the 3rd node.
>>
>> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> ---
>>   arch/powerpc/mm/numa.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>> index f2bf98bdcea2..673fa6e47850 100644
>> --- a/arch/powerpc/mm/numa.c
>> +++ b/arch/powerpc/mm/numa.c
>> @@ -893,7 +893,7 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)
>>   static void __init find_possible_nodes(void)
>>   {
>>   	struct device_node *rtas;
>> -	const __be32 *domains;
>> +	const __be32 *domains = NULL;
>>   	int prop_length, max_nodes;
>>   	u32 i;
>>
>> @@ -909,9 +909,14 @@ static void __init find_possible_nodes(void)
>>   	 * it doesn't exist, then fallback on ibm,max-associativity-domains.
>>   	 * Current denotes what the platform can support compared to max
>>   	 * which denotes what the Hypervisor can support.
>> +	 *
>> +	 * If the LPAR is migratable, new nodes might be activated after a LPM,
>> +	 * so we should consider the max number in that case.
>>   	 */
>> -	domains = of_get_property(rtas, "ibm,current-associativity-domains",
>> -					&prop_length);
>> +	if (!of_get_property(of_root, "ibm,migratable-partition", NULL))
>> +		domains = of_get_property(rtas,
>> +					  "ibm,current-associativity-domains",
>> +					  &prop_length);
>>   	if (!domains) {
>>   		domains = of_get_property(rtas, "ibm,max-associativity-domains",
>>   					&prop_length);
>> @@ -920,6 +925,9 @@ static void __init find_possible_nodes(void)
>>   	}
>>
>>   	max_nodes = of_read_number(&domains[min_common_depth], 1);
>> +	printk(KERN_INFO "Partition configured for %d NUMA nodes.\n",
>> +	       max_nodes);
>> +
>>   	for (i = 0; i < max_nodes; i++) {
>>   		if (!node_possible(i))
>>   			node_set(i, node_possible_map);
>>
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ppc64/numa: consider the max numa node for migratable LPAR
  2021-04-29 18:19 [PATCH] ppc64/numa: consider the max numa node for migratable LPAR Laurent Dufour
@ 2021-05-10 10:21   ` Srikar Dronamraju
  2021-05-10 10:21   ` Srikar Dronamraju
  1 sibling, 0 replies; 7+ messages in thread
From: Srikar Dronamraju @ 2021-05-10 10:21 UTC (permalink / raw)
  To: Laurent Dufour; +Cc: mpe, benh, paulus, nathanl, linuxppc-dev, linux-kernel

* Laurent Dufour <ldufour@linux.ibm.com> [2021-04-29 20:19:01]:

> When a LPAR is migratable, we should consider the maximum possible NUMA
> node instead the number of NUMA node from the actual system.
> 
> The DT property 'ibm,current-associativity-domains' is defining the maximum
> number of nodes the LPAR can see when running on that box. But if the LPAR
> is being migrated on another box, it may seen up to the nodes defined by
> 'ibm,max-associativity-domains'. So if a LPAR is migratable, that value
> should be used.
> 
> Unfortunately, there is no easy way to know if a LPAR is migratable or
> not. The hypervisor is exporting the property 'ibm,migratable-partition' in
> the case it set to migrate partition, but that would not mean that the
> current partition is migratable.
> 
> Without that patch, when a LPAR is started on a 2 nodes box and then
> migrated to a 3 nodes box, the hypervisor may spread the LPAR's CPUs on the
> 3rd node. In that case if a CPU from that 3rd node is added to the LPAR, it
> will be wrongly assigned to the node because the kernel has been set to use


> up to 2 nodes (the configuration of the departure node). With that patch
> applies, the CPU is correctly added to the 3rd node.

You probably meant, "With this patch applied"

Also you may want to add a fixes tag:

> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
>  arch/powerpc/mm/numa.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index f2bf98bdcea2..673fa6e47850 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -893,7 +893,7 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)
>  static void __init find_possible_nodes(void)
>  {
>  	struct device_node *rtas;
> -	const __be32 *domains;
> +	const __be32 *domains = NULL;
>  	int prop_length, max_nodes;
>  	u32 i;
> 
> @@ -909,9 +909,14 @@ static void __init find_possible_nodes(void)
>  	 * it doesn't exist, then fallback on ibm,max-associativity-domains.
>  	 * Current denotes what the platform can support compared to max
>  	 * which denotes what the Hypervisor can support.
> +	 *
> +	 * If the LPAR is migratable, new nodes might be activated after a LPM,
> +	 * so we should consider the max number in that case.
>  	 */
> -	domains = of_get_property(rtas, "ibm,current-associativity-domains",
> -					&prop_length);
> +	if (!of_get_property(of_root, "ibm,migratable-partition", NULL))
> +		domains = of_get_property(rtas,
> +					  "ibm,current-associativity-domains",
> +					  &prop_length);
>  	if (!domains) {
>  		domains = of_get_property(rtas, "ibm,max-associativity-domains",
>  					&prop_length);
> @@ -920,6 +925,9 @@ static void __init find_possible_nodes(void)
>  	}
> 
>  	max_nodes = of_read_number(&domains[min_common_depth], 1);
> +	printk(KERN_INFO "Partition configured for %d NUMA nodes.\n",
> +	       max_nodes);
> +

Another nit:
you may want to make this pr_info instead of printk

>  	for (i = 0; i < max_nodes; i++) {
>  		if (!node_possible(i))
>  			node_set(i, node_possible_map);
> -- 
> 2.31.1
> 

Otherwise looks good to me.

Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

-- 
Thanks and Regards
Srikar Dronamraju

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ppc64/numa: consider the max numa node for migratable LPAR
@ 2021-05-10 10:21   ` Srikar Dronamraju
  0 siblings, 0 replies; 7+ messages in thread
From: Srikar Dronamraju @ 2021-05-10 10:21 UTC (permalink / raw)
  To: Laurent Dufour; +Cc: nathanl, linux-kernel, paulus, linuxppc-dev

* Laurent Dufour <ldufour@linux.ibm.com> [2021-04-29 20:19:01]:

> When a LPAR is migratable, we should consider the maximum possible NUMA
> node instead the number of NUMA node from the actual system.
> 
> The DT property 'ibm,current-associativity-domains' is defining the maximum
> number of nodes the LPAR can see when running on that box. But if the LPAR
> is being migrated on another box, it may seen up to the nodes defined by
> 'ibm,max-associativity-domains'. So if a LPAR is migratable, that value
> should be used.
> 
> Unfortunately, there is no easy way to know if a LPAR is migratable or
> not. The hypervisor is exporting the property 'ibm,migratable-partition' in
> the case it set to migrate partition, but that would not mean that the
> current partition is migratable.
> 
> Without that patch, when a LPAR is started on a 2 nodes box and then
> migrated to a 3 nodes box, the hypervisor may spread the LPAR's CPUs on the
> 3rd node. In that case if a CPU from that 3rd node is added to the LPAR, it
> will be wrongly assigned to the node because the kernel has been set to use


> up to 2 nodes (the configuration of the departure node). With that patch
> applies, the CPU is correctly added to the 3rd node.

You probably meant, "With this patch applied"

Also you may want to add a fixes tag:

> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
>  arch/powerpc/mm/numa.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index f2bf98bdcea2..673fa6e47850 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -893,7 +893,7 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)
>  static void __init find_possible_nodes(void)
>  {
>  	struct device_node *rtas;
> -	const __be32 *domains;
> +	const __be32 *domains = NULL;
>  	int prop_length, max_nodes;
>  	u32 i;
> 
> @@ -909,9 +909,14 @@ static void __init find_possible_nodes(void)
>  	 * it doesn't exist, then fallback on ibm,max-associativity-domains.
>  	 * Current denotes what the platform can support compared to max
>  	 * which denotes what the Hypervisor can support.
> +	 *
> +	 * If the LPAR is migratable, new nodes might be activated after a LPM,
> +	 * so we should consider the max number in that case.
>  	 */
> -	domains = of_get_property(rtas, "ibm,current-associativity-domains",
> -					&prop_length);
> +	if (!of_get_property(of_root, "ibm,migratable-partition", NULL))
> +		domains = of_get_property(rtas,
> +					  "ibm,current-associativity-domains",
> +					  &prop_length);
>  	if (!domains) {
>  		domains = of_get_property(rtas, "ibm,max-associativity-domains",
>  					&prop_length);
> @@ -920,6 +925,9 @@ static void __init find_possible_nodes(void)
>  	}
> 
>  	max_nodes = of_read_number(&domains[min_common_depth], 1);
> +	printk(KERN_INFO "Partition configured for %d NUMA nodes.\n",
> +	       max_nodes);
> +

Another nit:
you may want to make this pr_info instead of printk

>  	for (i = 0; i < max_nodes; i++) {
>  		if (!node_possible(i))
>  			node_set(i, node_possible_map);
> -- 
> 2.31.1
> 

Otherwise looks good to me.

Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

-- 
Thanks and Regards
Srikar Dronamraju

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ppc64/numa: consider the max numa node for migratable LPAR
  2021-05-10 10:21   ` Srikar Dronamraju
@ 2021-05-10 13:01     ` Laurent Dufour
  -1 siblings, 0 replies; 7+ messages in thread
From: Laurent Dufour @ 2021-05-10 13:01 UTC (permalink / raw)
  To: Srikar Dronamraju; +Cc: mpe, benh, paulus, nathanl, linuxppc-dev, linux-kernel

Le 10/05/2021 à 12:21, Srikar Dronamraju a écrit :
> * Laurent Dufour <ldufour@linux.ibm.com> [2021-04-29 20:19:01]:
> 
>> When a LPAR is migratable, we should consider the maximum possible NUMA
>> node instead the number of NUMA node from the actual system.
>>
>> The DT property 'ibm,current-associativity-domains' is defining the maximum
>> number of nodes the LPAR can see when running on that box. But if the LPAR
>> is being migrated on another box, it may seen up to the nodes defined by
>> 'ibm,max-associativity-domains'. So if a LPAR is migratable, that value
>> should be used.
>>
>> Unfortunately, there is no easy way to know if a LPAR is migratable or
>> not. The hypervisor is exporting the property 'ibm,migratable-partition' in
>> the case it set to migrate partition, but that would not mean that the
>> current partition is migratable.
>>
>> Without that patch, when a LPAR is started on a 2 nodes box and then
>> migrated to a 3 nodes box, the hypervisor may spread the LPAR's CPUs on the
>> 3rd node. In that case if a CPU from that 3rd node is added to the LPAR, it
>> will be wrongly assigned to the node because the kernel has been set to use
> 
> 
>> up to 2 nodes (the configuration of the departure node). With that patch
>> applies, the CPU is correctly added to the 3rd node.
> 
> You probably meant, "With this patch applied"
> 
> Also you may want to add a fixes tag:

I'll fix "that" and add the fixes tag.

>> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> ---
>>   arch/powerpc/mm/numa.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>> index f2bf98bdcea2..673fa6e47850 100644
>> --- a/arch/powerpc/mm/numa.c
>> +++ b/arch/powerpc/mm/numa.c
>> @@ -893,7 +893,7 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)
>>   static void __init find_possible_nodes(void)
>>   {
>>   	struct device_node *rtas;
>> -	const __be32 *domains;
>> +	const __be32 *domains = NULL;
>>   	int prop_length, max_nodes;
>>   	u32 i;
>>
>> @@ -909,9 +909,14 @@ static void __init find_possible_nodes(void)
>>   	 * it doesn't exist, then fallback on ibm,max-associativity-domains.
>>   	 * Current denotes what the platform can support compared to max
>>   	 * which denotes what the Hypervisor can support.
>> +	 *
>> +	 * If the LPAR is migratable, new nodes might be activated after a LPM,
>> +	 * so we should consider the max number in that case.
>>   	 */
>> -	domains = of_get_property(rtas, "ibm,current-associativity-domains",
>> -					&prop_length);
>> +	if (!of_get_property(of_root, "ibm,migratable-partition", NULL))
>> +		domains = of_get_property(rtas,
>> +					  "ibm,current-associativity-domains",
>> +					  &prop_length);
>>   	if (!domains) {
>>   		domains = of_get_property(rtas, "ibm,max-associativity-domains",
>>   					&prop_length);
>> @@ -920,6 +925,9 @@ static void __init find_possible_nodes(void)
>>   	}
>>
>>   	max_nodes = of_read_number(&domains[min_common_depth], 1);
>> +	printk(KERN_INFO "Partition configured for %d NUMA nodes.\n",
>> +	       max_nodes);
>> +
> 
> Another nit:
> you may want to make this pr_info instead of printk

Sure !

>>   	for (i = 0; i < max_nodes; i++) {
>>   		if (!node_possible(i))
>>   			node_set(i, node_possible_map);
>> -- 
>> 2.31.1
>>
> 
> Otherwise looks good to me.
> 
> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thanks Srikar, I'll add you review tag in the v2.



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ppc64/numa: consider the max numa node for migratable LPAR
@ 2021-05-10 13:01     ` Laurent Dufour
  0 siblings, 0 replies; 7+ messages in thread
From: Laurent Dufour @ 2021-05-10 13:01 UTC (permalink / raw)
  To: Srikar Dronamraju; +Cc: nathanl, linux-kernel, paulus, linuxppc-dev

Le 10/05/2021 à 12:21, Srikar Dronamraju a écrit :
> * Laurent Dufour <ldufour@linux.ibm.com> [2021-04-29 20:19:01]:
> 
>> When a LPAR is migratable, we should consider the maximum possible NUMA
>> node instead the number of NUMA node from the actual system.
>>
>> The DT property 'ibm,current-associativity-domains' is defining the maximum
>> number of nodes the LPAR can see when running on that box. But if the LPAR
>> is being migrated on another box, it may seen up to the nodes defined by
>> 'ibm,max-associativity-domains'. So if a LPAR is migratable, that value
>> should be used.
>>
>> Unfortunately, there is no easy way to know if a LPAR is migratable or
>> not. The hypervisor is exporting the property 'ibm,migratable-partition' in
>> the case it set to migrate partition, but that would not mean that the
>> current partition is migratable.
>>
>> Without that patch, when a LPAR is started on a 2 nodes box and then
>> migrated to a 3 nodes box, the hypervisor may spread the LPAR's CPUs on the
>> 3rd node. In that case if a CPU from that 3rd node is added to the LPAR, it
>> will be wrongly assigned to the node because the kernel has been set to use
> 
> 
>> up to 2 nodes (the configuration of the departure node). With that patch
>> applies, the CPU is correctly added to the 3rd node.
> 
> You probably meant, "With this patch applied"
> 
> Also you may want to add a fixes tag:

I'll fix "that" and add the fixes tag.

>> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> ---
>>   arch/powerpc/mm/numa.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>> index f2bf98bdcea2..673fa6e47850 100644
>> --- a/arch/powerpc/mm/numa.c
>> +++ b/arch/powerpc/mm/numa.c
>> @@ -893,7 +893,7 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)
>>   static void __init find_possible_nodes(void)
>>   {
>>   	struct device_node *rtas;
>> -	const __be32 *domains;
>> +	const __be32 *domains = NULL;
>>   	int prop_length, max_nodes;
>>   	u32 i;
>>
>> @@ -909,9 +909,14 @@ static void __init find_possible_nodes(void)
>>   	 * it doesn't exist, then fallback on ibm,max-associativity-domains.
>>   	 * Current denotes what the platform can support compared to max
>>   	 * which denotes what the Hypervisor can support.
>> +	 *
>> +	 * If the LPAR is migratable, new nodes might be activated after a LPM,
>> +	 * so we should consider the max number in that case.
>>   	 */
>> -	domains = of_get_property(rtas, "ibm,current-associativity-domains",
>> -					&prop_length);
>> +	if (!of_get_property(of_root, "ibm,migratable-partition", NULL))
>> +		domains = of_get_property(rtas,
>> +					  "ibm,current-associativity-domains",
>> +					  &prop_length);
>>   	if (!domains) {
>>   		domains = of_get_property(rtas, "ibm,max-associativity-domains",
>>   					&prop_length);
>> @@ -920,6 +925,9 @@ static void __init find_possible_nodes(void)
>>   	}
>>
>>   	max_nodes = of_read_number(&domains[min_common_depth], 1);
>> +	printk(KERN_INFO "Partition configured for %d NUMA nodes.\n",
>> +	       max_nodes);
>> +
> 
> Another nit:
> you may want to make this pr_info instead of printk

Sure !

>>   	for (i = 0; i < max_nodes; i++) {
>>   		if (!node_possible(i))
>>   			node_set(i, node_possible_map);
>> -- 
>> 2.31.1
>>
> 
> Otherwise looks good to me.
> 
> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thanks Srikar, I'll add you review tag in the v2.



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-05-10 13:06 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-29 18:19 [PATCH] ppc64/numa: consider the max numa node for migratable LPAR Laurent Dufour
2021-04-29 19:29 ` Tyrel Datwyler
2021-04-30  8:08   ` Laurent Dufour
2021-05-10 10:21 ` Srikar Dronamraju
2021-05-10 10:21   ` Srikar Dronamraju
2021-05-10 13:01   ` Laurent Dufour
2021-05-10 13:01     ` Laurent Dufour

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.