[PATCH 4.19 1/2] perf/cgroups: Don't rotate events for cgroups unnecessarily

[PATCH 4.19 1/2] perf/cgroups: Don't rotate events for cgroups unnecessarily

[Wen Yang]

From: Ian Rogers <irogers@google.com>

[ Upstream commit fd7d55172d1e2e501e6da0a5c1de25f06612dc2e ]

Currently perf_rotate_context assumes that if the context's nr_events !=
nr_active a rotation is necessary for perf event multiplexing. With
cgroups, nr_events is the total count of events for all cgroups and
nr_active will not include events in a cgroup other than the current
task's. This makes rotation appear necessary for cgroups when it is not.

Add a perf_event_context flag that is set when rotation is necessary.
Clear the flag during sched_out and set it when a flexible sched_in
fails due to resources.

Signed-off-by: Ian Rogers <irogers@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Link: https://lkml.kernel.org/r/20190601082722.44543-1-irogers@google.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: stable@vger.kernel.org # 4.19+
Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
---
 include/linux/perf_event.h |  5 +++++
 kernel/events/core.c       | 42 ++++++++++++++++++++++--------------------
 2 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index d8b4d31..efe30b9 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -747,6 +747,11 @@ struct perf_event_context {
 	int				nr_stat;
 	int				nr_freq;
 	int				rotate_disable;
+	/*
+	 * Set when nr_events != nr_active, except tolerant to events not
+	 * necessary to be active due to scheduling constraints, such as cgroups.
+	 */
+	int				rotate_necessary;
 	atomic_t			refcount;
 	struct task_struct		*task;
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index b8b74a4..56e3789 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2952,6 +2952,12 @@ static void ctx_sched_out(struct perf_event_context *ctx,
 	if (!ctx->nr_active || !(is_active & EVENT_ALL))
 		return;
 
+	/*
+	 * If we had been multiplexing, no rotations are necessary, now no events
+	 * are active.
+	 */
+	ctx->rotate_necessary = 0;
+
 	perf_pmu_disable(ctx->pmu);
 	if (is_active & EVENT_PINNED) {
 		list_for_each_entry_safe(event, tmp, &ctx->pinned_active, active_list)
@@ -3319,10 +3325,13 @@ static int flexible_sched_in(struct perf_event *event, void *data)
 		return 0;
 
 	if (group_can_go_on(event, sid->cpuctx, sid->can_add_hw)) {
-		if (!group_sched_in(event, sid->cpuctx, sid->ctx))
-			list_add_tail(&event->active_list, &sid->ctx->flexible_active);
-		else
+		int ret = group_sched_in(event, sid->cpuctx, sid->ctx);
+		if (ret) {
 			sid->can_add_hw = 0;
+			sid->ctx->rotate_necessary = 1;
+			return 0;
+		}
+		list_add_tail(&event->active_list, &sid->ctx->flexible_active);
 	}
 
 	return 0;
@@ -3690,24 +3699,17 @@ static void rotate_ctx(struct perf_event_context *ctx, struct perf_event *event)
 static bool perf_rotate_context(struct perf_cpu_context *cpuctx)
 {
 	struct perf_event *cpu_event = NULL, *task_event = NULL;
-	bool cpu_rotate = false, task_rotate = false;
-	struct perf_event_context *ctx = NULL;
+	struct perf_event_context *task_ctx = NULL;
+	int cpu_rotate, task_rotate;
 
 	/*
 	 * Since we run this from IRQ context, nobody can install new
 	 * events, thus the event count values are stable.
 	 */
 
-	if (cpuctx->ctx.nr_events) {
-		if (cpuctx->ctx.nr_events != cpuctx->ctx.nr_active)
-			cpu_rotate = true;
-	}
-
-	ctx = cpuctx->task_ctx;
-	if (ctx && ctx->nr_events) {
-		if (ctx->nr_events != ctx->nr_active)
-			task_rotate = true;
-	}
+	cpu_rotate = cpuctx->ctx.rotate_necessary;
+	task_ctx = cpuctx->task_ctx;
+	task_rotate = task_ctx ? task_ctx->rotate_necessary : 0;
 
 	if (!(cpu_rotate || task_rotate))
 		return false;
@@ -3716,7 +3718,7 @@ static bool perf_rotate_context(struct perf_cpu_context *cpuctx)
 	perf_pmu_disable(cpuctx->ctx.pmu);
 
 	if (task_rotate)
-		task_event = ctx_first_active(ctx);
+		task_event = ctx_first_active(task_ctx);
 	if (cpu_rotate)
 		cpu_event = ctx_first_active(&cpuctx->ctx);
 
@@ -3724,17 +3726,17 @@ static bool perf_rotate_context(struct perf_cpu_context *cpuctx)
 	 * As per the order given at ctx_resched() first 'pop' task flexible
 	 * and then, if needed CPU flexible.
 	 */
-	if (task_event || (ctx && cpu_event))
-		ctx_sched_out(ctx, cpuctx, EVENT_FLEXIBLE);
+	if (task_event || (task_ctx && cpu_event))
+		ctx_sched_out(task_ctx, cpuctx, EVENT_FLEXIBLE);
 	if (cpu_event)
 		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
 
 	if (task_event)
-		rotate_ctx(ctx, task_event);
+		rotate_ctx(task_ctx, task_event);
 	if (cpu_event)
 		rotate_ctx(&cpuctx->ctx, cpu_event);
 
-	perf_event_sched_in(cpuctx, ctx, current);
+	perf_event_sched_in(cpuctx, task_ctx, current);
 
 	perf_pmu_enable(cpuctx->ctx.pmu);
 	perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
-- 
1.8.3.1

[PATCH 4.19 2/2] perf/core: Fix corner case in perf_rotate_context()

[Wen Yang]

From: Song Liu <songliubraving@fb.com>

[ Upstream commit 7fa343b7fdc4f351de4e3f28d5c285937dd1f42f ]

In perf_rotate_context(), when the first cpu flexible event fail to
schedule, cpu_rotate is 1, while cpu_event is NULL. Since cpu_event is
NULL, perf_rotate_context will _NOT_ call cpu_ctx_sched_out(), thus
cpuctx->ctx.is_active will have EVENT_FLEXIBLE set. Then, the next
perf_event_sched_in() will skip all cpu flexible events because of the
EVENT_FLEXIBLE bit.

In the next call of perf_rotate_context(), cpu_rotate stays 1, and
cpu_event stays NULL, so this process repeats. The end result is, flexible
events on this cpu will not be scheduled (until another event being added
to the cpuctx).

Here is an easy repro of this issue. On Intel CPUs, where ref-cycles
could only use one counter, run one pinned event for ref-cycles, one
flexible event for ref-cycles, and one flexible event for cycles. The
flexible ref-cycles is never scheduled, which is expected. However,
because of this issue, the cycles event is never scheduled either.

 $ perf stat -e ref-cycles:D,ref-cycles,cycles -C 5 -I 1000

           time             counts unit events
    1.000152973         15,412,480      ref-cycles:D
    1.000152973      <not counted>      ref-cycles     (0.00%)
    1.000152973      <not counted>      cycles         (0.00%)
    2.000486957         18,263,120      ref-cycles:D
    2.000486957      <not counted>      ref-cycles     (0.00%)
    2.000486957      <not counted>      cycles         (0.00%)

To fix this, when the flexible_active list is empty, try rotate the
first event in the flexible_groups. Also, rename ctx_first_active() to
ctx_event_to_rotate(), which is more accurate.

Signed-off-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: <kernel-team@fb.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sasha Levin <sashal@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: 8d5bce0c37fa ("perf/core: Optimize perf_rotate_context() event scheduling")
Link: https://lkml.kernel.org/r/20191008165949.920548-1-songliubraving@fb.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: stable@vger.kernel.org # 4.19+
Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
---
 kernel/events/core.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 56e3789..d3be2cd 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3689,11 +3689,23 @@ static void rotate_ctx(struct perf_event_context *ctx, struct perf_event *event)
 	perf_event_groups_insert(&ctx->flexible_groups, event);
 }
 
+/* pick an event from the flexible_groups to rotate */
 static inline struct perf_event *
-ctx_first_active(struct perf_event_context *ctx)
+ctx_event_to_rotate(struct perf_event_context *ctx)
 {
-	return list_first_entry_or_null(&ctx->flexible_active,
-					struct perf_event, active_list);
+	struct perf_event *event;
+
+	/* pick the first active flexible event */
+	event = list_first_entry_or_null(&ctx->flexible_active,
+					 struct perf_event, active_list);
+
+	/* if no active flexible event, pick the first event */
+	if (!event) {
+		event = rb_entry_safe(rb_first(&ctx->flexible_groups.tree),
+				      typeof(*event), group_node);
+	}
+
+	return event;
 }
 
 static bool perf_rotate_context(struct perf_cpu_context *cpuctx)
@@ -3718,9 +3730,9 @@ static bool perf_rotate_context(struct perf_cpu_context *cpuctx)
 	perf_pmu_disable(cpuctx->ctx.pmu);
 
 	if (task_rotate)
-		task_event = ctx_first_active(task_ctx);
+		task_event = ctx_event_to_rotate(task_ctx);
 	if (cpu_rotate)
-		cpu_event = ctx_first_active(&cpuctx->ctx);
+		cpu_event = ctx_event_to_rotate(&cpuctx->ctx);
 
 	/*
 	 * As per the order given at ctx_resched() first 'pop' task flexible
-- 
1.8.3.1

Re: [PATCH 4.19 1/2] perf/cgroups: Don't rotate events for cgroups unnecessarily

[Wen Yang]

Hi,

except for the following two patches:

fd7d55172d1e ("perf/cgroups: Don't rotate events for cgroups unnecessarily")

7fa343b7fdc4 ("perf/core: Fix corner case in perf_rotate_context()")

And this patch is also needed:

90c91dfb86d0 ("perf/core: Fix endless multiplex timer")


Would you please merge them into the 4.19 stable branch?
Thanks


--
Best wishes，
Wen


在 2021/6/7 下午3:59, Wen Yang 写道:
> From: Ian Rogers <irogers@google.com>
> 
> [ Upstream commit fd7d55172d1e2e501e6da0a5c1de25f06612dc2e ]
> 
> Currently perf_rotate_context assumes that if the context's nr_events !=
> nr_active a rotation is necessary for perf event multiplexing. With
> cgroups, nr_events is the total count of events for all cgroups and
> nr_active will not include events in a cgroup other than the current
> task's. This makes rotation appear necessary for cgroups when it is not.
> 
> Add a perf_event_context flag that is set when rotation is necessary.
> Clear the flag during sched_out and set it when a flexible sched_in
> fails due to resources.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Stephane Eranian <eranian@google.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Vince Weaver <vincent.weaver@maine.edu>
> Link: https://lkml.kernel.org/r/20190601082722.44543-1-irogers@google.com
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Cc: stable@vger.kernel.org # 4.19+
> Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
> ---
>   include/linux/perf_event.h |  5 +++++
>   kernel/events/core.c       | 42 ++++++++++++++++++++++--------------------
>   2 files changed, 27 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index d8b4d31..efe30b9 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -747,6 +747,11 @@ struct perf_event_context {
>   	int				nr_stat;
>   	int				nr_freq;
>   	int				rotate_disable;
> +	/*
> +	 * Set when nr_events != nr_active, except tolerant to events not
> +	 * necessary to be active due to scheduling constraints, such as cgroups.
> +	 */
> +	int				rotate_necessary;
>   	atomic_t			refcount;
>   	struct task_struct		*task;
>   
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index b8b74a4..56e3789 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2952,6 +2952,12 @@ static void ctx_sched_out(struct perf_event_context *ctx,
>   	if (!ctx->nr_active || !(is_active & EVENT_ALL))
>   		return;
>   
> +	/*
> +	 * If we had been multiplexing, no rotations are necessary, now no events
> +	 * are active.
> +	 */
> +	ctx->rotate_necessary = 0;
> +
>   	perf_pmu_disable(ctx->pmu);
>   	if (is_active & EVENT_PINNED) {
>   		list_for_each_entry_safe(event, tmp, &ctx->pinned_active, active_list)
> @@ -3319,10 +3325,13 @@ static int flexible_sched_in(struct perf_event *event, void *data)
>   		return 0;
>   
>   	if (group_can_go_on(event, sid->cpuctx, sid->can_add_hw)) {
> -		if (!group_sched_in(event, sid->cpuctx, sid->ctx))
> -			list_add_tail(&event->active_list, &sid->ctx->flexible_active);
> -		else
> +		int ret = group_sched_in(event, sid->cpuctx, sid->ctx);
> +		if (ret) {
>   			sid->can_add_hw = 0;
> +			sid->ctx->rotate_necessary = 1;
> +			return 0;
> +		}
> +		list_add_tail(&event->active_list, &sid->ctx->flexible_active);
>   	}
>   
>   	return 0;
> @@ -3690,24 +3699,17 @@ static void rotate_ctx(struct perf_event_context *ctx, struct perf_event *event)
>   static bool perf_rotate_context(struct perf_cpu_context *cpuctx)
>   {
>   	struct perf_event *cpu_event = NULL, *task_event = NULL;
> -	bool cpu_rotate = false, task_rotate = false;
> -	struct perf_event_context *ctx = NULL;
> +	struct perf_event_context *task_ctx = NULL;
> +	int cpu_rotate, task_rotate;
>   
>   	/*
>   	 * Since we run this from IRQ context, nobody can install new
>   	 * events, thus the event count values are stable.
>   	 */
>   
> -	if (cpuctx->ctx.nr_events) {
> -		if (cpuctx->ctx.nr_events != cpuctx->ctx.nr_active)
> -			cpu_rotate = true;
> -	}
> -
> -	ctx = cpuctx->task_ctx;
> -	if (ctx && ctx->nr_events) {
> -		if (ctx->nr_events != ctx->nr_active)
> -			task_rotate = true;
> -	}
> +	cpu_rotate = cpuctx->ctx.rotate_necessary;
> +	task_ctx = cpuctx->task_ctx;
> +	task_rotate = task_ctx ? task_ctx->rotate_necessary : 0;
>   
>   	if (!(cpu_rotate || task_rotate))
>   		return false;
> @@ -3716,7 +3718,7 @@ static bool perf_rotate_context(struct perf_cpu_context *cpuctx)
>   	perf_pmu_disable(cpuctx->ctx.pmu);
>   
>   	if (task_rotate)
> -		task_event = ctx_first_active(ctx);
> +		task_event = ctx_first_active(task_ctx);
>   	if (cpu_rotate)
>   		cpu_event = ctx_first_active(&cpuctx->ctx);
>   
> @@ -3724,17 +3726,17 @@ static bool perf_rotate_context(struct perf_cpu_context *cpuctx)
>   	 * As per the order given at ctx_resched() first 'pop' task flexible
>   	 * and then, if needed CPU flexible.
>   	 */
> -	if (task_event || (ctx && cpu_event))
> -		ctx_sched_out(ctx, cpuctx, EVENT_FLEXIBLE);
> +	if (task_event || (task_ctx && cpu_event))
> +		ctx_sched_out(task_ctx, cpuctx, EVENT_FLEXIBLE);
>   	if (cpu_event)
>   		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
>   
>   	if (task_event)
> -		rotate_ctx(ctx, task_event);
> +		rotate_ctx(task_ctx, task_event);
>   	if (cpu_event)
>   		rotate_ctx(&cpuctx->ctx, cpu_event);
>   
> -	perf_event_sched_in(cpuctx, ctx, current);
> +	perf_event_sched_in(cpuctx, task_ctx, current);
>   
>   	perf_pmu_enable(cpuctx->ctx.pmu);
>   	perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
> 

Re: [PATCH 4.19 1/2] perf/cgroups: Don't rotate events for cgroups unnecessarily

[Greg Kroah-Hartman]

On Wed, Jun 09, 2021 at 02:28:52PM +0800, Wen Yang wrote:
> 
> Hi,
> 
> except for the following two patches:
> 
> fd7d55172d1e ("perf/cgroups: Don't rotate events for cgroups unnecessarily")
> 
> 7fa343b7fdc4 ("perf/core: Fix corner case in perf_rotate_context()")
> 
> And this patch is also needed:
> 
> 90c91dfb86d0 ("perf/core: Fix endless multiplex timer")
> 
> 
> Would you please merge them into the 4.19 stable branch?

Last one now queued up, thanks.

greg k-h