Re: [PATCH v2 4/6] powerpc/pseries: Add CPU dlpar remove functionality

[Nathan Fontenot <nfont@linux.vnet.ibm.com>]

On 11/25/2015 11:02 PM, Michael Ellerman wrote:
> On Tue, 2015-10-27 at 13:26 -0500, Nathan Fontenot wrote:
> 
>> Add the ability to dlpar remove CPUs via hotplug rtas events, either by
>> specifying the drc-index of the CPU to remove or providing a count of cpus
>> to remove.
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index f080e81..635f0ba 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -570,6 +571,143 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index)
>>  	return 0;
>>  }
>>  
>> +static struct device_node *cpu_drc_index_to_dn(u32 drc_index)
>> +{
>> +	struct device_node *dn;
>> +	u32 my_index;
>> +	int rc;
>> +
>> +	for_each_node_by_type(dn, "cpu") {
>> +		rc = of_property_read_u32(dn, "ibm,my-drc-index", &my_index);
>> +		if (rc)
>> +			continue;
>> +
>> +		if (my_index == drc_index)
>> +			break;
>> +	}
>> +
>> +	return dn;
>> +}
>> +
>> +static int dlpar_cpu_remove_by_index(u32 drc_index)
>> +{
>> +	struct device_node *dn;
>> +	int rc;
>> +
>> +	dn = cpu_drc_index_to_dn(drc_index);
>> +	if (!dn)
>> +		return -ENODEV;
>> +
>> +	rc = dlpar_cpu_remove(dn, drc_index);
>> +	of_node_put(dn);
> 
> Does dlpar_cpu_remove() work when you still hold a reference to the dn?

Yes, this works while holding the dn reference here.
> 
> It looks like dlpar_detach_node() does an of_node_put() also.

Correct. The of_node_put() in dlpar_detach_node() is there to do a put
from the initial node creation, without this the reference count would never
go to zero and the node would not be released.

> 
>> +	return rc;
>> +}
>> +
>> +static u32 *dlpar_cpus_to_remove(int cpus_to_remove)
>> +{
>> +	struct device_node *dn;
>> +	u32 *cpu_drcs;
>> +	int cpus_found = 0;
>> +	int i, rc;
>> +
>> +	cpu_drcs = kcalloc(cpus_to_remove, sizeof(*cpu_drcs), GFP_KERNEL);
>> +	if (!cpu_drcs)
>> +		return NULL;
>> +
>> +	i = 0;
>> +	for_each_node_by_type(dn, "cpu") {
>> +		cpus_found++;
>> +
>> +		if (cpus_found > cpus_to_remove) {
>> +			of_node_put(dn);
>> +			break;
>> +		}
>> +
>> +		rc = of_property_read_u32(dn, "ibm,my-drc-index",
>> +					  &cpu_drcs[i++]);
>> +		if (rc) {
>> +			of_node_put(dn);
>> +			break;
> 
> I'm not sure about the logic here with cpus_found and i. If you break here
> cpus_found will be 1, but you found zero, which seems odd.
> 

Agreed, it is a bit odd. The cpus_found var is meant to count the number of
cpus we find when looping through for_each_node_by_type() whereas i is just
meant to be an index into the cpu_cars array so we can save the drc-index
of the cpus we find. The variable i is just to index the array, nothing more.

Perhaps instead of having i to use as an index into the cpu_drcs array I
could just use [cpus_found - 1] to index cpu_drcs and get rid of i. Not sure
if that makes the code any easier to read.

> If instead you delayed the increment of i:
> 
> 		rc = of_property_read_u32(dn, "ibm,my-drc-index",
> 					  &cpu_drcs[i]);
> 		if (rc) {
> 			of_node_put(dn);
> 			break;
> 		}
> 
> 		i++;
> 	}
> 
> Then i would equal the number of cpus found at all times. If you need to count
> one more in the if below you can just do that there.
> 
>> +
>> +	/* We want to find cpus_to_remove + 1 CPUs to ensure we do not
>> +	 * remove the last CPU.
>> +	 */
>> +	if (cpus_found <= cpus_to_remove) {
>> +		pr_warn("Failed to find enough CPUs (%d of %d) to remove\n",
>> +			cpus_found, cpus_to_remove);
>> +		kfree(cpu_drcs);
>> +		cpu_drcs = NULL;
>> +	}
> 
> On my two cpu system when I do "cpu remove count 1" this always says:
> 
>   pseries-hotplug-cpu: Failed to find enough CPUs (1 of 1) to remove
> 
> Which confuses me.
> 
> I suspect that's because I actually have one cpu *node*, which appears to Linux
> as two cpus (due to SMT). So I think it's working as expected, but it's not
> very clear from a user's perspective.
> 

You are correct. For Power CPU DLPAR works on a node basis. If there
is only one node it will not remove the last CPU.

I should update the cpus_found to check to print a message if the remove
request would entail removing the last CPU and that the request is being
failed because of that.


Thanks for the feedback,
-Nathan