[RFC v4 3/4] hotplug/drcinfo: Fix hot-add CPU issues

Michael Bringmann mwb at linux.vnet.ibm.com
Wed May 23 11:14:45 AEST 2018


See below.

On 05/22/2018 04:31 PM, Nathan Fontenot wrote:
> On 05/22/2018 11:37 AM, Michael Bringmann wrote:
>> This patch applies a common parse function for the ibm,drc-info
>> property that can be modified by a callback function to the
>> hot-add CPU code.  Candidate code is replaced by a call to the
>> parser including a pointer to a local context-specific functions,
>> and local data.
>>
>> In addition, a bug in the release of the previous patch set may
>> break things in some of the CPU DLPAR operations.  For instance,
>> when attempting to hot-add a new CPU or set of CPUs, the original
>> patch failed to always properly calculate the available resources,
>> and aborted the operation.
>>
>> Signed-off-by: Michael Bringmann <mwb at linux.vnet.ibm.com>
>> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar
>> e feature" -- end of patch series applied to powerpc next)
>> ---
>> Changes in V4:
>>   -- Update code to account for latest kernel checkins.
>>   -- Rebased to 4.17-rc5 kernel
>>   -- Compress some more code
>> ---
>>  arch/powerpc/platforms/pseries/hotplug-cpu.c    |  118 +++++++++++++++++------
>>  arch/powerpc/platforms/pseries/pseries_energy.c |  107 +++++++++++----------
>>  2 files changed, 141 insertions(+), 84 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index 6ef77ca..ceacad9 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -411,27 +411,63 @@ static bool dlpar_cpu_exists(struct device_node *parent, u32 drc_index)
>>  	return found;
>>  }
>>
>> -static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
>> +static bool check_cpu_drc_index(struct device_node *parent,
>> +				int (*cb)(struct of_drc_info *drc,
>> +					void *data, void *not_used,
>> +					int *ret_code),
>> +				void *cdata)
>>  {
>>  	bool found = false;
>> -	int rc, index;
>>
>> -	index = 0;
>> -	while (!found) {
>> -		u32 drc;
>> +	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
>> +		if (drc_info_parser(parent, cb, "CPU", cdata))
>> +			found = true;
>> +	} else {
>> +		int index = 0;
>>
>> -		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
>> -						index++, &drc);
>> -		if (rc)
>> -			break;
>> +		while (!found) {
>> +			u32 drc;
>>
>> -		if (drc == drc_index)
>> -			found = true;
>> +			if (of_property_read_u32_index(parent,
>> +						"ibm,drc-indexes",
>> +						index++, &drc))
>> +				break;
>> +			if (cb(NULL, cdata, &drc, NULL))
>> +				found = true;
>> +		}
>>  	}
>>
>>  	return found;
>>  }
>>
>> +struct valid_drc_index_struct {
>> +	u32 targ_drc_index;
>> +};
> 
> Can you help me understand the need to encapsulate the drc_index as a struct.

At this point, it is consistency of use across all of the instances
of using 'walk_drc_info'.  I believe that there were more values in
the structure earlier on.

