Make prof_counter use per-cpu areas patch 1/4 -- x86 arch
PATCH 1/1 Make prof_counter use per-cpu areas patch 1/4 -- x86 arch
Hi,
Here's a patchset to make prof_counter use percpu area infrastructure. Right now, prof_counter is a NR_CPUS array which gets modified every local timer interrupt causing cacheline ping-pong for i386, ppc, x86_64 and sparc arches. Other arches have made this var truly per-cpu.
This one's for i386 (voyager included).
Thanks,
Kiran
arch/i386/kernel/apic.c +8 -6
@@ -52,7 +52,7 @@int prof_multiplier[NR_CPUS ] = {1 , };int prof_old_multiplier[NR_CPUS ] = {1 , }; -int prof_counter[NR_CPUS ] = {1 , }; +DEFINE_PER_CPU (int, prof_counter) =1 ;int get_maxlvt (void ) {
@@ -997,7 +997,7 @@x86_do_profile (regs); -if (--prof_counter[cpu] <=0 ) { +if (--per_cpu (prof_counter, cpu) <=0 ) {/* * The multiplier may have changed since the last time we got * to this point as a result of the user writing to
@@ -1006,10 +1006,12 @@* * Interrupts are already masked off at this point. */ - prof_counter[cpu] = prof_multiplier[cpu]; -if (prof_counter[cpu] != prof_old_multiplier[cpu]) { -__setup_APIC_LVTT (calibration_result/prof_counter[cpu]); - prof_old_multiplier[cpu] = prof_counter[cpu]; +per_cpu (prof_counter, cpu) = prof_multiplier[cpu]; +if (per_cpu (prof_counter, cpu) != prof_old_multiplier[cpu]) { +__setup_APIC_LVTT ( + calibration_result/ +per_cpu (prof_counter, cpu)); + prof_old_multiplier[cpu] =per_cpu (prof_counter, cpu); }#ifdef CONFIG_SMP
arch/i386/kernel/smpboot.c +2 -2
@@ -937,7 +937,7 @@extern int prof_multiplier[NR_CPUS ];extern int prof_old_multiplier[NR_CPUS ]; -extern int prof_counter[NR_CPUS ]; +DECLARE_PER_CPU (int, prof_counter);static int boot_cpu_logical_apicid;/* Where the IO area was mapped on multiquad, always 0 otherwise */
@@ -955,7 +955,7 @@*/ for (cpu =0 ; cpu <NR_CPUS ; cpu++) { - prof_counter[cpu] =1 ; +per_cpu (prof_counter, cpu) =1 ; prof_old_multiplier[cpu] =1 ; prof_multiplier[cpu] =1 ; }
arch/i386/mach-voyager/voyager_smp.c +6 -6
@@ -236,7 +236,7 @@/* The per cpu profile stuff - used in smp_local_timer_interrupt */ static unsigned int prof_multiplier[NR_CPUS] __cacheline_aligned = { 1, }; static unsigned int prof_old_multiplier[NR_CPUS] __cacheline_aligned = { 1, }; -static unsigned int prof_counter[NR_CPUS] __cacheline_aligned = { 1, }; +static DEFINE_PER_CPU(unsigned int, prof_counter) = 1;/* the map used to check if a CPU has booted */ static __u32 cpu_booted_map;
@@ -393,7 +393,7 @@/* initialize the CPU structures (moved from smp_boot_cpus) */ for (i=0 ; i<NR_CPUS ; i++) { - prof_counter[i] =1 ; +per_cpu (prof_counter, i) =1 ; prof_old_multiplier[i] =1 ; prof_multiplier[i] =1 ; cpu_irq_affinity[i] = ~0 ;
@@ -1312,7 +1312,7 @@x86_do_profile (regs); -if (--prof_counter[cpu] <=0 ) { +if (--per_cpu (prof_counter, cpu) <=0 ) {/* * The multiplier may have changed since the last time we got * to this point as a result of the user writing to
@@ -1321,10 +1321,10 @@* * Interrupts are already masked off at this point. */ - prof_counter[cpu] = prof_multiplier[cpu]; -if (prof_counter[cpu] != prof_old_multiplier[cpu]) { +per_cpu (prof_counter,cpu) = prof_multiplier[cpu]; +if (per_cpu (prof_counter, cpu) != prof_old_multiplier[cpu]) {/* FIXME: need to update the vic timer tick here */ - prof_old_multiplier[cpu] = prof_counter[cpu]; + prof_old_multiplier[cpu] =per_cpu (prof_counter, cpu); }update_process_times (user_mode (regs));
Re: [patch] Make prof_counter use per-cpu areas patch 2/4 -- ppc arch
This one's for ppc.
arch/ppc/kernel/smp.c +3 -3
@@ -45,7 +45,7 @@atomic_t ipi_recv;atomic_t ipi_sent;unsigned int prof_multiplier[NR_CPUS ] = { [1 ...NR_CPUS -1 ] =1 }; -unsigned int prof_counter[NR_CPUS ] = { [1 ...NR_CPUS -1 ] =1 }; +DEFINE_PER_CPU(unsigned int, prof_counter) = 1;unsigned long cache_decay_ticks =HZ /100 ;unsigned long cpu_online_map =1UL ;unsigned long cpu_possible_map =1UL ;
@@ -90,9 +90,9 @@ {int cpu =smp_processor_id (); -if (!--prof_counter[cpu]) { +if (!--per_cpu (prof_counter, cpu)) {update_process_times (user_mode (regs)); - prof_counter[cpu]=prof_multiplier[cpu]; +per_cpu (prof_counter, cpu)=prof_multiplier[cpu]; } }
Re: [patch] Make prof_counter use per-cpu areas patch 2/4 -- ppc arch
Quoted 1 line · Ravikiran G Thirumalai
This one's for ppc.
[snip]
arch/ppc/kernel/smp.c line 48
Quoted 2 lines
arch/ppc/kernel/smp.c +1
-1-unsigned int prof_counter[NR_CPUS ] = { [1 ...NR_CPUS -1 ] =1 }; +DEFINE_PER_CPU(unsigned int, prof_counter) = 1;
I had already done something similar locally which I'll push to Linus shortly.
Thanks,
Paul.
Re: [patch] Make prof_counter use per-cpu areas patch 3/4 -- x86_64 arch
In reply to Ravikiran G ThirumalaiThis one's for x86_64
arch/x86_64/kernel/apic.c +7 -6
@@ -39,7 +39,7 @@int prof_multiplier[NR_CPUS ] = {1 , };int prof_old_multiplier[NR_CPUS ] = {1 , }; -int prof_counter[NR_CPUS ] = {1 , }; +DEFINE_PER_CPU (int, prof_counter) =1 ;int get_maxlvt (void ) {
@@ -901,7 +901,7 @@x86_do_profile (regs); -if (--prof_counter[cpu] <=0 ) { +if (--per_cpu (prof_counter, cpu) <=0 ) {/* * The multiplier may have changed since the last time we got * to this point as a result of the user writing to
@@ -910,10 +910,11 @@* * Interrupts are already masked off at this point. */ - prof_counter[cpu] = prof_multiplier[cpu]; -if (prof_counter[cpu] != prof_old_multiplier[cpu]) { -__setup_APIC_LVTT (calibration_result/prof_counter[cpu]); - prof_old_multiplier[cpu] = prof_counter[cpu]; +per_cpu (prof_counter, cpu) = prof_multiplier[cpu]; +if (per_cpu (prof_counter, cpu) != prof_old_multiplier[cpu]) { +__setup_APIC_LVTT (calibration_result/ +per_cpu (prof_counter, cpu)); + prof_old_multiplier[cpu] =per_cpu (prof_counter, cpu); }#ifdef CONFIG_SMP
arch/x86_64/kernel/smpboot.c +2 -2
@@ -774,7 +774,7 @@extern int prof_multiplier[NR_CPUS ];extern int prof_old_multiplier[NR_CPUS ]; -extern int prof_counter[NR_CPUS ]; +DECLARE_PER_CPU (int, prof_counter);static void __init smp_boot_cpus (unsigned int max_cpus) {
@@ -787,7 +787,7 @@for (apicid =0 ; apicid <NR_CPUS ; apicid++) { x86_apicid_to_cpu[apicid] =-1 ; - prof_counter[apicid] =1 ; +per_cpu (prof_counter, apicid) =1 ; prof_old_multiplier[apicid] =1 ; prof_multiplier[apicid] =1 ; }
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [patch] Make prof_counter use per-cpu areas patch 3/4 -- x86_64 arch
Quoted 6 lines · Andi Kleen
Quoted 1 line · Ravikiran G Thirumalai
This one's for
x86_64Thanks, applied.
-Andi
Please apply this over the earlier patch. As Andrew pointed out, the above patch will cause crashes when per-cpu areas are modified to allocate for cpu_possible cpus only.
Reinit of prof_counter/prof_multiplier/prof_old_multiplier seems to be redundant (They are already statically inited). Similar patch worked for x86 for me.
Thanks,
Kiran
arch/x86_64/kernel/smpboot.c +0 -8
@@ -772,24 +772,16 @@* Cycle through the processors sending APIC IPIs to boot each. */ -extern int prof_multiplier[NR_CPUS ]; -extern int prof_old_multiplier[NR_CPUS ]; -DECLARE_PER_CPU (int, prof_counter); -static void __init smp_boot_cpus (unsigned int max_cpus) {int apicid, cpu;/* * Initialize the logical to physical CPU number mapping -* and the per-CPU profiling counter/multiplier */ for (apicid =0 ; apicid <NR_CPUS ; apicid++) { x86_apicid_to_cpu[apicid] =-1 ; -per_cpu (prof_counter, apicid) =1 ; - prof_old_multiplier[apicid] =1 ; - prof_multiplier[apicid] =1 ; }/*
Re: [patch] Make prof_counter use per-cpu areas patch 4/4 -- sparc arch
In reply to Ravikiran G ThirumalaiThis one's for sparc
arch/sparc/kernel/smp.c +1 -1
@@ -256,7 +256,7 @@ }unsigned int prof_multiplier[NR_CPUS ]; -unsigned int prof_counter[NR_CPUS ]; +DEFINE_PER_CPU(unsigned int, prof_counter);extern unsigned int lvl14_resolution;int setup_profiling_timer (unsigned int multiplier)
arch/sparc/kernel/sun4d_smp.c +4 -4
@@ -431,7 +431,7 @@ }extern unsigned int prof_multiplier[NR_CPUS ]; -extern unsigned int prof_counter[NR_CPUS ]; +DECLARE_PER_CPU(unsigned int, prof_counter);extern void sparc_do_profile (unsigned long pc,unsigned long o7);
@@ -455,14 +455,14 @@if (!user_mode (regs))sparc_do_profile (regs->pc, regs->u_regs[UREG_RETPC ]); -if (!--prof_counter[cpu]) { +if (!--per_cpu (prof_counter, cpu)) {int user =user_mode (regs);irq_enter ();update_process_times (user);irq_exit (); - prof_counter[cpu] = prof_multiplier[cpu]; +per_cpu (prof_counter, cpu) = prof_multiplier[cpu]; } }
@@ -472,7 +472,7 @@ {int cpu =hard_smp4d_processor_id (); - prof_counter[cpu] = prof_multiplier[cpu] =1 ; +per_cpu (prof_counter, cpu) = prof_multiplier[cpu] =1 ;load_profile_irq (cpu, lvl14_resolution); }
Re: [patch] Make prof_counter use per-cpu areas patch 4/4 -- sparc arch
arch/sparc/kernel/smp.c line 259
Quoted 6 lines
Date: Mon, 13 Jan 2003 18:08:25 +0530
From: Ravikiran G Thirumalai <[email protected]>This one's for sparc
arch/sparc/kernel/smp.c +1
-1-unsigned int prof_counter[NR_CPUS ]; +DEFINE_PER_CPU(unsigned int, prof_counter);
Thanks. I'll apply at next pull, or Dave will.
-- Pete
Re: [patch] Make prof_counter use per-cpu areas patch 4/4 -- sparc arch
From: Pete Zaitcev <[email protected]>
Date: Mon, 13 Jan 2003 11:49:08 -0500
Thanks. I'll apply at next pull, or Dave will.
Pete, I'll take it from you at your leisure.
Review
Quoted 8 lines · Ravikiran G Thirumalai
Hi,
Here's a patchset to makeprof_counteruse percpu area infrastructure.
...(quoted diff) +1
-1/* initialize the CPU structures (moved from smp_boot_cpus) */ for(i=0; i<NR_CPUS; i++) { - prof_counter[i] = 1; + per_cpu(prof_counter, i) = 1;
Please always use the cpu_online() test here.
Yes, it's a bit awkward and yes, we should have a for_each_online_cpu() helper, but that didn't happen.
The reason (apart from increased efficiency) is that at some time we may make the per-cpu data areas only exist on online CPUs. We allocate the area from the CPU's node-local memory when it is coming online.
Code such as the above will oops with that change in place.
We did have all of this running, but I never submitted the patch for reasons of general scariness and lack of expressed interest from NUMA people.
<dig, dig>
Here it is:
The current per-cpu memory areas are allocated at startup for NR_CPUS, so we're really not saving much memory from them.
This is a problem for kernel/timer.c (128 kbytes), kernel/sched.c (78 kbytes) and presumably other places.
The patch from Rutsy allocates per-cpu areas only for those CPUs which may actually exist, before each one comes online.
So the per-cpu storage for not-possible CPUs does not exist, and accessing them will oops.
init/main.c | 40 ++++++++++++++++++++++++++++------------ 1 files changed, 28 insertions(+), 12 deletions(-)
init/main.c renamed from init/main.c~per-cpu-allocation +28 -12
@@ -304,32 +304,39 @@ static void __init smp_init(void)#define smp_init () do { } while (0)#endif -static inline void setup_per_cpu_areas (void ) { } +static inline void setup_per_cpu_area (unsigned int cpu) { }static inline void smp_prepare_cpus (unsigned int maxcpus) { }#else #ifdef __GENERIC_PER_CPU +/* Created by linker magic */ +extern char __per_cpu_start[], __per_cpu_end[]; +unsigned long __per_cpu_offset[NR_CPUS ]; -static void __init setup_per_cpu_areas (void ) +/* Sets up per-cpu area for boot CPU. */ +static void __init setup_per_cpu_area (unsigned int cpu) { -unsigned long size, i; +unsigned long size;char *ptr; -/* Created by linker magic */ -extern char __per_cpu_start[], __per_cpu_end[];/* Copy section for each CPU (we discard the original) */ size =ALIGN (__per_cpu_end - __per_cpu_start,SMP_CACHE_BYTES );if (!size)return ; - ptr =alloc_bootmem (size *NR_CPUS ); +/* First CPU happens really early... */ +if (cpu ==smp_processor_id ()) + ptr =alloc_bootmem (size); +else + ptr =kmalloc (size,GFP_ATOMIC ); -for (i =0 ; i <NR_CPUS ; i++, ptr += size) { - __per_cpu_offset[i] = ptr - __per_cpu_start; -memcpy (ptr, __per_cpu_start, size); - } +if (ptr ==NULL ) +panic ("failed to allocate per-cpu area for cpu %d\n" , cpu); + + __per_cpu_offset[cpu] = ptr - __per_cpu_start; +memcpy (ptr, __per_cpu_start, size); }#endif /* !__GENERIC_PER_CPU */
@@ -338,7 +345,16 @@ static void __init smp_init(void) {unsigned int i; -/* FIXME: This should be done in userspace --RR */ +for (i =0 ; i <NR_CPUS ; i++) { +if (cpu_possible (i)) { +if (i !=smp_processor_id ()) +setup_per_cpu_area (i); + }else { +/* Force a NULL deref on use */ + __per_cpu_offset[i] = (char *)0 - __per_cpu_start; + } + } +for (i =0 ; i <NR_CPUS ; i++) {if (num_online_cpus () >= max_cpus)break ;
@@ -390,7 +406,7 @@ asmlinkage void __init start_kernel(voidlock_kernel ();printk (linux_banner);setup_arch (&command_line); -setup_per_cpu_areas (); +setup_per_cpu_area (smp_processor_id ());build_all_zonelists ();printk ("Kernel command line: %s\n" , saved_command_line);parse_options (command_line);
.
Quoted 12 lines · Andrew Morton
Review
Quoted 8 lines · Ravikiran G Thirumalai
Hi,
Here's a patchset to makeprof_counteruse percpu area infrastructure.
...(quoted diff) +1
-1/* initialize the CPU structures (moved from smp_boot_cpus) */ for(i=0; i<NR_CPUS; i++) { - prof_counter[i] = 1; + per_cpu(prof_counter, i) = 1;Please always use the
cpu_online()test here.
Even cpu_possible does not seem to be setup this early. Seems like reinitialisation of prof_counter/prof_multiplier et al is redundant. Here's a newer patch which removes this initialisation at smp_boot_cpus. Works fine for me (tested same on a 4 way with difft profiling multipliers.. LOC interrupts seem to fire at the right intervals).
Only x86 and x86_64 arches had this reinits. Here's the corrected x86 patch. x86_64 patch to follow
Thanks,
Kiran
arch/i386/kernel/apic.c +8 -6
@@ -52,7 +52,7 @@int prof_multiplier[NR_CPUS ] = {1 , };int prof_old_multiplier[NR_CPUS ] = {1 , }; -int prof_counter[NR_CPUS ] = {1 , }; +DEFINE_PER_CPU (int, prof_counter) =1 ;int get_maxlvt (void ) {
@@ -997,7 +997,7 @@x86_do_profile (regs); -if (--prof_counter[cpu] <=0 ) { +if (--per_cpu (prof_counter, cpu) <=0 ) {/* * The multiplier may have changed since the last time we got * to this point as a result of the user writing to
@@ -1006,10 +1006,12 @@* * Interrupts are already masked off at this point. */ - prof_counter[cpu] = prof_multiplier[cpu]; -if (prof_counter[cpu] != prof_old_multiplier[cpu]) { -__setup_APIC_LVTT (calibration_result/prof_counter[cpu]); - prof_old_multiplier[cpu] = prof_counter[cpu]; +per_cpu (prof_counter, cpu) = prof_multiplier[cpu]; +if (per_cpu (prof_counter, cpu) != prof_old_multiplier[cpu]) { +__setup_APIC_LVTT ( + calibration_result/ +per_cpu (prof_counter, cpu)); + prof_old_multiplier[cpu] =per_cpu (prof_counter, cpu); }#ifdef CONFIG_SMP
arch/i386/kernel/smpboot.c +0 -15
@@ -935,10 +935,6 @@* Cycle through the processors sending APIC IPIs to boot each. */ -extern int prof_multiplier[NR_CPUS ]; -extern int prof_old_multiplier[NR_CPUS ]; -extern int prof_counter[NR_CPUS ]; -static int boot_cpu_logical_apicid;/* Where the IO area was mapped on multiquad, always 0 otherwise */ void *xquad_portio;
@@ -950,17 +946,6 @@int apicid, cpu, bit;/* -* Initialize the logical to physical CPU number mapping -* and the per-CPU profiling counter/multiplier -*/ - -for (cpu =0 ; cpu <NR_CPUS ; cpu++) { - prof_counter[cpu] =1 ; - prof_old_multiplier[cpu] =1 ; - prof_multiplier[cpu] =1 ; - } - -/* * Setup boot CPU information */ smp_store_cpu_info (0 );/* Final full version of the data */
arch/i386/mach-voyager/voyager_smp.c +5 -8
@@ -236,7 +236,7 @@/* The per cpu profile stuff - used in smp_local_timer_interrupt */ static unsigned int prof_multiplier[NR_CPUS] __cacheline_aligned = { 1, }; static unsigned int prof_old_multiplier[NR_CPUS] __cacheline_aligned = { 1, }; -static unsigned int prof_counter[NR_CPUS] __cacheline_aligned = { 1, }; +static DEFINE_PER_CPU(unsigned int, prof_counter) = 1;/* the map used to check if a CPU has booted */ static __u32 cpu_booted_map;
@@ -393,9 +393,6 @@/* initialize the CPU structures (moved from smp_boot_cpus) */ for (i=0 ; i<NR_CPUS ; i++) { - prof_counter[i] =1 ; - prof_old_multiplier[i] =1 ; - prof_multiplier[i] =1 ; cpu_irq_affinity[i] = ~0 ; } cpu_online_map = (1 <<boot_cpu_id);
@@ -1312,7 +1309,7 @@x86_do_profile (regs); -if (--prof_counter[cpu] <=0 ) { +if (--per_cpu (prof_counter, cpu) <=0 ) {/* * The multiplier may have changed since the last time we got * to this point as a result of the user writing to
@@ -1321,10 +1318,10 @@* * Interrupts are already masked off at this point. */ - prof_counter[cpu] = prof_multiplier[cpu]; -if (prof_counter[cpu] != prof_old_multiplier[cpu]) { +per_cpu (prof_counter,cpu) = prof_multiplier[cpu]; +if (per_cpu (prof_counter, cpu) != prof_old_multiplier[cpu]) {/* FIXME: need to update the vic timer tick here */ - prof_old_multiplier[cpu] = prof_counter[cpu]; + prof_old_multiplier[cpu] =per_cpu (prof_counter, cpu); }update_process_times (user_mode (regs));
Quoted 6 lines · Ravikiran G Thirumalai
Even
cpu_possibledoes not seem to be setup this early. Seems like reinitialisation of prof_counter/prof_multiplier et al is redundant. Here's a newer patch which removes this initialisation atsmp_boot_cpus. Works fine for me (tested same on a 4 way with difft profiling multipliers.. LOC interrupts seem to fire at the right intervals).
Things still look a bit fishy to me.
In apic.c:
int prof_multiplier[NR_CPUS] = { 1, };
int prof_old_multiplier[NR_CPUS] = { 1, };
DEFINE_PER_CPU(int, prof_counter) = 1;This means that all the prof_counter values are set to 1, but the multiplier arrays have 1 in the zeroeth entry, and zero in the remaining entries.
The zero multipliers remain in place until someone runs setup_profiling_timer().
One approach would be to initialise all members:
int prof_multiplier[NR_CPUS] = { [ 0 ... NR_CPUS-1 ] = 1 };But I think it would be better to put the multipliers into per-cpu memory as well. Something like:
struct profiling_info {
int multiplier;
int old_multiplier;
int counter;
};DEFINE_PER_CPU(struct profiling_info, profiling_info) = {1, 1, 1};
Perhaps?
Also bits and pieces of the profiling code seem to be randomly splattered all over the place. Consolidating it all into the one .c file would be nice.