From: Srikar Dronamraju <srikar@linux.vnet.ibm.com> To: Gautham R Shenoy <ego@linux.vnet.ibm.com> Cc: Michael Ellerman <michaele@au1.ibm.com>, linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, LKML <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Valentin Schneider <valentin.schneider@arm.com>, Nick Piggin <npiggin@au1.ibm.com>, Oliver OHalloran <oliveroh@au1.ibm.com>, Nathan Lynch <nathanl@linux.ibm.com>, Michael Neuling <mikey@linux.ibm.com>, Anton Blanchard <anton@au1.ibm.com>, Vaidyanathan Srinivasan <svaidy@linux.ibm.com>, Jordan Niethe <jniethe5@gmail.com> Subject: Re: [PATCH v2 05/10] powerpc/smp: Dont assume l2-cache to be superset of sibling Date: Wed, 22 Jul 2020 12:27:47 +0530 [thread overview] Message-ID: <20200722065747.GB9290@linux.vnet.ibm.com> (raw) In-Reply-To: <20200722062114.GD31038@in.ibm.com> * Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-22 11:51:14]: > Hi Srikar, > > > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c > > index 72f16dc0cb26..57468877499a 100644 > > --- a/arch/powerpc/kernel/smp.c > > +++ b/arch/powerpc/kernel/smp.c > > @@ -1196,6 +1196,7 @@ static bool update_mask_by_l2(int cpu, struct cpumask *(*mask_fn)(int)) > > if (!l2_cache) > > return false; > > > > + cpumask_set_cpu(cpu, mask_fn(cpu)); > > > Ok, we need to do this because "cpu" is not yet set in the > cpu_online_mask. Prior to your patch the "cpu" was getting set in > cpu_l2_cache_map(cpu) as a side-effect of the code that is removed in > the patch. > Right. > > > for_each_cpu(i, cpu_online_mask) { > > /* > > * when updating the marks the current CPU has not been marked > > @@ -1278,29 +1279,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)); Note: Above, we are explicitly setting the cpu_core_mask. > > > > 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) { > > I suppose this "if" condition is an optimization, since if pkg_id != -1, > we anyway set these CPUs in the cpu_core_mask below. > > However... This is not just an optimization. The hunk removed would only work if cpu_l2_cache_mask is bigger than cpu_sibling_mask. (this was the previous assumption that we want to break) If the cpu_sibling_mask is bigger than cpu_l2_cache_mask and pkg_id is -1, then setting only cpu_l2_cache_mask in cpu_core_mask will result in a broken topology. > > > + 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; > > + > > + for_each_cpu(i, mask(cpu)) > > + set_cpus_related(cpu, i, cpu_core_mask); > > > > - if (pkg_id == -1) > > return; > > + } > > > ... since "cpu" is not yet set in the cpu_online_mask, do we not miss setting > "cpu" in the cpu_core_mask(cpu) in the for-loop below ? > > As noted above, we are setting before. So we don't missing the cpu and hence have not different from before. > -- > Thanks and Regards > gautham. -- Thanks and Regards Srikar Dronamraju
WARNING: multiple messages have this Message-ID (diff)
From: Srikar Dronamraju <srikar@linux.vnet.ibm.com> To: Gautham R Shenoy <ego@linux.vnet.ibm.com> Cc: Nathan Lynch <nathanl@linux.ibm.com>, Oliver OHalloran <oliveroh@au1.ibm.com>, Michael Neuling <mikey@linux.ibm.com>, Michael Ellerman <michaele@au1.ibm.com>, Peter Zijlstra <peterz@infradead.org>, Jordan Niethe <jniethe5@gmail.com>, Anton Blanchard <anton@au1.ibm.com>, LKML <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@kernel.org>, Nick Piggin <npiggin@au1.ibm.com>, linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, Valentin Schneider <valentin.schneider@arm.com> Subject: Re: [PATCH v2 05/10] powerpc/smp: Dont assume l2-cache to be superset of sibling Date: Wed, 22 Jul 2020 12:27:47 +0530 [thread overview] Message-ID: <20200722065747.GB9290@linux.vnet.ibm.com> (raw) In-Reply-To: <20200722062114.GD31038@in.ibm.com> * Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-22 11:51:14]: > Hi Srikar, > > > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c > > index 72f16dc0cb26..57468877499a 100644 > > --- a/arch/powerpc/kernel/smp.c > > +++ b/arch/powerpc/kernel/smp.c > > @@ -1196,6 +1196,7 @@ static bool update_mask_by_l2(int cpu, struct cpumask *(*mask_fn)(int)) > > if (!l2_cache) > > return false; > > > > + cpumask_set_cpu(cpu, mask_fn(cpu)); > > > Ok, we need to do this because "cpu" is not yet set in the > cpu_online_mask. Prior to your patch the "cpu" was getting set in > cpu_l2_cache_map(cpu) as a side-effect of the code that is removed in > the patch. > Right. > > > for_each_cpu(i, cpu_online_mask) { > > /* > > * when updating the marks the current CPU has not been marked > > @@ -1278,29 +1279,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)); Note: Above, we are explicitly setting the cpu_core_mask. > > > > 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) { > > I suppose this "if" condition is an optimization, since if pkg_id != -1, > we anyway set these CPUs in the cpu_core_mask below. > > However... This is not just an optimization. The hunk removed would only work if cpu_l2_cache_mask is bigger than cpu_sibling_mask. (this was the previous assumption that we want to break) If the cpu_sibling_mask is bigger than cpu_l2_cache_mask and pkg_id is -1, then setting only cpu_l2_cache_mask in cpu_core_mask will result in a broken topology. > > > + 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; > > + > > + for_each_cpu(i, mask(cpu)) > > + set_cpus_related(cpu, i, cpu_core_mask); > > > > - if (pkg_id == -1) > > return; > > + } > > > ... since "cpu" is not yet set in the cpu_online_mask, do we not miss setting > "cpu" in the cpu_core_mask(cpu) in the for-loop below ? > > As noted above, we are setting before. So we don't missing the cpu and hence have not different from before. > -- > Thanks and Regards > gautham. -- Thanks and Regards Srikar Dronamraju
next prev parent reply other threads:[~2020-07-22 6:58 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-21 11:38 [PATCH v2 00/10] Coregroup support on Powerpc Srikar Dronamraju 2020-07-21 11:38 ` Srikar Dronamraju 2020-07-21 11:38 ` [PATCH v2 01/10] powerpc/smp: Cache node for reuse Srikar Dronamraju 2020-07-21 11:38 ` Srikar Dronamraju 2020-07-22 7:41 ` Michael Ellerman 2020-07-22 7:41 ` Michael Ellerman 2020-07-22 8:04 ` Srikar Dronamraju 2020-07-22 8:04 ` Srikar Dronamraju 2020-07-21 11:38 ` [PATCH v2 02/10] powerpc/smp: Merge Power9 topology with Power topology Srikar Dronamraju 2020-07-21 11:38 ` Srikar Dronamraju 2020-07-22 5:48 ` Gautham R Shenoy 2020-07-22 5:48 ` Gautham R Shenoy 2020-07-21 11:38 ` [PATCH v2 03/10] powerpc/smp: Move powerpc_topology above Srikar Dronamraju 2020-07-21 11:38 ` Srikar Dronamraju 2020-07-21 11:38 ` [PATCH v2 04/10] powerpc/smp: Enable small core scheduling sooner Srikar Dronamraju 2020-07-21 11:38 ` Srikar Dronamraju 2020-07-22 5:59 ` Gautham R Shenoy 2020-07-22 5:59 ` Gautham R Shenoy 2020-07-22 6:59 ` Srikar Dronamraju 2020-07-22 6:59 ` Srikar Dronamraju 2020-07-21 11:38 ` [PATCH v2 05/10] powerpc/smp: Dont assume l2-cache to be superset of sibling Srikar Dronamraju 2020-07-21 11:38 ` Srikar Dronamraju 2020-07-22 6:21 ` Gautham R Shenoy 2020-07-22 6:21 ` Gautham R Shenoy 2020-07-22 6:57 ` Srikar Dronamraju [this message] 2020-07-22 6:57 ` Srikar Dronamraju 2020-07-24 7:10 ` Gautham R Shenoy 2020-07-24 7:10 ` Gautham R Shenoy 2020-07-21 11:38 ` [PATCH v2 06/10] powerpc/smp: Generalize 2nd sched domain Srikar Dronamraju 2020-07-21 11:38 ` Srikar Dronamraju 2020-07-22 6:56 ` Gautham R Shenoy 2020-07-22 6:56 ` Gautham R Shenoy 2020-07-22 7:39 ` Srikar Dronamraju 2020-07-22 7:39 ` Srikar Dronamraju 2020-07-22 7:46 ` peterz 2020-07-22 7:46 ` peterz 2020-07-22 8:18 ` Srikar Dronamraju 2020-07-22 8:18 ` Srikar Dronamraju 2020-07-22 8:48 ` Peter Zijlstra 2020-07-22 8:48 ` Peter Zijlstra 2020-07-22 8:54 ` peterz 2020-07-22 8:54 ` peterz 2020-07-21 11:38 ` [PATCH v2 07/10] Powerpc/numa: Detect support for coregroup Srikar Dronamraju 2020-07-21 11:38 ` Srikar Dronamraju 2020-07-21 11:38 ` [PATCH v2 08/10] powerpc/smp: Allocate cpumask only after searching thread group Srikar Dronamraju 2020-07-21 11:38 ` Srikar Dronamraju 2020-07-21 11:38 ` [PATCH v2 09/10] Powerpc/smp: Create coregroup domain Srikar Dronamraju 2020-07-21 11:38 ` Srikar Dronamraju 2020-07-22 7:04 ` Gautham R Shenoy 2020-07-22 7:04 ` Gautham R Shenoy 2020-07-22 7:29 ` Srikar Dronamraju 2020-07-22 7:29 ` Srikar Dronamraju 2020-07-21 11:38 ` [PATCH v2 10/10] powerpc/smp: Implement cpu_to_coregroup_id Srikar Dronamraju 2020-07-21 11:38 ` Srikar Dronamraju 2020-07-22 7:06 ` Gautham R Shenoy 2020-07-22 7:06 ` Gautham R Shenoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200722065747.GB9290@linux.vnet.ibm.com \ --to=srikar@linux.vnet.ibm.com \ --cc=anton@au1.ibm.com \ --cc=ego@linux.vnet.ibm.com \ --cc=jniethe5@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=michaele@au1.ibm.com \ --cc=mikey@linux.ibm.com \ --cc=mingo@kernel.org \ --cc=nathanl@linux.ibm.com \ --cc=npiggin@au1.ibm.com \ --cc=oliveroh@au1.ibm.com \ --cc=peterz@infradead.org \ --cc=svaidy@linux.ibm.com \ --cc=valentin.schneider@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.