Re: [perfmon2] [PATCH] perf_events: improve x86 event scheduling (v6 incremental)
Status: Beta
Brought to you by:
seranian
From: stephane e. <er...@go...> - 2010-01-25 17:12:30
|
On Fri, Jan 22, 2010 at 9:27 PM, Peter Zijlstra <pe...@in...> wrote: > On Thu, 2010-01-21 at 17:39 +0200, Stephane Eranian wrote: >> @@ -1395,40 +1430,28 @@ void hw_perf_enable(void) >> * apply assignment obtained either from >> * hw_perf_group_sched_in() or x86_pmu_enable() >> * >> - * step1: save events moving to new counters >> - * step2: reprogram moved events into new counters >> + * We either re-enable or re-program and re-enable. >> + * All events are disabled by the time we come here. >> + * That means their state has been saved already. >> */ > > I'm not seeing how it is true. > Suppose a core2 with counter0 active counting a non-restricted event, > say cpu_cycles. Then we do: > > perf_disable() > hw_perf_disable() > intel_pmu_disable_all > wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0); > everything is disabled globally, yet individual counter0 is not. But that's enough to stop it. > ->enable(MEM_LOAD_RETIRED) /* constrained to counter0 */ > x86_pmu_enable() > collect_events() > x86_schedule_events() > n_added = 1 > > /* also slightly confused about this */ > if (hwc->idx != -1) > x86_perf_event_set_period() > In x86_pmu_enable(), we have not yet actually assigned the counter to hwc->idx. This is only accomplished by hw_perf_enable(). Yet, x86_perf_event_set_period() is going to write the MSR. My understanding is that you never call enable(event) in code outside of a perf_disable()/perf_enable() section. > perf_enable() > hw_perf_enable() > > /* and here we'll assign the new event to counter0 > * except we never disabled it... */ > You will have two events, scheduled, cycles in counter1 and mem_load_retired in counter0. Neither hwc->idx will match previous state and thus both will be rewritten. I think the case you are worried about is different. It is the case where you would move an event to a new counter without replacing it with a new event. Given that the individual MSR.en would still be 1 AND that enable_all() enables all counters (even the ones not actively used), then we would get a runaway counter so to speak. It seems a solution would be to call x86_pmu_disable() before assigning an event to a new counter for all events which are moving. This is because we cannot assume all events have been previously disabled individually. Something like if (!match_prev_assignment(hwc, cpuc, i)) { if (hwc->idx != -1) x86_pmu.disable(hwc, hwc->idx); x86_assign_hw_event(event, cpuc, cpuc->assign[i]); x86_perf_event_set_period(event, hwc, hwc->idx); } > intel_pmu_enable_all() > wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, intel_ctrl) > > Or am I missing something? > |