Hello,
Last month, I got an email from Paul Mackerras (the maintainer of the
Power arch for Linux) saying that the timer used for sampling the
free-running PMU counters 5 & 6 was causing a problem with spurious
kernel hangs. Paul said that we ought to switch to using high
resolution timers instead, since they avoid deadlock problems when the
handlers are executed in an interrupts-off context.
I have created and tried this change and it appears to work correctly.
Can you please review it and let me know if there any issues with it?
It patches just a single source file (the one for power6).
Thanks for your consideration,
- Corey
Corey Ashford
Software Engineer
IBM Linux Technology Center, Linux Toolchain
Beaverton, OR
503-578-3507
cjash...@us.ibm.com
diff --git a/arch/powerpc/perfmon/perfmon_power6.c
b/arch/powerpc/perfmon/perfmon_power6.c
index 7882feb..c8e10bc 100644
--- a/arch/powerpc/perfmon/perfmon_power6.c
+++ b/arch/powerpc/perfmon/perfmon_power6.c
@@ -25,6 +25,8 @@
#include <linux/module.h>
#include <linux/perfmon_kern.h>
+#include <linux/hrtimer.h>
+
MODULE_AUTHOR("Corey Ashford <cjash...@us.ibm.com>");
MODULE_DESCRIPTION("POWER6 PMU description table");
MODULE_LICENSE("GPL");
@@ -74,7 +76,7 @@ static struct pfm_regmap_desc pfm_power6_pmd_desc[] = {
u32 pmc5_start_save[NR_CPUS];
u32 pmc6_start_save[NR_CPUS];
-static struct timer_list pmc5_6_update[NR_CPUS];
+static struct hrtimer pmc5_6_update[NR_CPUS];
u64 enable_cntrs_cnt;
u64 disable_cntrs_cnt;
u64 call_delta;
@@ -84,7 +86,7 @@ u64 pm1_4_interrupt;
* timer.
*/
struct pfm_arch_context *pmc5_6_ctx_arch[NR_CPUS];
-long int update_time;
+ktime_t update_time;
static void delta(int cpu_num, struct pfm_arch_context *ctx_arch)
{
@@ -110,12 +112,14 @@ static void delta(int cpu_num, struct pfm_arch_context
*ctx_arch)
}
-static void pmc5_6_updater(unsigned long cpu_num)
+static enum hrtimer_restart pmc5_6_updater(struct hrtimer *hrtimer)
{
+ int cpu_num = smp_processor_id();
/* update the virtual pmd 5 and pmd 6 counters */
delta(cpu_num, pmc5_6_ctx_arch[cpu_num]);
- mod_timer(&pmc5_6_update[cpu_num], jiffies + update_time);
+ hrtimer_forward_now(&pmc5_6_update[cpu_num], update_time);
+ return HRTIMER_RESTART;
}
@@ -286,6 +290,7 @@ static void pfm_power6_enable_counters(struct pfm_context
*ctx,
unsigned int i, max_pmc;
int cpu_num = smp_processor_id();
struct pfm_arch_context *ctx_arch;
+ int retval;
enable_cntrs_cnt++;
@@ -314,10 +319,9 @@ static void pfm_power6_enable_counters(struct pfm_context
*ctx,
/* Start kernel timer for this cpu to periodically update
* the virtual counters.
*/
- init_timer(&pmc5_6_update[cpu_num]);
- pmc5_6_update[cpu_num].function = pmc5_6_updater;
- pmc5_6_update[cpu_num].data = (unsigned long) cpu_num;
- pmc5_6_update[cpu_num].expires = jiffies + update_time;
+ hrtimer_init(&pmc5_6_update[cpu_num], CLOCK_MONOTONIC,
HRTIMER_MODE_REL);
+ pmc5_6_update[cpu_num].function = pmc5_6_updater;
+ pmc5_6_update[cpu_num].cb_mode = HRTIMER_CB_IRQSAFE_UNLOCKED;
/* context for this timer, timer will be removed if context
* is switched because the counters will be stopped first.
* NEEDS WORK, I think this is all ok, a little concerned about a
@@ -326,7 +330,8 @@ static void pfm_power6_enable_counters(struct pfm_context
*ctx,
* about this.
*/
pmc5_6_ctx_arch[cpu_num] = ctx_arch;
- add_timer(&pmc5_6_update[cpu_num]);
+ retval = hrtimer_start(&pmc5_6_update[cpu_num], update_time,
HRTIMER_MODE_REL);
+ BUG_ON(retval);
}
/**
@@ -338,6 +343,7 @@ static void pfm_power6_disable_counters(struct pfm_context
*ctx,
{
struct pfm_arch_context *ctx_arch;
int cpu_num = smp_processor_id();
+ int retval;
disable_cntrs_cnt++;
@@ -345,8 +351,8 @@ static void pfm_power6_disable_counters(struct pfm_context
*ctx,
mtspr(SPRN_MMCR0, mfspr(SPRN_MMCR0) | MMCR0_FC);
asm volatile ("sync");
- /* delete kernel update timer */
- del_timer_sync(&pmc5_6_update[cpu_num]);
+ /* cancel the pmc5/6 update timer */
+ retval = hrtimer_cancel(&pmc5_6_update[cpu_num]);
/* Update the virtual pmd 5 and 6 counters from the free running
* HW counters
@@ -498,15 +504,21 @@ static int __init pfm_power6_pmu_init_module(void)
*/
# define MAX_EVENT_RATE (ppc_proc_freq * 2)
-
- /*
- * Calculate the time, in jiffies, it takes for event counter 5 or
- * 6 to completely wrap when counting at the max event rate, and
- * then figure on sampling at twice that rate.
- */
- update_time = (((unsigned long)HZ * OVERFLOW_VALUE)
- / ((unsigned long)MAX_EVENT_RATE)) / 2;
-
+# define SAFETY_FACTOR 2
+ {
+ u64 update_time_ns;
+
+ /*
+ * Calculate the time, in seconds + nanoseconds, it takes for
+ * counter 5 or 6 to completely wrap when counting at the max
+ * event rate, and then figure on sampling at half that period.
+ */
+ update_time_ns = ((NSEC_PER_SEC * OVERFLOW_VALUE) /
+ MAX_EVENT_RATE) / SAFETY_FACTOR;
+ update_time = ktime_set(
+ update_time_ns / NSEC_PER_SEC,
+ update_time_ns % NSEC_PER_SEC);
+ }
ret = pfm_pmu_register(&pfm_power6_pmu_conf);
return ret;
}
------------------------------------------------------------------------------
_______________________________________________
perfmon2-devel mailing list
perfmon2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
