Messages in this thread Patch in this message | | | From | Song Liu <> | Subject | Re: [PATCH v8] perf: Sharing PMU counters across compatible events | Date | Thu, 12 Dec 2019 15:37:05 +0000 |
| |
> On Dec 12, 2019, at 5:49 AM, Peter Zijlstra <peterz@infradead.org> wrote: > > On Fri, Dec 06, 2019 at 04:24:47PM -0800, Song Liu wrote: > >> @@ -750,6 +752,16 @@ struct perf_event { >> void *security; >> #endif >> struct list_head sb_list; >> + >> + /* for PMU sharing */ >> + struct perf_event *dup_master; >> + /* check event_sync_dup_count() for the use of dup_base_* */ >> + u64 dup_base_count; >> + u64 dup_base_child_count; >> + /* when this event is master, read from master*count */ >> + local64_t master_count; >> + atomic64_t master_child_count; >> + int dup_active_count; >> #endif /* CONFIG_PERF_EVENTS */ >> }; > >> +/* PMU sharing aware version of event->pmu->add() */ >> +static int event_pmu_add(struct perf_event *event, >> + struct perf_event_context *ctx) >> +{ >> + struct perf_event *master; >> + int ret; >> + >> + /* no sharing, just do event->pmu->add() */ >> + if (!event->dup_master) >> + return event->pmu->add(event, PERF_EF_START); > > Possibly we should look at the location of perf_event::dup_master to be > in a hot cacheline. Because I'm thinking you just added a guaranteed > miss here.
I am not quite sure the best location for these. How about:
diff --git i/include/linux/perf_event.h w/include/linux/perf_event.h index 7d49f9251621..218cc7f75775 100644 --- i/include/linux/perf_event.h +++ w/include/linux/perf_event.h @@ -643,6 +643,16 @@ struct perf_event { local64_t count; atomic64_t child_count;
+ /* for PMU sharing */ + struct perf_event *dup_master; + /* check event_sync_dup_count() for the use of dup_base_* */ + u64 dup_base_count; + u64 dup_base_child_count; + /* when this event is master, read from master*count */ + local64_t master_count; + atomic64_t master_child_count; + int dup_active_count; + /* * These are the total time in nanoseconds that the event * has been enabled (i.e. eligible to run, and the task has ?
> >> + >> + master = event->dup_master; >> + >> + if (!master->dup_active_count) { >> + ret = event->pmu->add(master, PERF_EF_START); >> + if (ret) >> + return ret; >> + >> + if (master != event) >> + perf_event_set_state(master, PERF_EVENT_STATE_ENABLED); >> + } >> + >> + master->dup_active_count++; >> + master->pmu->read(master); >> + event->dup_base_count = local64_read(&master->count); >> + event->dup_base_child_count = atomic64_read(&master->child_count); >> + return 0; >> +} > >> +/* PMU sharing aware version of event->pmu->del() */ >> +static void event_pmu_del(struct perf_event *event, >> + struct perf_event_context *ctx) >> +{ >> + struct perf_event *master; >> + >> + if (event->dup_master == NULL) { >> + event->pmu->del(event, 0); >> + return; >> + } > > How about you write it exactly like the add version: > > if (!event->dup_master) > return event->pmu->del(event, 0); > > ?
Sure, will fix in v9.
Thanks, Song > >> + >> + master = event->dup_master; >> + event_sync_dup_count(event, master); >> + if (--master->dup_active_count == 0) { >> + event->pmu->del(master, 0); >> + perf_event_set_state(master, PERF_EVENT_STATE_INACTIVE); >> + } else if (master == event) { >> + perf_event_set_state(master, PERF_EVENT_STATE_ENABLED); >> + } >> +} >> + >> +/* PMU sharing aware version of event->pmu->read() */ >> +static void event_pmu_read(struct perf_event *event) >> +{ >> + if (event->dup_master == NULL) { >> + event->pmu->read(event); >> + return; >> + } > > And here too. > >> + event_sync_dup_count(event, event->dup_master); >> +}
| |