Re: [RFC v4 2/3] powerpc migration/cpu: Associativity & cpu changes

2018-05-18 Thread Michael Bringmann 


On 05/18/2018 02:28 PM, Nathan Fontenot wrote:
> On 05/17/2018 05:26 PM, Michael Bringmann wrote:
>> powerpc migration/cpu: Now apply changes to the associativity of cpus
>> for the topology of LPARS in Post Migration events.  Recognize more
>> changes to the associativity of memory blocks described by the
>> 'cpu' properties when processing the topology of LPARS in Post Migration
>> events.  Previous efforts only recognized whether a memory block's
>> assignment had changed in the property.  Changes here include:
>>
>> * Provide hotplug CPU 'readd by index' operation
>> * Checking for changes in cpu associativity and making 'readd' calls
>>   when differences are observed.
>> * Queue up  changes to CPU properties so that they may take place
>>   after all PowerPC device-tree changes have been applied i.e. after
>>   the device hotplug is released in the mobility code.
> 
> This kinda feels like three different patches in one. any reason to not split
> this into three patches? Perhaps at the least split the last item into it's
> own patch.

E.g.
1) Conditional 'acquire_drc'
2) PSERIES_HP_ELOG_ACTION_READD call to dlpar_cpu_readd_by_index
3) Add lock/unlock device hotplug
> 
>>
>> Signed-off-by: Michael Bringmann 
>> ---
>> Changes include:
>>   -- Rearrange patches to co-locate CPU property-related changes.
>>   -- Modify dlpar_cpu_add & dlpar_cpu_remove to skip DRC index acquire
>>  or release operations during the CPU readd process.
>>   -- Correct a bug in DRC index selection for queued operation.
>>   -- Rebase to 4.17-rc5 kernel
>> ---
>>  arch/powerpc/platforms/pseries/hotplug-cpu.c |  123 
>> +++---
>>  arch/powerpc/platforms/pseries/mobility.c|3 +
>>  2 files changed, 95 insertions(+), 31 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c 
>> b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index a408217..23d4cb8 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -474,7 +474,7 @@ static bool valid_cpu_drc_index(struct device_node 
>> *parent, u32 drc_index)
>>  );
>>  }
>>
>> -static ssize_t dlpar_cpu_add(u32 drc_index)
>> +static ssize_t dlpar_cpu_add(u32 drc_index, bool acquire_drc)
>>  {
>>  struct device_node *dn, *parent;
>>  int rc, saved_rc;
>> @@ -499,19 +499,22 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
>>  return -EINVAL;
>>  }
>>
>> -rc = dlpar_acquire_drc(drc_index);
>> -if (rc) {
>> -pr_warn("Failed to acquire DRC, rc: %d, drc index: %x\n",
>> -rc, drc_index);
>> -of_node_put(parent);
>> -return -EINVAL;
>> +if (acquire_drc) {
>> +rc = dlpar_acquire_drc(drc_index);
>> +if (rc) {
>> +pr_warn("Failed to acquire DRC, rc: %d, drc index: 
>> %x\n",
>> +rc, drc_index);
>> +of_node_put(parent);
>> +return -EINVAL;
>> +}
>>  }
>>
>>  dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
>>  if (!dn) {
>>  pr_warn("Failed call to configure-connector, drc index: %x\n",
>>  drc_index);
>> -dlpar_release_drc(drc_index);
>> +if (acquire_drc)
>> +dlpar_release_drc(drc_index);
>>  of_node_put(parent);
>>  return -EINVAL;
>>  }
>> @@ -526,8 +529,9 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
>>  pr_warn("Failed to attach node %s, rc: %d, drc index: %x\n",
>>  dn->name, rc, drc_index);
>>
>> -rc = dlpar_release_drc(drc_index);
>> -if (!rc)
>> +if (acquire_drc)
>> +rc = dlpar_release_drc(drc_index);
>> +if (!rc || acquire_drc)
>>  dlpar_free_cc_nodes(dn);
> 
> This seems like it would be more readable if everything were inside the
> if (acquire_drc) block.
> 
>>
>>  return saved_rc;
>> @@ -540,7 +544,7 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
>>  dn->name, rc, drc_index);
>>
>>  rc = dlpar_detach_node(dn);
>> -if (!rc)
>> +if (!rc && acquire_drc)
>>  dlpar_release_drc(drc_index);
>>
>>  return saved_rc;
>> @@ -608,12 +612,13 @@ static int dlpar_offline_cpu(struct device_node *dn)
>>
>>  }
>>
>> -static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index)
>> +static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index,
>> +bool release_drc)
>>  {
>>  int rc;
>>
>> -pr_debug("Attempting to remove CPU %s, drc index: %x\n",
>> - dn->name, drc_index);
>> +pr_debug("Attempting to remove CPU %s, drc index: %x (%d)\n",
>> + dn->name, drc_index, release_drc);
>>
>>  rc =

Re: [RFC v4 2/3] powerpc migration/cpu: Associativity & cpu changes

2018-05-18 Thread Nathan Fontenot 
On 05/17/2018 05:26 PM, Michael Bringmann wrote:
> powerpc migration/cpu: Now apply changes to the associativity of cpus
> for the topology of LPARS in Post Migration events.  Recognize more
> changes to the associativity of memory blocks described by the
> 'cpu' properties when processing the topology of LPARS in Post Migration
> events.  Previous efforts only recognized whether a memory block's
> assignment had changed in the property.  Changes here include:
> 
> * Provide hotplug CPU 'readd by index' operation
> * Checking for changes in cpu associativity and making 'readd' calls
>   when differences are observed.
> * Queue up  changes to CPU properties so that they may take place
>   after all PowerPC device-tree changes have been applied i.e. after
>   the device hotplug is released in the mobility code.

This kinda feels like three different patches in one. any reason to not split
this into three patches? Perhaps at the least split the last item into it's
own patch.

> 
> Signed-off-by: Michael Bringmann 
> ---
> Changes include:
>   -- Rearrange patches to co-locate CPU property-related changes.
>   -- Modify dlpar_cpu_add & dlpar_cpu_remove to skip DRC index acquire
>  or release operations during the CPU readd process.
>   -- Correct a bug in DRC index selection for queued operation.
>   -- Rebase to 4.17-rc5 kernel
> ---
>  arch/powerpc/platforms/pseries/hotplug-cpu.c |  123 
> +++---
>  arch/powerpc/platforms/pseries/mobility.c|3 +
>  2 files changed, 95 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c 
> b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index a408217..23d4cb8 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -474,7 +474,7 @@ static bool valid_cpu_drc_index(struct device_node 
> *parent, u32 drc_index)
>   );
>  }
> 
> -static ssize_t dlpar_cpu_add(u32 drc_index)
> +static ssize_t dlpar_cpu_add(u32 drc_index, bool acquire_drc)
>  {
>   struct device_node *dn, *parent;
>   int rc, saved_rc;
> @@ -499,19 +499,22 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
>   return -EINVAL;
>   }
> 
> - rc = dlpar_acquire_drc(drc_index);
> - if (rc) {
> - pr_warn("Failed to acquire DRC, rc: %d, drc index: %x\n",
> - rc, drc_index);
> - of_node_put(parent);
> - return -EINVAL;
> + if (acquire_drc) {
> + rc = dlpar_acquire_drc(drc_index);
> + if (rc) {
> + pr_warn("Failed to acquire DRC, rc: %d, drc index: 
> %x\n",
> + rc, drc_index);
> + of_node_put(parent);
> + return -EINVAL;
> + }
>   }
> 
>   dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
>   if (!dn) {
>   pr_warn("Failed call to configure-connector, drc index: %x\n",
>   drc_index);
> - dlpar_release_drc(drc_index);
> + if (acquire_drc)
> + dlpar_release_drc(drc_index);
>   of_node_put(parent);
>   return -EINVAL;
>   }
> @@ -526,8 +529,9 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
>   pr_warn("Failed to attach node %s, rc: %d, drc index: %x\n",
>   dn->name, rc, drc_index);
> 
> - rc = dlpar_release_drc(drc_index);
> - if (!rc)
> + if (acquire_drc)
> + rc = dlpar_release_drc(drc_index);
> + if (!rc || acquire_drc)
>   dlpar_free_cc_nodes(dn);

This seems like it would be more readable if everything were inside the
if (acquire_drc) block.

> 
>   return saved_rc;
> @@ -540,7 +544,7 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
>   dn->name, rc, drc_index);
> 
>   rc = dlpar_detach_node(dn);
> - if (!rc)
> + if (!rc && acquire_drc)
>   dlpar_release_drc(drc_index);
> 
>   return saved_rc;
> @@ -608,12 +612,13 @@ static int dlpar_offline_cpu(struct device_node *dn)
> 
>  }
> 
> -static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index)
> +static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index,
> + bool release_drc)
>  {
>   int rc;
> 
> - pr_debug("Attempting to remove CPU %s, drc index: %x\n",
> -  dn->name, drc_index);
> + pr_debug("Attempting to remove CPU %s, drc index: %x (%d)\n",
> +  dn->name, drc_index, release_drc);
> 
>   rc = dlpar_offline_cpu(dn);
>   if (rc) {
> @@ -621,12 +626,14 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, 
> u32 drc_index)
>   return -EINVAL;
>   }
> 
> - rc = dlpar_release_drc(drc_index);
> - if (rc) {
> -

[RFC v4 2/3] powerpc migration/cpu: Associativity & cpu changes

2018-05-17 Thread Michael Bringmann 
powerpc migration/cpu: Now apply changes to the associativity of cpus
for the topology of LPARS in Post Migration events.  Recognize more
changes to the associativity of memory blocks described by the
'cpu' properties when processing the topology of LPARS in Post Migration
events.  Previous efforts only recognized whether a memory block's
assignment had changed in the property.  Changes here include:

* Provide hotplug CPU 'readd by index' operation
* Checking for changes in cpu associativity and making 'readd' calls
  when differences are observed.
* Queue up  changes to CPU properties so that they may take place
  after all PowerPC device-tree changes have been applied i.e. after
  the device hotplug is released in the mobility code.

Signed-off-by: Michael Bringmann 
---
Changes include:
  -- Rearrange patches to co-locate CPU property-related changes.
  -- Modify dlpar_cpu_add & dlpar_cpu_remove to skip DRC index acquire
 or release operations during the CPU readd process.
  -- Correct a bug in DRC index selection for queued operation.
  -- Rebase to 4.17-rc5 kernel
---
 arch/powerpc/platforms/pseries/hotplug-cpu.c |  123 +++---
 arch/powerpc/platforms/pseries/mobility.c|3 +
 2 files changed, 95 insertions(+), 31 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c 
b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index a408217..23d4cb8 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -474,7 +474,7 @@ static bool valid_cpu_drc_index(struct device_node *parent, 
u32 drc_index)
);
 }
 
