Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\))
Subject: Re: [PATCH v2] perf/core: fix userpage->time_enabled of inactive
 events
From:   Athira Rajeev <atrajeev@linux.vnet.ibm.com>
In-Reply-To: <4AB79DA5-A8AF-456D-A009-3F24EEDA20F2@fb.com>
Date:   Thu, 30 Sep 2021 10:02:43 +0530
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Kernel Team <Kernel-team@fb.com>,
        Stephane Eranian <eranian@google.com>,
        Lucian Grijincu <lucian@fb.com>,
        Madhavan Srinivasan <maddy@linux.vnet.ibm.com>,
        kajoljain <kjain@linux.ibm.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <BB5E9E0E-A4C9-432E-8F0D-A6DD2F3915DE@linux.vnet.ibm.com>
References: <20210924012800.2461781-1-songliubraving@fb.com>
 <EEA8D03E-14A7-4A8B-8C99-B68CACEC25D2@linux.vnet.ibm.com>
 <4AB79DA5-A8AF-456D-A009-3F24EEDA20F2@fb.com>
To:     Song Liu <songliubraving@fb.com>
Precedence: bulk


> On 30-Sep-2021, at 1:11 AM, Song Liu <songliubraving@fb.com> wrote:
>=20
>=20
>=20
>> On Sep 29, 2021, at 10:04 AM, Athira Rajeev =
<atrajeev@linux.vnet.ibm.com> wrote:
>>=20
>>=20
>>=20
>>> On 24-Sep-2021, at 6:58 AM, Song Liu <songliubraving@fb.com> wrote:
>>>=20
>>> 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:
>>>=20
>>> /* open 20 task perf_event "cycles", to create multiplexing */
>>>=20
>>> fd =3D perf_event_open();  /* open task perf_event "cycles" */
>>> userpage =3D mmap(fd);     /* use mmap and rdmpc */
>>>=20
>>> while (true) {
>>>   time_enabled_mmap =3D xxx; /* use logic in perf_event_mmap_page */
>>>   time_enabled_read =3D read(fd).time_enabled;
>>>   if (time_enabled_mmap > time_enabled_read)
>>>       BUG();
>>> }
>>>=20
>>> Fix this by updating userpage for inactive events in merge_sched_in.
>>>=20
>>> 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(-)
>>>=20
>>> 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;
>>> }
>>>=20
>>> +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 =3D event->ctx;
>>> @@ -3725,14 +3765,15 @@ static int merge_sched_in(struct perf_event =
*event, void *data)
>>> 	}
>>>=20
>>> 	if (event->state =3D=3D PERF_EVENT_STATE_INACTIVE) {
>>> +		*can_add_hw =3D 0;
>>> 		if (event->attr.pinned) {
>>> 			perf_cgroup_event_disable(event, ctx);
>>> 			perf_event_set_state(event, =
PERF_EVENT_STATE_ERROR);
>>> +		} else {
>>> +			ctx->rotate_necessary =3D 1;
>>> +			perf_mux_hrtimer_restart(cpuctx);
>>> +			group_update_userpage(event);
>>> 		}
>>> -
>>> -		*can_add_hw =3D 0;
>>> -		ctx->rotate_necessary =3D 1;
>>> -		perf_mux_hrtimer_restart(cpuctx);
>>> 	}
>>=20
>> 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.=20
>>=20
>> Something like below:
>>=20
>> 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)
>>               }
>>=20
>>               *can_add_hw =3D 0;
>> -               ctx->rotate_necessary =3D 1;
>> -               perf_mux_hrtimer_restart(cpuctx);
>> +               if (!ctx->rotate_necessary) {
>> +                       ctx->rotate_necessary =3D 1;
>> +                       perf_mux_hrtimer_restart(cpuctx);
>> +               }
>>       }
>>=20
>>       return 0;
>=20
> Yeah, this makes sense. Do you plan to send a patch for this?=20

Yes, I will send a separate patch for this change right away.

Thanks
Athira
>=20
> Thanks,
> Song