| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 30 Sep 2021 10:02:43 +0530
From: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
To: Song Liu <songliubraving@...com>
Cc: LKML <linux-kernel@...r.kernel.org>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Kernel Team <Kernel-team@...com>,
Stephane Eranian <eranian@...gle.com>,
Lucian Grijincu <lucian@...com>,
Madhavan Srinivasan <maddy@...ux.vnet.ibm.com>,
kajoljain <kjain@...ux.ibm.com>
Subject: Re: [PATCH v2] perf/core: fix userpage->time_enabled of inactive
events
> On 30-Sep-2021, at 1:11 AM, Song Liu <songliubraving@...com> wrote:
>
>
>
>> On Sep 29, 2021, at 10:04 AM, Athira Rajeev <atrajeev@...ux.vnet.ibm.com> wrote:
>>
>>
>>
>>> On 24-Sep-2021, at 6:58 AM, Song Liu <songliubraving@...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@...radead.org>
>>> Reported-and-tested-by: Lucian Grijincu <lucian@...com>
>>> Signed-off-by: Song Liu <songliubraving@...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
Powered by blists - more mailing lists