-static ssize_t dlpar_cpu_add(u32 drc_index)
+static ssize_t dlpar_cpu_add(u32 drc_index, bool acquire_drc)
 {
struct device_node *dn, *parent;
int rc, saved_rc;
@@ -499,19 +499,22 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
return -EINVAL;
}
 
-   rc = dlpar_acquire_drc(drc_index);
-   if (rc) {
-   pr_warn("Failed to acquire DRC, rc: %d, drc index: %x\n",
-   rc, drc_index);
-   of_node_put(parent);
-   return -EINVAL;
+   if (acquire_drc) {
+   rc = dlpar_acquire_drc(drc_index);
+   if (rc) {
+   pr_warn("Failed to acquire DRC, rc: %d, drc index: 
%x\n",
+   rc, drc_index);
+   of_node_put(parent);
+   return -EINVAL;
+   }
}
 
dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
if (!dn) {
pr_warn("Failed call to configure-connector, drc index: %x\n",
drc_index);
-   dlpar_release_drc(drc_index);
+   if (acquire_drc)
+   dlpar_release_drc(drc_index);
of_node_put(parent);
return -EINVAL;
}
@@ -526,8 +529,9 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
pr_warn("Failed to attach node %s, rc: %d, drc index: %x\n",
dn->name, rc, drc_index);
 
-   rc = dlpar_release_drc(drc_index);
-   if (!rc)
+   if (acquire_drc)
+   rc = dlpar_release_drc(drc_index);
+   if (!rc || acquire_drc)
dlpar_free_cc_nodes(dn);
 
return saved_rc;
@@ -540,7 +544,7 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
dn->name, rc, drc_index);
 
rc = dlpar_detach_node(dn);
-   if (!rc)
+   if (!rc && acquire_drc)
dlpar_release_drc(drc_index);
 
return saved_rc;
@@ -608,12 +612,13 @@ static int dlpar_offline_cpu(struct device_node *dn)
 
 }
 
-static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index)
+static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index,
+   bool release_drc)
 {
int rc;
 
-   pr_debug("Attempting to remove CPU %s, drc index: %x\n",
-dn->name, drc_index);
+   pr_debug("Attempting to remove CPU %s, drc index: %x (%d)\n",
+dn->name, drc_index, release_drc);
 
rc = dlpar_offline_cpu(dn);
if (rc) {
@@ -621,12 +626,14 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, 
u32 drc_index)
return -EINVAL;
}
 
-   rc = dlpar_release_drc(drc_index);
-   if (rc) {
-   pr_warn("Failed to release drc (%x) for CPU %s, rc: %d\n",
-   drc_index, dn->name, rc);
-   dlpar_online_cpu(dn);
-   return rc;
+   if (release_drc) {
+   rc = dlpar_release_drc(drc_index);
+   if (rc) {
+   pr_warn("Failed to release drc (%x) for CPU %s, rc: 
%d\n",
+   drc_index, dn->name, rc);
+