> 
>> +
>> +static int valid_drc_index_cb(struct of_drc_info *drc, void *idata,
>> +				void *drc_index, int *ret_code)
>> +{
>> +	struct valid_drc_index_struct *cdata = idata;
>> +
>> +	if (drc) {
>> +		if (!((drc->drc_index_start <= cdata->targ_drc_index) &&
>> +			(cdata->targ_drc_index <= drc->last_drc_index)))
>> +			return 0;
>> +	} else {
>> +		if (*((u32 *)drc_index) != cdata->targ_drc_index)
>> +			return 0;
>> +	}
>> +	(*ret_code) = 1;
>> +	return 1;
>> +}
>> +
>> +static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
>> +{
>> +	struct valid_drc_index_struct cdata = { drc_index };
>> +
>> +	return check_cpu_drc_index(parent, valid_drc_index_cb, &cdata);
>> +}
>> +
>>  static ssize_t dlpar_cpu_add(u32 drc_index)
>>  {
>>  	struct device_node *dn, *parent;
>> @@ -721,11 +757,43 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
>>  	return rc;
>>  }
>>
>> +struct cpus_to_add_struct {
>> +	struct device_node *parent;
>> +	u32 *cpu_drcs;
>> +	u32 cpus_to_add;
>> +	u32 cpus_found;
>> +};
>> +
>> +static int cpus_to_add_cb(struct of_drc_info *drc, void *idata,
>> +			void *drc_index, int *ret_code)
>> +{
>> +	struct cpus_to_add_struct *cdata = idata;
>> +
>> +	if (drc) {
>> +		int k;
>> +
>> +		for (k = 0; (k < drc->num_sequential_elems) &&
>> +			(cdata->cpus_found < cdata->cpus_to_add); k++) {
>> +			u32 idrc = drc->drc_index_start +
>> +				(k * drc->sequential_inc);
>> +
>> +			if (dlpar_cpu_exists(cdata->parent, idrc))
>> +				continue;
>> +			cdata->cpu_drcs[cdata->cpus_found++] = idrc;
>> +		}
>> +	} else {
>> +		if (!dlpar_cpu_exists(cdata->parent, *((u32 *)drc_index)))
>> +			cdata->cpu_drcs[cdata->cpus_found++] =
>> +				*((u32 *)drc_index);
>> +	}
>> +	return 0;
>> +}
>> +
>>  static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>>  {
>>  	struct device_node *parent;
>> -	int cpus_found = 0;
>> -	int index, rc;
>> +	struct cpus_to_add_struct cdata = {
>> +		NULL, cpu_drcs, cpus_to_add, 0 };
>>
>>  	parent = of_find_node_by_path("/cpus");
>>  	if (!parent) {
>> @@ -734,28 +802,14 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>>  		return -1;
>>  	}
>>
>> -	/* Search the ibm,drc-indexes array for possible CPU drcs to
>> -	 * add. Note that the format of the ibm,drc-indexes array is
>> -	 * the number of entries in the array followed by the array
>> -	 * of drc values so we start looking at index = 1.
>> +	/* Search the appropriate property for possible CPU drcs to
>> +	 * add.
>>  	 */
>> -	index = 1;
>> -	while (cpus_found < cpus_to_add) {
>> -		u32 drc;
>> -
>> -		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
>> -						index++, &drc);
>> -		if (rc)
>> -			break;
>> -
>> -		if (dlpar_cpu_exists(parent, drc))
>> -			continue;
>> -
>> -		cpu_drcs[cpus_found++] = drc;
>> -	}
>> +	cdata.parent = parent;
>> +	check_cpu_drc_index(parent, cpus_to_add_cb, &cdata);
>>
>>  	of_node_put(parent);
>> -	return cpus_found;
>> +	return cdata.cpus_found;
>>  }
>>
>>  static int dlpar_cpu_add_by_count(u32 cpus_to_add)
>> diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c
>> index 5261975..d8d7750 100644
>> --- a/arch/powerpc/platforms/pseries/pseries_energy.c
>> +++ b/arch/powerpc/platforms/pseries/pseries_energy.c
>> @@ -36,6 +36,26 @@
>>
>>  /* Helper Routines to convert between drc_index to cpu numbers */
>>
>> +struct cpu_to_drc_index_struct {
>> +	u32	thread_index;
>> +	u32	ret;
>> +};
>> +
>> +static int cpu_to_drc_index_cb(struct of_drc_info *drc, void *idata,
>> +				void *not_used, int *ret_code)
>> +{
>> +	struct cpu_to_drc_index_struct *cdata = idata;
>> +	int ret = 0;
>> +
>> +	if (cdata->thread_index < drc->last_drc_index) {
> 
> Is this correct? You're comparing a thread index to a drc index.

I believe so.  I will check when I retest this week, hopefully.

> 
>> +		cdata->ret = drc->drc_index_start +
>> +			(cdata->thread_index * drc->sequential_inc);
>> +		ret = 1;
>> +	}
>> +	(*ret_code) = ret;
>> +	return ret;
>> +}
>> +
>>  static u32 cpu_to_drc_index(int cpu)
>>  {
>>  	struct device_node *dn = NULL;
>> @@ -51,30 +71,14 @@ static u32 cpu_to_drc_index(int cpu)
>>  	thread_index = cpu_core_index_of_thread(cpu);
>>
>>  	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
>> -		struct property *info = NULL;
>> -		struct of_drc_info drc;
>> -		int j;
>> -		u32 num_set_entries;
>> -		const __be32 *value;
>> -
>> -		info = of_find_property(dn, "ibm,drc-info", NULL);
>> -		if (info == NULL)
>> -			goto err_of_node_put;
>> +		struct cpu_to_drc_index_struct cdata = {
>> +			thread_index, 0 };
>>
>> -		value = info->value;
>> -		num_set_entries = of_read_number(value++, 1);
>> -
>> -		for (j = 0; j < num_set_entries; j++) {
>> -
>> -			of_read_drc_info_cell(&info, &value, &drc);
>> -			if (strncmp(drc.drc_type, "CPU", 3))
>> -				goto err;
>> -
>> -			if (thread_index < drc.last_drc_index)
>> -				break;
>> -		}
>> -
>> -		ret = drc.drc_index_start + (thread_index * drc.sequential_inc);
>> +		rc = drc_info_parser(dn, &cpu_to_drc_index_cb,
>> +					"CPU", &cdata);
>> +		if (rc < 0)
>> +			goto err_of_node_put;
>> +		ret = cdata.ret;
>>  	} else {
>>  		const __be32 *indexes;
>>
>> @@ -100,11 +104,33 @@ static u32 cpu_to_drc_index(int cpu)
>>  	return ret;
>>  }
>>
>> +struct drc_index_to_cpu_struct {
>> +	u32	drc_index;
>> +	u32	thread_index;
>> +	u32	cpu;
>> +};
>> +
>> +static int drc_index_to_cpu_cb(struct of_drc_info *drc,
>> +				void *idata, void *not_used, int *ret_code)
>> +{
>> +	struct drc_index_to_cpu_struct *cdata = idata;
>> +
>> +	if (cdata->drc_index > drc->last_drc_index) {
>> +		cdata->cpu += drc->num_sequential_elems;
>> +	} else {
>> +		cdata->cpu += ((cdata->drc_index - drc->drc_index_start) /
>> +				drc->sequential_inc);
>> +		cdata->thread_index = cpu_first_thread_of_core(cdata->cpu);
> 
> Should this return 1 here to avoid continuing to walk the drc_info entries?

Yes.

> 
> -Nathan

Michael

> 
>> +	}
>> +	(*ret_code) = 0;
>> +	return 0;
>> +}
>> +
>>  static int drc_index_to_cpu(u32 drc_index)
>>  {
>>  	struct device_node *dn = NULL;
>>  	const int *indexes;
>> -	int thread_index = 0, cpu = 0;
>> +	int thread_index = 0;
>>  	int rc = 1;
>>
>>  	dn = of_find_node_by_path("/cpus");
>> @@ -112,36 +138,13 @@ static int drc_index_to_cpu(u32 drc_index)
>>  		goto err;
>>
>>  	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
>> -		struct property *info = NULL;
>> -		struct of_drc_info drc;
>> -		int j;
>> -		u32 num_set_entries;
>> -		const __be32 *value;
>> -
>> -		info = of_find_property(dn, "ibm,drc-info", NULL);
>> -		if (info == NULL)
>> -			goto err_of_node_put;
>> -
>> -		value = info->value;
>> -		num_set_entries = of_read_number(value++, 1);
>> -
>> -		for (j = 0; j < num_set_entries; j++) {
>> +		struct drc_index_to_cpu_struct cdata = {
>> +			drc_index, 0, 0 };
>>
>> -			of_read_drc_info_cell(&info, &value, &drc);
>> -			if (strncmp(drc.drc_type, "CPU", 3))
>> -				goto err;
>> +		rc = drc_info_parser(dn, &drc_index_to_cpu_cb,
>> +					"CPU", &cdata);
>> +		thread_index = cdata.thread_index;
>>
>> -			if (drc_index > drc.last_drc_index) {
>> -				cpu += drc.num_sequential_elems;
>> -				continue;
>> -			}
>> -			cpu += ((drc_index - drc.drc_index_start) /
>> -				drc.sequential_inc);
>> -
>> -			thread_index = cpu_first_thread_of_core(cpu);
>> -			rc = 0;
>> -			break;
>> -		}
>>  	} else {
>>  		unsigned long int i;
>>
> 
> 

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb at linux.vnet.ibm.com



More information about the Linuxppc-dev mailing list