Messages in this thread | | | From | Song Liu <> | Subject | Re: [PATCH v2] perf/core: fix userpage->time_enabled of inactive events | Date | Wed, 29 Sep 2021 19:41:19 +0000 |
| |
> On Sep 29, 2021, at 10:04 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote: > > > >> On 24-Sep-2021, at 6:58 AM, Song Liu <songliubraving@fb.com> wrote: >> >> Users of rdpmc rely on the mmapped user page to calculate accurate >> time_enabled. Currently, userpage->time_enabled is only updated when the >> event is added to the pmu. As a result, inactive event (due to counter >> multiplexing) does not have accurate userpage->time_enabled. This can >> be reproduced with something like: >> >> /* open 20 task perf_event "cycles", to create multiplexing */ >> >> fd = perf_event_open(); /* open task perf_event "cycles" */ >> userpage = mmap(fd); /* use mmap and rdmpc */ >> >> while (true) { >> time_enabled_mmap = xxx; /* use logic in perf_event_mmap_page */ >> time_enabled_read = read(fd).time_enabled; >> if (time_enabled_mmap > time_enabled_read) >> BUG(); >> } >> >> Fix this by updating userpage for inactive events in merge_sched_in. >> >> Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org> >> Reported-and-tested-by: Lucian Grijincu <lucian@fb.com> >> Signed-off-by: Song Liu <songliubraving@fb.com> >> --- >> include/linux/perf_event.h | 4 +++- >> kernel/events/core.c | 49 ++++++++++++++++++++++++++++++++++---- >> 2 files changed, 48 insertions(+), 5 deletions(-) >> >> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h >> index 2d510ad750edc..4aa52f7a48c16 100644 >> --- a/include/linux/perf_event.h >> +++ b/include/linux/perf_event.h >> @@ -683,7 +683,9 @@ struct perf_event { >> /* >> * timestamp shadows the actual context timing but it can >> * be safely used in NMI interrupt context. It reflects the >> - * context time as it was when the event was last scheduled in. >> + * context time as it was when the event was last scheduled in, >> + * or when ctx_sched_in failed to schedule the event because we >> + * run out of PMC. >> * >> * ctx_time already accounts for ctx->timestamp. Therefore to >> * compute ctx_time for a sample, simply add perf_clock(). >> diff --git a/kernel/events/core.c b/kernel/events/core.c >> index 1cb1f9b8392e2..d73f986eef7b3 100644 >> --- a/kernel/events/core.c >> +++ b/kernel/events/core.c >> @@ -3707,6 +3707,46 @@ static noinline int visit_groups_merge(struct perf_cpu_context *cpuctx, >> return 0; >> } >> >> +static inline bool event_update_userpage(struct perf_event *event) >> +{ >> + /* >> + * Checking mmap_count to avoid unnecessary work. This does leave a >> + * corner case: if the event is enabled before mmap(), the first >> + * time the event gets scheduled is via: >> + * >> + * __perf_event_enable (or __perf_install_in_context) >> + * -> ctx_resched >> + * -> perf_event_sched_in >> + * -> ctx_sched_in >> + * >> + * with mmap_count of 0, so we will skip here. As a result, >> + * userpage->offset is not accurate after mmap and before the >> + * first rotation. >> + * >> + * To avoid the discrepancy of this window, the user space should >> + * mmap the event before enabling it. >> + */ >> + if (likely(!atomic_read(&event->mmap_count))) >> + return false; >> + >> + perf_event_update_time(event); >> + perf_set_shadow_time(event, event->ctx); >> + perf_event_update_userpage(event); >> + >> + return true; >> +} >> + >> +static inline void group_update_userpage(struct perf_event *group_event) >> +{ >> + struct perf_event *event; >> + >> + if (!event_update_userpage(group_event)) >> + return; >> + >> + for_each_sibling_event(event, group_event) >> + event_update_userpage(event); >> +} >> + >> static int merge_sched_in(struct perf_event *event, void *data) >> { >> struct perf_event_context *ctx = event->ctx; >> @@ -3725,14 +3765,15 @@ static int merge_sched_in(struct perf_event *event, void *data) >> } >> >> if (event->state == PERF_EVENT_STATE_INACTIVE) { >> + *can_add_hw = 0; >> if (event->attr.pinned) { >> perf_cgroup_event_disable(event, ctx); >> perf_event_set_state(event, PERF_EVENT_STATE_ERROR); >> + } else { >> + ctx->rotate_necessary = 1; >> + perf_mux_hrtimer_restart(cpuctx); >> + group_update_userpage(event); >> } >> - >> - *can_add_hw = 0; >> - ctx->rotate_necessary = 1; >> - perf_mux_hrtimer_restart(cpuctx); >> } > > Another optimisation that is possible in merge_sched_in is we can avoid calling "perf_mux_hrtimer_restart" multiple times if already rotate_necessary is set for that context. Even though "perf_mux_hrtimer_restart" will just return if hrtimer is already active, we could avoid the overhead of calling this function multiple times if there are many groups. > > Something like below: > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 0c000cb01eeb..26eae79bd723 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -3731,8 +3731,10 @@ static int merge_sched_in(struct perf_event *event, void *data) > } > > *can_add_hw = 0; > - ctx->rotate_necessary = 1; > - perf_mux_hrtimer_restart(cpuctx); > + if (!ctx->rotate_necessary) { > + ctx->rotate_necessary = 1; > + perf_mux_hrtimer_restart(cpuctx); > + } > } > > return 0;
Yeah, this makes sense. Do you plan to send a patch for this?
Thanks, Song
| |