Re: [PATCH 05/11] powerpc/smp: Dont assume l2-cache to be superset of sibling

Hi Srikar,

On Mon, Jul 20, 2020 at 12:15:04PM +0530, Srikar Dronamraju wrote:

* Gautham R Shenoy [off-list ref] [2020-07-17 11:30:11]:

quoted

Hi Srikar,

On Tue, Jul 14, 2020 at 10:06:18AM +0530, Srikar Dronamraju wrote:

quoted

Current code assumes that cpumask of cpus sharing a l2-cache mask will
always be a superset of cpu_sibling_mask.

Lets stop that assumption.

Cc: linuxppc-dev <redacted>
Cc: Michael Ellerman <redacted>
Cc: Nick Piggin <redacted>
Cc: Oliver OHalloran <redacted>
Cc: Nathan Lynch <redacted>
Cc: Michael Neuling <redacted>
Cc: Anton Blanchard <redacted>
Cc: Gautham R Shenoy <redacted>
Cc: Vaidyanathan Srinivasan <redacted>
Signed-off-by: Srikar Dronamraju <redacted>
---
 arch/powerpc/kernel/smp.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 7d430fc536cc..875f57e41355 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c

@@ -1198,6 +1198,7 @@ static bool update_mask_by_l2(int cpu, struct cpumask *(*mask_fn)(int))
 	struct device_node *l2_cache, *np;
 	int i;

+	cpumask_set_cpu(cpu, mask_fn(cpu));

It would be good to comment why do we need to do set the CPU in the
l2-mask if we don't have a l2cache domain.

Good Catch, 
We should move this after the cpu_to_l2cache.

quoted

quoted

 	l2_cache = cpu_to_l2cache(cpu);
 	if (!l2_cache)
 		return false;

@@ -1284,29 +1285,30 @@ static void add_cpu_to_masks(int cpu)
 	 * add it to it's own thread sibling mask.
 	 */
 	cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
+	cpumask_set_cpu(cpu, cpu_core_mask(cpu));

 	for (i = first_thread; i < first_thread + threads_per_core; i++)
 		if (cpu_online(i))
 			set_cpus_related(i, cpu, cpu_sibling_mask);

 	add_cpu_to_smallcore_masks(cpu);
-	/*
-	 * Copy the thread sibling mask into the cache sibling mask
-	 * and mark any CPUs that share an L2 with this CPU.
-	 */
-	for_each_cpu(i, cpu_sibling_mask(cpu))
-		set_cpus_related(cpu, i, cpu_l2_cache_mask);
 	update_mask_by_l2(cpu, cpu_l2_cache_mask);

-	/*
-	 * Copy the cache sibling mask into core sibling mask and mark
-	 * any CPUs on the same chip as this CPU.
-	 */
-	for_each_cpu(i, cpu_l2_cache_mask(cpu))
-		set_cpus_related(cpu, i, cpu_core_mask);
+	if (pkg_id == -1) {
+		struct cpumask *(*mask)(int) = cpu_sibling_mask;
+
+		/*
+		 * Copy the sibling mask into core sibling mask and
+		 * mark any CPUs on the same chip as this CPU.
+		 */
+		if (shared_caches)
+			mask = cpu_l2_cache_mask;
+

Now that we decoupling the containment relationship between
sibling_mask and l2-cache mask, should we set all the CPUs that are
both in cpu_sibling_mask(cpu) as well as cpu_l2_mask(cpu) in
cpu_core_mask ? 

Are you saying instead of setting this cpu in this cpu_core_mask, can we set
all the cpus in the mask in cpu_core_mask?

No. What I am referring to is in the for-loop below, you are setting
the CPUs that are set in mask(cpu) in the cpu_core_mask.

Now, the above code sets
mask(cpu) == cpu_sibling_mask(cpu) in the absence of shared_caches, and 
          == cpu_l2_cache_mask(cpu) in the presence of shared_cache.

Since we have decoupled the assumption that cpu_sibling_mask(cpu) may not
be contained within cpu_l2_cache_mask(cpu), in the presence of a
shared-cache, why are we only picking the CPUs in
cpu_l2_cache_mask(cpu) to be set in cpu_core_maks(cpu) ? It should
ideally be the superset whose CPUs should be set in
cpu_core_mask(cpu). And the correct cpuset is
cpumask_or(cpu_sibling_mask(cpu), cpu_l2_cache_mask(cpu))

Currently we dont know if any of the cpus of the mask were already set or
not. Plus we need to anyway update cpumask of all other cpus to says they
are related. So setting a mask instead of cpu at a time will not change
anything for our side.

quoted

quoted

+		for_each_cpu(i, mask(cpu))
+			set_cpus_related(cpu, i, cpu_core_mask);

-	if (pkg_id == -1)
 		return;
+	}

 	for_each_cpu(i, cpu_online_mask)
 		if (get_physical_package_id(i) == pkg_id)
-- 
2.17.1

--
Thanks and Regards
gautham.

-- 
Thanks and Regards
Srikar Dronamraju