[PATCH v2] perf/core: fix userpage->time_enabled of inactive events

[PATCH v2] perf/core: fix userpage->time_enabled of inactive events

[Song Liu]

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);
 	}
 
 	return 0;
-- 
2.30.2

Re: [PATCH v2] perf/core: fix userpage->time_enabled of inactive events

[Song Liu]

Hi Peter, 

> On Sep 23, 2021, at 6:28 PM, 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>

Could you please share your comments on this version?

Thanks,
Song

> ---
> 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);
> 	}
> 
> 	return 0;
> -- 
> 2.30.2
> 

Re: [PATCH v2] perf/core: fix userpage->time_enabled of inactive events

[Peter Zijlstra]

On Thu, Sep 23, 2021 at 06:28:00PM -0700, Song Liu wrote:

> 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.
> +	 */

It seems to me that writing that comment was more work than actually
fixing perf_mmap() to DTRT, no? AFAICT all we need is something like:

diff --git a/kernel/events/core.c b/kernel/events/core.c
index fd2ae70fa6c4..1e33c2e6b0dc 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6324,6 +6324,8 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
 
 		ring_buffer_attach(event, rb);
 
+		perf_event_update_time(event);
+		perf_set_shadow_time(event, event->ctx);
 		perf_event_init_userpage(event);
 		perf_event_update_userpage(event);
 	} else {

Re: [PATCH v2] perf/core: fix userpage->time_enabled of inactive events

[Athira Rajeev]

> 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;


Thanks,
Athira Rajeev

> 
> 	return 0;
> -- 
> 2.30.2
> 

Re: [PATCH v2] perf/core: fix userpage->time_enabled of inactive events

[Song Liu]

> On Sep 29, 2021, at 2:18 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Thu, Sep 23, 2021 at 06:28:00PM -0700, Song Liu wrote:
> 
>> 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.
>> +	 */
> 
> It seems to me that writing that comment was more work than actually
> fixing perf_mmap() to DTRT, no? AFAICT all we need is something like:
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index fd2ae70fa6c4..1e33c2e6b0dc 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6324,6 +6324,8 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
> 
> 		ring_buffer_attach(event, rb);
> 
> +		perf_event_update_time(event);
> +		perf_set_shadow_time(event, event->ctx);
> 		perf_event_init_userpage(event);
> 		perf_event_update_userpage(event);
> 	} else {

Yeah, this does work. I will fold this in v3. 

Thanks,
Song

Re: [PATCH v2] perf/core: fix userpage->time_enabled of inactive events

[Song Liu]

> 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

Re: [PATCH v2] perf/core: fix userpage->time_enabled of inactive events

[Athira Rajeev]

> 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