Re: [RFC PATCH v2 6/7] lib/persubnode: Introducing a simple per-subnode APIs

[Waiman Long <waiman.long@hpe.com>]

On 07/11/2016 11:14 PM, Boqun Feng wrote:
> On Mon, Jul 11, 2016 at 01:32:11PM -0400, Waiman Long wrote:
>> +/*
>> + * Initialize the subnodes
>> + *
>> + * All the sibling CPUs will be in the same subnode. On top of that, we will
>> + * put at most 2 sibling groups into the same subnode. The percpu
>> + * topology_sibling_cpumask() and topology_core_cpumask() are used for
>> + * grouping CPUs into subnodes. The subnode ID is the CPU number of the
>> + * first CPU in the subnode.
>> + */
>> +static int __init subnode_init(void)
>> +{
>> +	int cpu;
>> +	int nr_subnodes = 0;
>> +	const int subnode_nr_cpus = 2;
>> +
>> +	/*
>> +	 * Some of the bits in the subnode_mask will be cleared as we proceed.
>> +	 */
>> +	for_each_cpu(cpu, subnode_mask) {
>> +		int ccpu, scpu;
>> +		int cpucnt = 0;
>> +
>> +		cpumask_var_t core_mask = topology_core_cpumask(cpu);
>> +		cpumask_var_t sibling_mask;
>> +
>> +		/*
>> +		 * Put subnode_nr_cpus of CPUs and their siblings into each
>> +		 * subnode.
>> +		 */
>> +		for_each_cpu_from(cpu, ccpu, core_mask) {
>> +			sibling_mask = topology_sibling_cpumask(ccpu);
>> +			for_each_cpu_from(ccpu, scpu, sibling_mask) {
>> +				/*
>> +				 * Clear the bits of the higher CPUs.
>> +				 */
>> +				if (scpu>  cpu)
>> +					cpumask_clear_cpu(scpu, subnode_mask);
> Do we also need to clear the 'core_mask' here? Consider a core consist
> of two sibling groups and each sibling group consist of two cpus. At the
> beginning of the outer loop(for_each_cpu_from(cpu, ccpu, core_mask)):
>
> 'core_mask' is 0b1111
>
> so at the beginning of the inner loop first time:
>
> 'ccpu' is 0, therefore 'sibling_mask' is 0b1100, in this loop we set the
> 'cpu_subnode_id' of cpu 0 and 1 to 0.
>
> at the beginning of the inner loop second time:
>
> 'ccpu' is 1 because we don't clear cpu 1 from 'core_mask'. Therefore
> sibling_mask is still 0b1100, so in this loop we still do the setting on
> 'cpu_subnode_id' of cpu 0 and 1.
>
> Am I missing something here?
>

You are right. The current code work in my test as the 2 sibling CPUs 
occupy the a lower and higher numbers like (0, 72) for a 72-core system. 
It may not work for other sibling CPU assignment.

The core_mask, however, is a global data variable and we cannot modify 
it. I will make the following change instead:

diff --git a/lib/persubnode.c b/lib/persubnode.c
index 9febe7c..d1c8c29 100644
--- a/lib/persubnode.c
+++ b/lib/persubnode.c
@@ -94,6 +94,8 @@ static int __init subnode_init(void)
                  * subnode.
                  */
                 for_each_cpu_from(cpu, ccpu, core_mask) {
+                       if (!cpumask_test_cpu(ccpu, subnode_mask))
+                               continue;       /* Skip allocated CPU */
                         sibling_mask = topology_sibling_cpumask(ccpu);
                         for_each_cpu_from(ccpu, scpu, sibling_mask) {
                                 /*

Thanks for catching this bug.

Cheers,
Longman