On Fri, Jan 22, 2010 at 9:27 PM, Peter Zijlstra <pet...@infradead.org> 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?
>
------------------------------------------------------------------------------
Throughout its 18-year history, RSA Conference consistently attracts the
world's best and brightest in the field, creating opportunities for Conference
attendees to learn about information security's most important issues through
interactions with peers, luminaries and emerging and established companies.
http://p.sf.net/sfu/rsaconf-dev2dev
_______________________________________________
perfmon2-devel mailing list
perfmon2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
