Re: [RFC PATCH v2 1/2] powerpc: cleanup APIs for cpu/thread/core mappings
* Vaidyanathan Srinivasan sva...@linux.vnet.ibm.com [2010-05-10 11:18:01]: * Paul Mackerras pau...@samba.org [2010-05-10 09:05:22]: On Fri, May 07, 2010 at 05:18:42PM +0530, Vaidyanathan Srinivasan wrote: These APIs take logical cpu number as input Change cpu_first_thread_in_core() to cpu_leftmost_thread_sibling() Change cpu_last_thread_in_core() to cpu_rightmost_thread_sibling() These APIs convert core number (index) to logical cpu/thread numbers Add cpu_first_thread_of_core(int core) Changed cpu_thread_to_core() to cpu_core_of_thread(int cpu) Why make all these changes? The end result doesn't seem any cleaner or better than how it was before, and your patch description doesn't give any reason for us to think yes, we should make this change. I assume you think this is a good change to make, so you need to explain why it's a good idea and convince the rest of us. Sure Paul.. let me explain. The crux of the issue is to make 'threads_per_core' accessible to the pseries_energy module. In the first RFC, I had directly exported the variable which is not a good design. Ben H suggested to make an API around it and then export the function: http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-April/081610.html Instead of making an API to read threads_per_core, as Ben suggested, I have made a wrapper at a higher level to make an API to convert from logical cpu number to core number. The current APIs cpu_first_thread_in_core() and cpu_last_thread_in_core() returns logical CPU number while cpu_thread_to_core() returns core number or index which is not a logical CPU number. Ben recommended to clearly name them to distinguish 'core number' versus first and last 'logical cpu number' in that core. Hence in the new scheme, I have: Change cpu_first_thread_in_core() to cpu_leftmost_thread_sibling() Change cpu_last_thread_in_core() to cpu_rightmost_thread_sibling() which work on logical cpu numbers. While cpu_first_thread_of_core() and cpu_core_of_thread() work on core index. Example usage: (4 threads per core system) cpu_leftmost_thread_sibling(5) = 4 cpu_rightmost_thread_sibling(5) = 7 cpu_core_of_thread(5) = 1 cpu_first_thread_of_core(1) = 4 cpu_core_of_thread() is used in cpu_to_drc_index() in the module and cpu_first_thread_of_core() is used in drc_index_to_cpu() in the module. These APIs may be useful in other modules in future, and the proposed design is a good method to export these APIs to modules. An alternative approach could be to move both the base functions cpu_to_drc_index() and drc_index_to_cpu() into the kernel like in arch/powerpc/kernel/smp.c Thanks for the review, I hope I have explained the requirements and design for this cleanup. Hi Paul and Ben, Do you have any further comments on this patch series and related cleanup? I will post the next iteration in a day or two. Thanks, Vaidy ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH v2 1/2] powerpc: cleanup APIs for cpu/thread/core mappings
On Fri, May 07, 2010 at 05:18:42PM +0530, Vaidyanathan Srinivasan wrote: These APIs take logical cpu number as input Change cpu_first_thread_in_core() to cpu_leftmost_thread_sibling() Change cpu_last_thread_in_core() to cpu_rightmost_thread_sibling() These APIs convert core number (index) to logical cpu/thread numbers Add cpu_first_thread_of_core(int core) Changed cpu_thread_to_core() to cpu_core_of_thread(int cpu) Why make all these changes? The end result doesn't seem any cleaner or better than how it was before, and your patch description doesn't give any reason for us to think yes, we should make this change. I assume you think this is a good change to make, so you need to explain why it's a good idea and convince the rest of us. Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH v2 1/2] powerpc: cleanup APIs for cpu/thread/core mappings
* Paul Mackerras pau...@samba.org [2010-05-10 09:05:22]: On Fri, May 07, 2010 at 05:18:42PM +0530, Vaidyanathan Srinivasan wrote: These APIs take logical cpu number as input Change cpu_first_thread_in_core() to cpu_leftmost_thread_sibling() Change cpu_last_thread_in_core() to cpu_rightmost_thread_sibling() These APIs convert core number (index) to logical cpu/thread numbers Add cpu_first_thread_of_core(int core) Changed cpu_thread_to_core() to cpu_core_of_thread(int cpu) Why make all these changes? The end result doesn't seem any cleaner or better than how it was before, and your patch description doesn't give any reason for us to think yes, we should make this change. I assume you think this is a good change to make, so you need to explain why it's a good idea and convince the rest of us. Sure Paul.. let me explain. The crux of the issue is to make 'threads_per_core' accessible to the pseries_energy module. In the first RFC, I had directly exported the variable which is not a good design. Ben H suggested to make an API around it and then export the function: http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-April/081610.html Instead of making an API to read threads_per_core, as Ben suggested, I have made a wrapper at a higher level to make an API to convert from logical cpu number to core number. The current APIs cpu_first_thread_in_core() and cpu_last_thread_in_core() returns logical CPU number while cpu_thread_to_core() returns core number or index which is not a logical CPU number. Ben recommended to clearly name them to distinguish 'core number' versus first and last 'logical cpu number' in that core. Hence in the new scheme, I have: Change cpu_first_thread_in_core() to cpu_leftmost_thread_sibling() Change cpu_last_thread_in_core() to cpu_rightmost_thread_sibling() which work on logical cpu numbers. While cpu_first_thread_of_core() and cpu_core_of_thread() work on core index. Example usage: (4 threads per core system) cpu_leftmost_thread_sibling(5) = 4 cpu_rightmost_thread_sibling(5) = 7 cpu_core_of_thread(5) = 1 cpu_first_thread_of_core(1) = 4 cpu_core_of_thread() is used in cpu_to_drc_index() in the module and cpu_first_thread_of_core() is used in drc_index_to_cpu() in the module. These APIs may be useful in other modules in future, and the proposed design is a good method to export these APIs to modules. An alternative approach could be to move both the base functions cpu_to_drc_index() and drc_index_to_cpu() into the kernel like in arch/powerpc/kernel/smp.c Thanks for the review, I hope I have explained the requirements and design for this cleanup. --Vaidy ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[RFC PATCH v2 1/2] powerpc: cleanup APIs for cpu/thread/core mappings
These APIs take logical cpu number as input
Change cpu_first_thread_in_core() to cpu_leftmost_thread_sibling()
Change cpu_last_thread_in_core() to cpu_rightmost_thread_sibling()
These APIs convert core number (index) to logical cpu/thread numbers
Add cpu_first_thread_of_core(int core)
Changed cpu_thread_to_core() to cpu_core_of_thread(int cpu)
Made API changes to few callers. Exported symbols for use in modules.
Signed-off-by: Vaidyanathan Srinivasan sva...@linux.vnet.ibm.com
---
arch/powerpc/include/asm/cputhreads.h | 15 +--
arch/powerpc/kernel/smp.c | 17 +++--
arch/powerpc/mm/mmu_context_nohash.c | 12 ++--
3 files changed, 30 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/include/asm/cputhreads.h
b/arch/powerpc/include/asm/cputhreads.h
index a8e1844..26dc6bd 100644
--- a/arch/powerpc/include/asm/cputhreads.h
+++ b/arch/powerpc/include/asm/cputhreads.h
@@ -61,22 +61,25 @@ static inline cpumask_t cpu_online_cores_map(void)
return cpu_thread_mask_to_cores(cpu_online_map);
}
-static inline int cpu_thread_to_core(int cpu)
-{
- return cpu threads_shift;
-}
+#ifdef CONFIG_SMP
+int cpu_core_of_thread(int cpu);
+int cpu_first_thread_of_core(int core);
+#else
+static inline int cpu_core_of_thread(int cpu) { return cpu; }
+static inline int cpu_first_thread_of_core(int core) { return core; }
+#endif
static inline int cpu_thread_in_core(int cpu)
{
return cpu (threads_per_core - 1);
}
-static inline int cpu_first_thread_in_core(int cpu)
+static inline int cpu_leftmost_thread_sibling(int cpu)
{
return cpu ~(threads_per_core - 1);
}
-static inline int cpu_last_thread_in_core(int cpu)
+static inline int cpu_rightmost_thread_sibling(int cpu)
{
return cpu | (threads_per_core - 1);
}
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index c2ee144..02dedff 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -462,6 +462,19 @@ out:
return id;
}
+/* Helper routines for cpu to core mapping */
+int cpu_core_of_thread(int cpu)
+{
+ return cpu threads_shift;
+}
+EXPORT_SYMBOL_GPL(cpu_core_of_thread);
+
+int cpu_first_thread_of_core(int core)
+{
+ return core threads_shift;
+}
+EXPORT_SYMBOL_GPL(cpu_first_thread_of_core);
+
/* Must be called when no change can occur to cpu_present_map,
* i.e. during cpu online or offline.
*/
@@ -513,7 +526,7 @@ int __devinit start_secondary(void *unused)
notify_cpu_starting(cpu);
set_cpu_online(cpu, true);
/* Update sibling maps */
- base = cpu_first_thread_in_core(cpu);
+ base = cpu_leftmost_thread_sibling(cpu);
for (i = 0; i threads_per_core; i++) {
if (cpu_is_offline(base + i))
continue;
@@ -589,7 +602,7 @@ int __cpu_disable(void)
return err;
/* Update sibling maps */
- base = cpu_first_thread_in_core(cpu);
+ base = cpu_leftmost_thread_sibling(cpu);
for (i = 0; i threads_per_core; i++) {
cpu_clear(cpu, per_cpu(cpu_sibling_map, base + i));
cpu_clear(base + i, per_cpu(cpu_sibling_map, cpu));
diff --git a/arch/powerpc/mm/mmu_context_nohash.c
b/arch/powerpc/mm/mmu_context_nohash.c
index 1f2d9ff..7c66e82 100644
--- a/arch/powerpc/mm/mmu_context_nohash.c
+++ b/arch/powerpc/mm/mmu_context_nohash.c
@@ -111,8 +111,8 @@ static unsigned int steal_context_smp(unsigned int id)
* a core map instead but this will do for now.
*/
for_each_cpu(cpu, mm_cpumask(mm)) {
- for (i = cpu_first_thread_in_core(cpu);
-i = cpu_last_thread_in_core(cpu); i++)
+ for (i = cpu_leftmost_thread_sibling(cpu);
+i = cpu_rightmost_thread_sibling(cpu); i++)
__set_bit(id, stale_map[i]);
cpu = i - 1;
}
@@ -264,14 +264,14 @@ void switch_mmu_context(struct mm_struct *prev, struct
mm_struct *next)
*/
if (test_bit(id, stale_map[cpu])) {
pr_hardcont( | stale flush %d [%d..%d],
- id, cpu_first_thread_in_core(cpu),
- cpu_last_thread_in_core(cpu));
+ id, cpu_leftmost_thread_sibling(cpu),
+ cpu_rightmost_thread_sibling(cpu));
local_flush_tlb_mm(next);
/* XXX This clear should ultimately be part of
local_flush_tlb_mm */
- for (i = cpu_first_thread_in_core(cpu);
-i = cpu_last_thread_in_core(cpu); i++) {
+ for (i = cpu_leftmost_thread_sibling(cpu);
+i = cpu_rightmost_thread_sibling(cpu); i++) {
__clear_bit(id, stale_map[i]);
}
}
___
Linuxppc-dev
