| Message ID | 1426999379-20542-1-git-send-email-shreyas@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e602ffb2fa6b5533acd6847bd19a135e14ed4c54 |
| Delegated to: | Michael Ellerman |
| Headers | show |
On Sun, 2015-22-03 at 04:42:57 UTC, "Shreyas B. Prabhu" wrote: > Currently, cpu_online_cores_map returns a mask, which for every core > that has atleast one online thread, has the first-cpu-of-that-core's bit > set. ... which for every core with at least one online thread, has the bit for thread 0 of the core set to 1, and the bits for all other threads of the core set to 0. Maybe that's clearer? > But the first cpu itself may not be online always. In such cases, if ^ of the core > the returned mask is used for IPI, then it'll cause IPIs to be skipped > on cores where the first thread is offline. .. because the IPI code refuses to send IPIs to offline threads, right? > Fix this by setting first-online-cpu-of-the-core's bit in the mask. .. by setting the bit of the first online thread in the core. > This is done by fixing this in the underlying function > cpu_thread_mask_to_cores. The result has the property that for all cores with online threads, there is one bit set in the returned map. And further, all bits that are set in the returned map correspond to online threads. > Signed-off-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com> > --- > This patch is new in v3 > > In an example scenario where all the threads of 1st core are offline > and argument to cpu_thread_mask_to_cores is cpu_possible_mask, > with this implementation, return value will not have any bit > corresponding to 1st core set. I think that should be okay. Any thoughts? Looking at linux-next: $ git grep cpu_thread_mask_to_cores arch/powerpc/include/asm/cputhreads.h:/* cpu_thread_mask_to_cores - Return a cpumask of one per cores arch/powerpc/include/asm/cputhreads.h:static inline cpumask_t cpu_thread_mask_to_cores(const struct cpumask *threads) arch/powerpc/include/asm/cputhreads.h: return cpu_thread_mask_to_cores(cpu_online_mask); $ git grep cpu_online_cores_map arch/powerpc/include/asm/cputhreads.h:static inline cpumask_t cpu_online_cores_map(void) ie. There are no users. So yeah I think we can change the semantics of this, and the semantics you describe make sense. If you agree with my changelog comments I'm happy to fix that up and merge this, or you can send a v4 if you like. cheers
On Monday 30 March 2015 03:06 PM, Michael Ellerman wrote: > On Sun, 2015-22-03 at 04:42:57 UTC, "Shreyas B. Prabhu" wrote: >> Currently, cpu_online_cores_map returns a mask, which for every core >> that has atleast one online thread, has the first-cpu-of-that-core's bit >> set. > > ... which for every core with at least one online thread, has the bit for > thread 0 of the core set to 1, and the bits for all other threads of the core > set to 0. > > Maybe that's clearer? > >> But the first cpu itself may not be online always. In such cases, if > ^ > of the core > >> the returned mask is used for IPI, then it'll cause IPIs to be skipped >> on cores where the first thread is offline. > > .. because the IPI code refuses to send IPIs to offline threads, right? Yes. > >> Fix this by setting first-online-cpu-of-the-core's bit in the mask. > > .. by setting the bit of the first online thread in the core. > >> This is done by fixing this in the underlying function >> cpu_thread_mask_to_cores. > > > The result has the property that for all cores with online threads, there is > one bit set in the returned map. And further, all bits that are set in the > returned map correspond to online threads. > > >> Signed-off-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com> >> --- >> This patch is new in v3 >> >> In an example scenario where all the threads of 1st core are offline >> and argument to cpu_thread_mask_to_cores is cpu_possible_mask, >> with this implementation, return value will not have any bit >> corresponding to 1st core set. I think that should be okay. Any thoughts? > > Looking at linux-next: > > $ git grep cpu_thread_mask_to_cores > arch/powerpc/include/asm/cputhreads.h:/* cpu_thread_mask_to_cores - Return a cpumask of one per cores > arch/powerpc/include/asm/cputhreads.h:static inline cpumask_t cpu_thread_mask_to_cores(const struct cpumask *threads) > arch/powerpc/include/asm/cputhreads.h: return cpu_thread_mask_to_cores(cpu_online_mask); > $ git grep cpu_online_cores_map > arch/powerpc/include/asm/cputhreads.h:static inline cpumask_t cpu_online_cores_map(void) > > ie. There are no users. > > So yeah I think we can change the semantics of this, and the semantics you > describe make sense. > > If you agree with my changelog comments I'm happy to fix that up and merge > this, or you can send a v4 if you like. > I'll fix the changelog in v4. > cheers >
diff --git a/arch/powerpc/include/asm/cputhreads.h b/arch/powerpc/include/asm/cputhreads.h index 2bf8e93..9e8485c 100644 --- a/arch/powerpc/include/asm/cputhreads.h +++ b/arch/powerpc/include/asm/cputhreads.h @@ -31,9 +31,9 @@ extern cpumask_t threads_core_mask; /* cpu_thread_mask_to_cores - Return a cpumask of one per cores * hit by the argument * - * @threads: a cpumask of threads + * @threads: a cpumask of online threads * - * This function returns a cpumask which will have one "cpu" (or thread) + * This function returns a cpumask which will have one online cpu's * bit set for each core that has at least one thread set in the argument. * * This can typically be used for things like IPI for tlb invalidations @@ -42,13 +42,16 @@ extern cpumask_t threads_core_mask; static inline cpumask_t cpu_thread_mask_to_cores(const struct cpumask *threads) { cpumask_t tmp, res; - int i; + int i, cpu; cpumask_clear(&res); for (i = 0; i < NR_CPUS; i += threads_per_core) { cpumask_shift_left(&tmp, &threads_core_mask, i); - if (cpumask_intersects(threads, &tmp)) - cpumask_set_cpu(i, &res); + if (cpumask_intersects(threads, &tmp)) { + cpu = cpumask_next_and(-1, &tmp, cpu_online_mask); + if (cpu < nr_cpu_ids) + cpumask_set_cpu(cpu, &res); + } } return res; }
Currently, cpu_online_cores_map returns a mask, which for every core that has atleast one online thread, has the first-cpu-of-that-core's bit set. But the first cpu itself may not be online always. In such cases, if the returned mask is used for IPI, then it'll cause IPIs to be skipped on cores where the first thread is offline. Fix this by setting first-online-cpu-of-the-core's bit in the mask. This is done by fixing this in the underlying function cpu_thread_mask_to_cores. Signed-off-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com> --- This patch is new in v3 In an example scenario where all the threads of 1st core are offline and argument to cpu_thread_mask_to_cores is cpu_possible_mask, with this implementation, return value will not have any bit corresponding to 1st core set. I think that should be okay. Any thoughts? arch/powerpc/include/asm/cputhreads.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)