Subject: Bug fixes for OProfile for Cell From: Maynard Johnson This patch contains three crucial fixes: - op_model_cell.c:cell_virtual_cntr: correctly access the per-cpu pmc_values arrays - op_model_cell.c:cell_reg_setup: initialize _all_ 4 elements of pmc_values with reset_value - include/asm-powerpc/cell-pmu.h: fix broken macro, PM07_CTR_INPUT_MUX Signed-off-by: Carl Love Signed-off-by: Maynard Johnson Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/oprofile/op_model_cell.c =================================================================== --- linux-2.6.19-rc6-arnd1+patches.orig/arch/powerpc/oprofile/op_model_cell.c 2006-11-20 10:23:38.520401976 -0600 +++ linux-2.6.19-rc6-arnd1+patches/arch/powerpc/oprofile/op_model_cell.c 2006-11-20 10:26:49.912328712 -0600 @@ -102,20 +102,20 @@ #define GET_INPUT_CONTROL(x) ((x & 0x00000004) >> 2) -DEFINE_PER_CPU(unsigned long[NR_PHYS_CTRS], pmc_values); +static DEFINE_PER_CPU(unsigned long[NR_PHYS_CTRS], pmc_values); -struct pmc_cntrl_data pmc_cntrl[NUM_THREADS][NR_PHYS_CTRS]; +static struct pmc_cntrl_data pmc_cntrl[NUM_THREADS][NR_PHYS_CTRS]; /* Interpetation of hdw_thread: * 0 - even virtual cpus 0, 2, 4,... * 1 - odd virtual cpus 1, 3, 5, ... */ -static u32 hdw_thread; +static u32 hdw_thread; static u32 virt_cntr_inter_mask; static struct timer_list timer_virt_cntr; -/* pm_signal needs to be glogal since it is initialized in +/* pm_signal needs to be global since it is initialized in * cell_reg_setup at the time when the necessary information * is available. */ @@ -178,7 +178,7 @@ { int ret; int j; - struct pm_signal pm_signal_local[NR_PHYS_CTRS]; + struct pm_signal pm_signal_local[NR_PHYS_CTRS]; for (j = 0; j < count; j++) { /* fw expects physical cpu # */ @@ -278,7 +278,7 @@ ; } -static void write_pm_cntrl(int cpu, struct pm_cntrl * pm_cntrl) +static void write_pm_cntrl(int cpu, struct pm_cntrl *pm_cntrl) { /* Oprofile will use 32 bit counters, set bits 7:10 to 0 */ u32 val = 0; @@ -328,36 +328,48 @@ } /* - * Oprofile setup functions + * Oprofile is expected to collect data on all CPUs simultaneously. + * However, there is one set of performance counters per node. There are + * two hardware threads or virtual CPUs on each node. Hence, OProfile must + * multiplex in time the performance counter collection on the two virtual + * CPUs. The multiplexing of the performance counters is done by this + * virtual counter routine. + * + * The pmc_values used below is defined as 'per-cpu' but its use is + * more akin to 'per-node'. We need to store two sets of counter + * values per node -- one for the previous run and one for the next. + * The per-cpu[NR_PHYS_CTRS] gives us the storage we need. Each odd/even + * pair of per-cpu arrays is used for storing the previous and next + * pmc values for a given node. + * NOTE: We use the per-cpu variable to improve cache performance. */ static void cell_virtual_cntr(unsigned long data) { /* This routine will alternate loading the virtual counters for * virtual CPUs */ - int i, cntr_offset_prev, cntr_offset_next, num_enabled; + int i, prev_hdw_thread, next_hdw_thread; u32 cpu; unsigned long flags; /* Make sure that the interrupt_hander and * the virt counter are not both playing with - * the counters on the same node. */ + * the counters on the same node. + */ spin_lock_irqsave(&virt_cntr_lock, flags); - /* cntr offset for the cntrs that were running */ - cntr_offset_prev = num_counters * hdw_thread; + prev_hdw_thread = hdw_thread; - /* switch the cpu handling the interrupts */ - hdw_thread = 1 ^ hdw_thread; + /* switch the cpu handling the interrupts */ + hdw_thread = 1 ^ hdw_thread; + next_hdw_thread = hdw_thread; - /* cntr offset for the cntrs to start */ - cntr_offset_next = num_counters * hdw_thread; /* The following is done only once per each node, but * we need cpu #, not node #, to pass to the cbe_xxx functions. */ for_each_online_cpu(cpu) { - if (cbe_get_hw_thread_id(cpu)) + if (cbe_get_hw_thread_id(cpu)) continue; /* stop counters, save counter values, restore counts @@ -366,55 +378,57 @@ cbe_disable_pm(cpu); cbe_disable_pm_interrupts(cpu); for (i = 0; i < num_counters; i++) { - per_cpu(pmc_values, cpu)[i + cntr_offset_prev] - = cbe_read_ctr(cpu, i); + per_cpu(pmc_values, cpu + prev_hdw_thread)[i] + = cbe_read_ctr(cpu, i); - if (per_cpu(pmc_values, cpu)[i+cntr_offset_next] == \ - 0xFFFFFFFF) - /* If the cntr value is 0xffffffff, we must - * reset that to 0xfffffff0 when the current - * thread is restarted. This will generate a new - * interrupt and make sure that we never restore - * the counters to the max value. If the counters - * were restored to the max value, they do not - * increment and no interrupts are generated. Hence - * no more samples will be collected on that cpu. - */ + if (per_cpu(pmc_values, cpu + next_hdw_thread)[i] + == 0xFFFFFFFF) + /* If the cntr value is 0xffffffff, we must + * reset that to 0xfffffff0 when the current + * thread is restarted. This will generate a new + * interrupt and make sure that we never restore + * the counters to the max value. If the counters + * were restored to the max value, they do not + * increment and no interrupts are generated. Hence + * no more samples will be collected on that cpu. + */ cbe_write_ctr(cpu, i, 0xFFFFFFF0); else cbe_write_ctr(cpu, i, - per_cpu(pmc_values, - cpu)[i+cntr_offset_next]); + per_cpu(pmc_values, + cpu + + next_hdw_thread)[i]); } /* Switch to the other thread. Change the interrupt * and control regs to be scheduled on the CPU * corresponding to the thread to execute. */ - num_enabled = 0; - for (i = 0; i < num_counters; i++) { - if (pmc_cntrl[hdw_thread][i+cntr_offset_next].enabled) { - set_pm_event(num_enabled, - pmc_cntrl[hdw_thread][i].enabled, - pmc_cntrl[hdw_thread][i].masks); - - num_enabled++; - ctr_enabled |= (1 << i); + if (pmc_cntrl[next_hdw_thread][i].enabled) { + /* There are some per thread events. + * Must do the set event, enable_cntr + * for each cpu. + */ + set_pm_event(i, + pmc_cntrl[next_hdw_thread][i].evnts, + pmc_cntrl[next_hdw_thread][i].masks); + enable_ctr(cpu, i, + pm_regs.pm07_cntrl); + } else { + cbe_write_pm07_control(cpu, i, 0); } } /* Enable interrupts on the CPU thread that is starting */ - cbe_enable_pm_interrupts(cpu, hdw_thread, + cbe_enable_pm_interrupts(cpu, next_hdw_thread, virt_cntr_inter_mask); cbe_enable_pm(cpu); } spin_unlock_irqrestore(&virt_cntr_lock, flags); - mod_timer(&timer_virt_cntr, jiffies + HZ/10); - - return; + mod_timer(&timer_virt_cntr, jiffies + HZ / 10); } static void start_virt_cntrs(void) @@ -422,7 +436,7 @@ init_timer(&timer_virt_cntr); timer_virt_cntr.function = cell_virtual_cntr; timer_virt_cntr.data = 0UL; - timer_virt_cntr.expires = jiffies + HZ/10; + timer_virt_cntr.expires = jiffies + HZ / 10; add_timer(&timer_virt_cntr); } @@ -431,8 +445,7 @@ cell_reg_setup(struct op_counter_config *ctr, struct op_system_config *sys, int num_ctrs) { - int i, j; - int num_enabled = 0; + int i, j, cpu; pm_rtas_token = rtas_token("ibm,cbe-perftools"); if (pm_rtas_token == RTAS_UNKNOWN_SERVICE) { @@ -471,7 +484,6 @@ * equivalent thread 1 event. */ for (i = 0; i < num_ctrs; ++i) { - if ((ctr[i].event >= 2100) && (ctr[i].event <= 2111)) pmc_cntrl[1][i].evnts = ctr[i].event + 19; else if (ctr[i].event == 2203) @@ -500,18 +512,23 @@ */ for (i = 0; i < num_counters; ++i) { /* start with virtual counter set 0 */ - if (pmc_cntrl[0][i].enabled) { /* Using 32bit counters, reset max - count */ reset_value[i] = 0xFFFFFFFF - ctr[i].count; - set_pm_event(num_enabled, + set_pm_event(i, pmc_cntrl[0][i].evnts, pmc_cntrl[0][i].masks); - num_enabled++; - ctr_enabled |= (1 << i); + + /* global, used by cell_cpu_setup */ + ctr_enabled |= (1 << i); } } + /* initialize the previous counts for the virtual cntrs */ + for_each_online_cpu(cpu) + for (i = 0; i < num_counters; ++i) { + per_cpu(pmc_values, cpu)[i] = reset_value[i]; + } out: ; } @@ -577,7 +594,8 @@ if (ctr_enabled & (1 << i)) { cbe_write_ctr(cpu, i, reset_value[i]); enable_ctr(cpu, i, pm_regs.pm07_cntrl); - interrupt_mask |= CBE_PM_CTR_OVERFLOW_INTR(i); + interrupt_mask |= + CBE_PM_CTR_OVERFLOW_INTR(i); } else { /* Disable counter */ cbe_write_pm07_control(cpu, i, 0); @@ -591,14 +609,14 @@ virt_cntr_inter_mask = interrupt_mask; oprofile_running = 1; - smp_wmb(); + smp_wmb(); /* NOTE: start_virt_cntrs will result in cell_virtual_cntr() being * executed which manipulates the PMU. We start the "virtual counter" * here so that we do not need to synchronize access to the PMU in * the above for-loop. - */ - start_virt_cntrs(); + */ + start_virt_cntrs(); } static void cell_global_stop(void) @@ -609,7 +627,6 @@ * There is one performance monitor per node, so we * only need to perform this function once per node. */ - del_timer_sync(&timer_virt_cntr); oprofile_running = 0; smp_wmb(); @@ -644,9 +661,10 @@ /* Need to make sure the interrupt handler and the virt counter * routine are not running at the same time. See the - * cell_virtual_cntr() routine for additional comments. */ + * cell_virtual_cntr() routine for additional comments. + */ spin_lock_irqsave(&virt_cntr_lock, flags); - + /* Need to disable and reenable the performance counters * to get the desired behavior from the hardware. This * is hardware specific. @@ -667,8 +685,8 @@ is_kernel = is_kernel_addr(pc); for (i = 0; i < num_counters; ++i) { - if ((interrupt_mask & CBE_PM_CTR_OVERFLOW_INTR(i)) && - ctr[i].enabled) { + if ((interrupt_mask & CBE_PM_CTR_OVERFLOW_INTR(i)) + && ctr[i].enabled) { oprofile_add_pc(pc, is_kernel, i); cbe_write_ctr(cpu, i, reset_value[i]); } @@ -676,8 +694,13 @@ /* The counters were frozen by the interrupt. * Reenable the interrupt and restart the counters. + * If there was a race between the interrupt handler and + * the virtual counter routine. The virutal counter + * routine may have cleared the interrupts. Hence must + * use the virt_cntr_inter_mask to re-enable the interrupts. */ - cbe_enable_pm_interrupts(cpu, hdw_thread, interrupt_mask); + cbe_enable_pm_interrupts(cpu, hdw_thread, + virt_cntr_inter_mask); /* The writes to the various performance counters only writes * to a latch. The new values (interrupt setting bits, reset Index: linux-2.6.19-rc6-arnd1+patches/include/asm-powerpc/cell-pmu.h =================================================================== --- linux-2.6.19-rc6-arnd1+patches.orig/include/asm-powerpc/cell-pmu.h 2006-11-20 10:23:38.524401368 -0600 +++ linux-2.6.19-rc6-arnd1+patches/include/asm-powerpc/cell-pmu.h 2006-11-20 10:26:49.914328408 -0600 @@ -104,7 +104,7 @@ #define CBE_COUNT_ALL_MODES 3 /* Macros for the pm07_control registers. */ -#define PM07_CTR_INPUT_MUX(x) ((((x) & 1) << 26) & 0x3f) +#define PM07_CTR_INPUT_MUX(x) (((x) & 0x3F) << 26) #define PM07_CTR_INPUT_CONTROL(x) (((x) & 1) << 25) #define PM07_CTR_POLARITY(x) (((x) & 1) << 24) #define PM07_CTR_COUNT_CYCLES(x) (((x) & 1) << 23)