Messages in this thread | | | Date | Mon, 31 May 2010 18:09:10 +0400 | From | Cyrill Gorcunov <> | Subject | Re: [RFC] perf, x86: Segregate PMU workaraunds into x86_pmu_quirk_ops structure |
| |
On Mon, May 31, 2010 at 03:00:58PM +0200, Robert Richter wrote: > On 29.05.10 14:24:09, Cyrill Gorcunov wrote: > > Hi, > > > > I would appreciate comments/complains on the following patch. The idea is to implement > > PMU quirks with minimal impact. At the moment two quirks are addressed - > > PEBS disabling on Clovertown and P4 performance counter double write. > > PEBS disabling already was there only moved to x86_pmu_quirk_ops. Note > > that I didn't use pointer to the structure intensionally but embed it into > > x86_pmu, if the structure grow we will need to use a pointer to the structure. > > The quirk functions add additional code and ops structures to the > already existing model specific code. This quirks would be fine if we > would could merge model specific code and get unified code. But these > model specific code cannot be replaced. So I rather prefer to > implement cpu errata in model specific code. >
agreed, but this quirks ops looked as more clear solution for me, at least in a sake of initiating the 'find something better' dialogue you know :)
> > @@ -185,6 +185,11 @@ union perf_capabilities { > > u64 capabilities; > > }; > > > > +struct x86_pmu_quirk_ops { > > + void (*pmu_init)(void); > > This init quirk could be much better handled in the model specific > init code (intel_pmu_init()/amd_pmu_init()). I don't see a reason for > adding the quirk first and then immediately calling it. The quirk > function could be called directly instead. >
well, at moment we have only the one caller but if the number of callers increase _better_ to have it called via function pointer since it's easier to find out the callers in further and we're allowed to use such approach since this is a not fast path code. On the other hands I rather agree with you -- it's overzealous at the moment. /me: dropping idea on *pmu_init
> > + void (*perfctr_write)(unsigned long addr, u64 value); > > This one is difficult to avoid ... >
unfortunately
> > @@ -924,7 +930,11 @@ x86_perf_event_set_period(struct perf_ev > > */ > > atomic64_set(&hwc->prev_count, (u64)-left); > > > > - wrmsrl(hwc->event_base + idx, > > + if (x86_pmu.quirks.perfctr_write) > > + x86_pmu.quirks.perfctr_write(hwc->event_base + idx, > > + (u64)(-left) & x86_pmu.cntval_mask); > > + else > > + wrmsrl(hwc->event_base + idx, > > ... but it introduces another check in the fast path. There are some > options to avoid this. First we could see if we rather implement this > in model specific interrupt handlers (there is p4_pmu_handle_irq()).
no, we can't, I thought about that, this code is called from several places.
> Or, we implement an optimized check for perf quirks, maybe using > ALTERNATIVE or jump labels. >
yes! Robert, I completely forgot about alternatives. I guess that is exactly what we need! I'll try to implement.
> I think we can handle both quirks, but if we start using and extending > it more, it will have a performance impact and code will also more > complicated. So, I think it is rather inappropriate as a general > approach. > > -Robert >
Thanks a huge for comments, Robert!
> > (u64)(-left) & x86_pmu.cntval_mask); > > > > perf_event_update_userpage(event); > > -- > Advanced Micro Devices, Inc. > Operating System Research Center > email: robert.richter@amd.com > -- Cyrill
| |