Messages in this thread | | | Date | Mon, 27 Feb 2012 11:06:32 -0500 | From | Vince Weaver <> | Subject | Re: [PATCH] perf_event use rdpmc rather than rdmsr when possible in kernel |
| |
On Mon, 27 Feb 2012, Peter Zijlstra wrote:
> On Mon, 2012-02-20 at 17:38 -0500, Vince Weaver wrote: > > > diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c > > index 5adce10..5550047 100644 > > --- a/arch/x86/kernel/cpu/perf_event.c > > +++ b/arch/x86/kernel/cpu/perf_event.c > > @@ -85,7 +85,7 @@ u64 x86_perf_event_update(struct perf_event *event) > > */ > > again: > > prev_raw_count = local64_read(&hwc->prev_count); > > - rdmsrl(hwc->event_base, new_raw_count); > > + new_raw_count=native_read_pmc(hwc->event_base_rdpmc); > > That really wants to be rdpmc(), bypassing paravirt like that isn't > nice.
I couldn't find another usable rdpmc() call in the kernel. Should I add one? I admit I hadn't thought that this might break VMs not expecting rdpmc calls from the kernel.
> > index abb2776..432ac69 100644 > > --- a/include/linux/perf_event.h > > +++ b/include/linux/perf_event.h > > @@ -562,6 +562,7 @@ struct hw_perf_event { > > u64 last_tag; > > unsigned long config_base; > > unsigned long event_base; > > + unsigned long event_base_rdpmc; > > We could make that unsigned int, the SDM explicitly states > rdmsr/wrmsr/rdpmc take ECX (and ignore the upper 32bits RCX).
OK.
> > int idx; > > int last_cpu; > > struct hw_perf_event_extra extra_reg; > > But yeah, avoiding the extra variable will add a conditional and extra > instructions to the rdpmc path.
yes, I couldn't think of a good way to get rid of the extra field without adding extra conditional branches in key code paths.
Mostly that's the fault of the Intel fixed counters, otherwise you could possibly fix things by cleverly adding or subtracting off the msr_base to get the counter number.
Vince
| |