On Wed, May 07, 2014 at 10:00:50PM -0400, Don Zickus wrote:
>
> I think my commit 13beacee817d27a40ffc6f065ea0042685611dd5 explains this
> corruption. Though I have to admit I haven't looked through the problem
> very closely yet.
>
> IOW my lazy fix in that commit doesn't cover fuzzers and the real problem
> in p4_pmu_schedule_events. :-)
Don, Vince, could you please give the patch a run? I've only compile tested
it obviously since I've no real p4 hw. And the patch itself is a bit ugly
but should bring the light if we're still having problems in events
scheduling.
---
arch/x86/kernel/cpu/perf_event_p4.c | 61 +++++++++++++++---------------------
1 file changed, 27 insertions(+), 34 deletions(-)
Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/cpu/perf_event_p4.c
+++ linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c
@@ -1063,23 +1063,23 @@ static int p4_pmu_handle_irq(struct pt_r
* swap thread specific fields according to a thread
* we are going to run on
*/
-static void p4_pmu_swap_config_ts(struct hw_perf_event *hwc, int cpu)
+static u64 p4_pmu_swap_config_ts(u64 config, int cpu)
{
u32 escr, cccr;
/*
* we either lucky and continue on same cpu or no HT support
*/
- if (!p4_should_swap_ts(hwc->config, cpu))
- return;
+ if (!p4_should_swap_ts(config, cpu))
+ return config;
/*
* the event is migrated from an another logical
* cpu, so we need to swap thread specific flags
*/
- escr = p4_config_unpack_escr(hwc->config);
- cccr = p4_config_unpack_cccr(hwc->config);
+ escr = p4_config_unpack_escr(config);
+ cccr = p4_config_unpack_cccr(config);
if (p4_ht_thread(cpu)) {
cccr &= ~P4_CCCR_OVF_PMI_T0;
@@ -1092,9 +1092,9 @@ static void p4_pmu_swap_config_ts(struct
escr &= ~P4_ESCR_T0_USR;
escr |= P4_ESCR_T1_USR;
}
- hwc->config = p4_config_pack_escr(escr);
- hwc->config |= p4_config_pack_cccr(cccr);
- hwc->config |= P4_CONFIG_HT;
+ config = p4_config_pack_escr(escr);
+ config |= p4_config_pack_cccr(cccr);
+ config |= P4_CONFIG_HT;
} else {
cccr &= ~P4_CCCR_OVF_PMI_T1;
cccr |= P4_CCCR_OVF_PMI_T0;
@@ -1106,10 +1106,12 @@ static void p4_pmu_swap_config_ts(struct
escr &= ~P4_ESCR_T1_USR;
escr |= P4_ESCR_T0_USR;
}
- hwc->config = p4_config_pack_escr(escr);
- hwc->config |= p4_config_pack_cccr(cccr);
- hwc->config &= ~P4_CONFIG_HT;
+ config = p4_config_pack_escr(escr);
+ config |= p4_config_pack_cccr(cccr);
+ config &= ~P4_CONFIG_HT;
}
+
+ return config;
}
/*
@@ -1208,6 +1210,7 @@ static int p4_pmu_schedule_events(struct
unsigned long used_mask[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
unsigned long escr_mask[BITS_TO_LONGS(P4_ESCR_MSR_TABLE_SIZE)];
int cpu = smp_processor_id();
+ u64 config[X86_PMC_IDX_MAX];
struct hw_perf_event *hwc;
struct p4_event_bind *bind;
unsigned int i, thread, num;
@@ -1233,12 +1236,13 @@ again:
if (pass > 2)
goto done;
- bind = p4_config_get_bind(hwc->config);
+ config[i] = hwc->config;
+ bind = p4_config_get_bind(config[i]);
escr_idx = p4_get_escr_idx(bind->escr_msr[thread]);
if (unlikely(escr_idx == -1))
goto done;
- if (hwc->idx != -1 && !p4_should_swap_ts(hwc->config, cpu)) {
+ if (hwc->idx != -1 && !p4_should_swap_ts(config[i], cpu)) {
cntr_idx = hwc->idx;
if (assign)
assign[i] = hwc->idx;
@@ -1250,32 +1254,15 @@ again:
/*
* Check whether an event alias is still available.
*/
- config_alias = p4_get_alias_event(hwc->config);
+ config_alias = p4_get_alias_event(config[i]);
if (!config_alias)
goto done;
- hwc->config = config_alias;
+ config[i] = config_alias;
pass++;
goto again;
}
- /*
- * Perf does test runs to see if a whole group can be assigned
- * together succesfully. There can be multiple rounds of this.
- * Unfortunately, p4_pmu_swap_config_ts touches the hwc->config
- * bits, such that the next round of group assignments will
- * cause the above p4_should_swap_ts to pass instead of fail.
- * This leads to counters exclusive to thread0 being used by
- * thread1.
- *
- * Solve this with a cheap hack, reset the idx back to -1 to
- * force a new lookup (p4_next_cntr) to get the right counter
- * for the right thread.
- *
- * This probably doesn't comply with the general spirit of how
- * perf wants to work, but P4 is special. :-(
- */
- if (p4_should_swap_ts(hwc->config, cpu))
- hwc->idx = -1;
- p4_pmu_swap_config_ts(hwc, cpu);
+
+ config[i] = p4_pmu_swap_config_ts(config[i], cpu);
if (assign)
assign[i] = cntr_idx;
reserve:
@@ -1284,6 +1271,12 @@ reserve:
}
done:
+ if (num == 0) {
+ for (i = 0; i < n; i++, num--) {
+ hwc = &cpuc->event_list[i]->hw;
+ hwc->config = config[i];
+ }
+ }
return num ? -EINVAL : 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
