Messages in this thread | | | From | Alexander Shishkin <> | Subject | Re: [PATCH v9 12/14] x86: perf: intel_pt: Intel PT PMU driver | Date | Tue, 20 Jan 2015 15:20:00 +0200 |
| |
Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:
> Peter Zijlstra <peterz@infradead.org> writes: > >> On Wed, Jan 14, 2015 at 02:18:21PM +0200, Alexander Shishkin wrote: >>> +static __init int pt_init(void) >>> +{ >> >>> + pt_pmu.pmu.attr_groups = pt_attr_groups; >>> + pt_pmu.pmu.task_ctx_nr = perf_hw_context; >> >> I just noticed this one, how can this ever work? We want the PT thing to >> always get programmed, right? -- because we disallow creating more than >> 1? >> >> Which reminds me; does that exclusive thing you did not allow you to >> create one cpu wide and one per task (they're separate contexts) events? >> At which point we're not schedulable at all. >> >> By sticking it on the HW context list it can end up not being programed >> because its stuck after a bunch of hardware events that don't all fit on >> the PMU. >> >> Would not the SW list be more appropriate; the SW list is a list of >> events that's guaranteed to be schedulable. > > You're right, of course. > > As for the exclusive events, how about something like the code below (on > top of the previous exclusive event patch)? The only remaining issue > that I see is creating cpu-wide events in the presence of per-thread > (event->cpu==-1) events. Both would still work, but only one of them > will actually get scheduled at a time. I'm thinking about adding a > counter for per-thread events to struct pmu for this purpose, so that if > any are present, we can disallow creating cpu-wide events. Or, we can > leave it as it is. > > What do you think? > > --- > kernel/events/core.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 47 insertions(+), 5 deletions(-) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index cf0bf99f53..e8c86530e2 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -7688,14 +7688,11 @@ static bool exclusive_event_match(struct perf_event *e1, struct perf_event *e2) > return false; > } > > -static bool exclusive_event_ok(struct perf_event *event, > - struct perf_event_context *ctx) > +static bool __exclusive_event_ok(struct perf_event *event, > + struct perf_event_context *ctx) > { > struct perf_event *iter_event; > > - if (!(event->pmu->capabilities & PERF_PMU_CAP_EXCLUSIVE)) > - return true; > - > list_for_each_entry(iter_event, &ctx->event_list, event_entry) { > if (exclusive_event_match(iter_event, event)) > return false; > @@ -7704,6 +7701,51 @@ static bool exclusive_event_ok(struct perf_event *event, > return true; > } > > +static bool __exclusive_event_ok_on_cpu(struct perf_event *event, int cpu) > +{ > + struct perf_event_context *cpuctx; > + bool ret; > + > + cpuctx = find_get_context(event->pmu, NULL, cpu); > + mutex_lock(&cpuctx->mutex); > + ret = __exclusive_event_ok(event, cpuctx); > + perf_unpin_context(cpuctx); > + put_ctx(cpuctx); > + mutex_unlock(&cpuctx->mutex);
Actually, find_get_context() is not needed here, the following should be sufficient:
cpuctx = &per_cpu_ptr(event->pmu->pmu_cpu_context, cpu)->ctx;
mutex_lock(&cpuctx->mutex); ret = __exclusive_event_ok(event, cpuctx); mutex_unlock(&cpuctx->mutex);
Regards, -- Alex
| |