Messages in this thread | | | Subject | Re: [PATCH v2] perf/core: fix userpage->time_enabled of inactive events | From | Athira Rajeev <> | Date | Thu, 30 Sep 2021 10:02:43 +0530 |
| |
> On 30-Sep-2021, at 1:11 AM, Song Liu <songliubraving@fb.com> wrote: > > > >> 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?
Yes, I will send a separate patch for this change right away.
Thanks Athira > > Thanks, > Song
| |