Skip to contentSkip to conversation

perf/core: Fix installing arbitrary cgroup event into cpu

PATCH RESEND In review [email protected] 4 participants 8 messages Mar 6, 2018 – Mar 12, 2018 perf events

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH RESEND v5 1/1 perf/core: Fix installing arbitrary cgroup event into cpu

From: "leilei.lin" <[email protected]>

Do not install cgroup event into the CPU context and schedule it if the cgroup is not running on this CPU

While there is no task of cgroup running specified CPU, current kernel still install cgroup event into CPU context that causes another cgroup event can't be installed into this CPU.

This patch prevent scheduling events at __perf_install_in_context() and installing events at list_update_cgroup_event() if cgroup isn't running on specified CPU.

Signed-off-by leilei.lin <[email protected]>

---
v2: Set cpuctx->cgrp only if the same cgroup is running on this
CPU otherwise following events couldn't be activated immediately
v3: Enhance the comments and commit message
v4: Adjust to config
v5: Clear cpuctx->cgrp only when no cgroup event exists

 kernel/events/core.c | 54 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 13 deletions(-)
kernel/events/core.c +41 -13
index 4df5b69..f3ffa70 100644
@@ -933,31 +933,45 @@ list_update_cgroup_event(struct perf_event *event,
 {
 	struct perf_cpu_context *cpuctx;
 	struct list_head *cpuctx_entry;
+	struct perf_cgroup *cgrp;
 
 	if (!is_cgroup_event(event))
 		return;
 
-	if (add && ctx->nr_cgroups++)
-		return;
-	else if (!add && --ctx->nr_cgroups)
-		return;
 	/*
 	 * Because cgroup events are always per-cpu events,
 	 * this will always be called from the right CPU.
 	 */
 	cpuctx = __get_cpu_context(ctx);
-	cpuctx_entry = &cpuctx->cgrp_cpuctx_entry;
-	/* cpuctx->cgrp is NULL unless a cgroup event is active in this CPU .*/
-	if (add) {
-		struct perf_cgroup *cgrp = perf_cgroup_from_task(current, ctx);
+	cgrp = perf_cgroup_from_task(current, ctx);
+
+	/*
+	 * if only the cgroup is running on this cpu
+	 * and cpuctx->cgrp == NULL (otherwise it would've
+	 * been set with running cgroup), we put this cgroup
+	 * into cpu context. Or it would case mismatch in
+	 * following cgroup events at event_filter_match()
+	 */
+	if (add && !cpuctx->cgrp &&
+			cgroup_is_descendant(cgrp->css.cgroup,
+			event->cgrp->css.cgroup)) {
+		cpuctx->cgrp = cgrp;
+	}
+
+	if (add && ctx->nr_cgroups++)
+		return;
+	else if (!add && --ctx->nr_cgroups)
+		return;
 
+	/* no cgroup running */
+	if (!add)
+		cpuctx->cgrp = NULL;
+
+	cpuctx_entry = &cpuctx->cgrp_cpuctx_entry;
+	if (add)
 		list_add(cpuctx_entry, this_cpu_ptr(&cgrp_cpuctx_list));
-		if (cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup))
-			cpuctx->cgrp = cgrp;
-	} else {
+	else
 		list_del(cpuctx_entry);
-		cpuctx->cgrp = NULL;
-	}
 }
 
 #else /* !CONFIG_CGROUP_PERF */

@@ -2311,6 +2325,20 @@ static int  __perf_install_in_context(void *info)
 		raw_spin_lock(&task_ctx->lock);
 	}
 
+#ifdef CONFIG_CGROUP_PERF
+	if (is_cgroup_event(event)) {
+		/*
+		 * Only care about cgroup events.
+		 *
+		 * If only the task belongs to cgroup of this event,
+		 * we will continue the installment
+		 */
+		struct perf_cgroup *cgrp = perf_cgroup_from_task(current, ctx);
+		reprogram = cgroup_is_descendant(cgrp->css.cgroup,
+					event->cgrp->css.cgroup);
+	}
+#endif
+
 	if (reprogram) {
 		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
 		add_event_to_ctx(event, ctx);
Signature · 1 line
2.8.4.31.g9ed660f
maintainer
#1 lore raw
Quoted 4 lines · On Tue, Mar 06, 2018 at 05:36:37PM +0800, [email protected] wrote:

From: "leilei.lin" <[email protected]>

Do not install cgroup event into the CPU context and schedule it if the cgroup is not running on this CPU

OK, so far so good, this explains the bit in
__perf_install_in_context().

Quoted 7 lines

While there is no task of cgroup running specified CPU, current kernel still install cgroup event into CPU context that causes another cgroup event can't be installed into this CPU.

This patch prevent scheduling events at __perf_install_in_context() and installing events at list_update_cgroup_event() if cgroup isn't running on specified CPU.

This bit doesn't make sense, you don't in fact avoid anything in list_update_cgroup_event(), you do more, not less.

Quoted 17 lines
kernel/events/core.c +1 -0
index 4df5b69..f3ffa70 100644
@@ -933,31 +933,45 @@ list_update_cgroup_event(struct perf_event *event,
 {
 	struct perf_cpu_context *cpuctx;
 	struct list_head *cpuctx_entry;
+	struct perf_cgroup *cgrp;
 
 	if (!is_cgroup_event(event))
 		return;
 
 	/*
 	 * Because cgroup events are always per-cpu events,
 	 * this will always be called from the right CPU.
 	 */

kernel/events/core.c list_update_cgroup_event(struct perf_event *event, · line 954

Quoted 10 lines
kernel/events/core.c +9 -0
 	cpuctx = __get_cpu_context(ctx);
+	cgrp = perf_cgroup_from_task(current, ctx);
+
+	/*
+	 * if only the cgroup is running on this cpu
+	 * and cpuctx->cgrp == NULL (otherwise it would've
+	 * been set with running cgroup), we put this cgroup
+	 * into cpu context. Or it would case mismatch in
+	 * following cgroup events at event_filter_match()
+	 */

This is utterly incomprehensible, what?

kernel/events/core.c list_update_cgroup_event(struct perf_event *event, · line 959

Quoted 5 lines
kernel/events/core.c +5 -0
+	if (add && !cpuctx->cgrp &&
+			cgroup_is_descendant(cgrp->css.cgroup,
+			event->cgrp->css.cgroup)) {
+		cpuctx->cgrp = cgrp;
+	}

And that's just horrible coding style. Maybe something like:

	if (add && cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup)) {
		if (cpuctx->cgrp)
			WARN_ON_ONCE(cpuctx->cgrp != cgrp);
		cpuctx->cgrp = cgrp;
	}

that? But that still needs a comment to explain _why_ we do that here. Under what condition would we fail to have cpuctx->cgrp set while ctx->nr_cgroups. Your comment doesn't explain nor does your Changelog.

Quoted 19 lines
kernel/events/core.c +12 -0
+
+	if (add && ctx->nr_cgroups++)
+		return;
+	else if (!add && --ctx->nr_cgroups)
+		return;
 
+	/* no cgroup running */
+	if (!add)
+		cpuctx->cgrp = NULL;
+
+	cpuctx_entry = &cpuctx->cgrp_cpuctx_entry;
+	if (add)
 		list_add(cpuctx_entry, this_cpu_ptr(&cgrp_cpuctx_list));
+	else
 		list_del(cpuctx_entry);
 }
 
 #else /* !CONFIG_CGROUP_PERF */

kernel/events/core.c static int __perf_install_in_context(void *info) · line 2332

Quoted 8 lines
kernel/events/core.c +5 -0
@@ -2311,6 +2325,20 @@ static int  __perf_install_in_context(void *info)
 		raw_spin_lock(&task_ctx->lock);
 	}
 
+#ifdef CONFIG_CGROUP_PERF
+	if (is_cgroup_event(event)) {
+		/*
+		 * Only care about cgroup events.
+		 *

That bit is entirely spurious, if it right after if (is_cgroup_event()), obviously this block is only for cgroup events.

kernel/events/core.c static int __perf_install_in_context(void *info) · line 2334

Quoted 2 lines
kernel/events/core.c +2 -0
+		 * If only the task belongs to cgroup of this event,
+		 * we will continue the installment

And that isn't really english. I think you meant to write something like:

		/*
		 * If the current cgroup doesn't match the event's
		 * cgroup, we should not try to schedule it.
		 */
Quoted 13 lines
kernel/events/core.c +7 -0
+		 */
+		struct perf_cgroup *cgrp = perf_cgroup_from_task(current, ctx);
+		reprogram = cgroup_is_descendant(cgrp->css.cgroup,
+					event->cgrp->css.cgroup);
+	}
+#endif
+
 	if (reprogram) {
 		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
 		add_event_to_ctx(event, ctx);
Signature · 1 line
2.8.4.31.g9ed660f
author
#2 lore raw

2018-03-06 19:50 GMT+08:00 Peter Zijlstra <[email protected]>:

Quoted 9 lines · On Tue, Mar 06, 2018 at 05:36:37PM +0800, [email protected] wrote:
Quoted 4 lines

From: "leilei.lin" <[email protected]>

Do not install cgroup event into the CPU context and schedule it if the cgroup is not running on this CPU

OK, so far so good, this explains the bit in
__perf_install_in_context().

Actually, the new codes in __perf_install_in_context() only takes care whether if events should be scheduled with PMU.

Quoted 11 lines
Quoted 7 lines

While there is no task of cgroup running specified CPU, current kernel still install cgroup event into CPU context that causes another cgroup event can't be installed into this CPU.

This patch prevent scheduling events at __perf_install_in_context() and installing events at list_update_cgroup_event() if cgroup isn't running on specified CPU.

This bit doesn't make sense, you don't in fact avoid anything in list_update_cgroup_event(), you do more, not less.

And the new codes in list_update_cgroup_event() don't want cpuctx->cgrp to be set arbitrarily. The more logic, you mentioned, was added for making sure cpuctx->cgrp is set consistently with the cgroup running on the cpu.

Quoted 13 lines
Quoted 13 lines
kernel/events/core.c +1 -0
index 4df5b69..f3ffa70 100644
@@ -933,31 +933,45 @@ list_update_cgroup_event(struct perf_event *event,
 {
      struct perf_cpu_context *cpuctx;
      struct list_head *cpuctx_entry;
+     struct perf_cgroup *cgrp;
 
      if (!is_cgroup_event(event))
              return;
Quoted 16 lines
Quoted 4 lines
kernel/events/core.c +0 -0
      /*
       * Because cgroup events are always per-cpu events,
       * this will always be called from the right CPU.
       */

kernel/events/core.c list_update_cgroup_event(struct perf_event *event, · line 954

Quoted 10 lines
kernel/events/core.c +9 -0
      cpuctx = __get_cpu_context(ctx);
+     cgrp = perf_cgroup_from_task(current, ctx);
+
+     /*
+      * if only the cgroup is running on this cpu
+      * and cpuctx->cgrp == NULL (otherwise it would've
+      * been set with running cgroup), we put this cgroup
+      * into cpu context. Or it would case mismatch in
+      * following cgroup events at event_filter_match()
+      */

This is utterly incomprehensible, what?

Yes, this is bit messy. I should've made it clear. This comment was supposed to explain the reason why I modified the if statement below.

And the logic is

1) when cpuctx-> cgrp is NULL, we __must__ take care of how to set it appropriately, that means, we __have to__ check if the cgroup is running on the cpu

2) when cpuctx-> cgrp is __NOT__ NULL. It means cpuctx->cgrp had been set appropriately by cgroup_switch() or list_update_cgroup_event() before. Therefore, We do __nothing__ here

Quoted 15 lines

kernel/events/core.c list_update_cgroup_event(struct perf_event *event, · line 959

Quoted 6 lines
kernel/events/core.c +5 -0
+     if (add && !cpuctx->cgrp &&
+                     cgroup_is_descendant(cgrp->css.cgroup,
+                     event->cgrp->css.cgroup)) {
+             cpuctx->cgrp = cgrp;
+     }

And that's just horrible coding style. Maybe something like:

        if (add && cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup)) {
                if (cpuctx->cgrp)
                        WARN_ON_ONCE(cpuctx->cgrp != cgrp);
                cpuctx->cgrp = cgrp;
        }
Quoted 4 lines

that? But that still needs a comment to explain _why_ we do that here. Under what condition would we fail to have cpuctx->cgrp set while ctx->nr_cgroups. Your comment doesn't explain nor does your Changelog.

        if (cpuctx->cgrp == NULL) /* As I said above, we only take
care this case. */
             if (add && cgroup_is_descendant(cgrp->css.cgroup,
event->cgrp->css.cgroup)) {
                      /* only when this cgroup is running */
                      cpuctx->cgrp = cgrp;
         }
Quoted 22 lines
Quoted 22 lines
kernel/events/core.c +12 -0
+
+     if (add && ctx->nr_cgroups++)
+             return;
+     else if (!add && --ctx->nr_cgroups)
+             return;
 
+     /* no cgroup running */
+     if (!add)
+             cpuctx->cgrp = NULL;
+
+     cpuctx_entry = &cpuctx->cgrp_cpuctx_entry;
+     if (add)
              list_add(cpuctx_entry, this_cpu_ptr(&cgrp_cpuctx_list));
+     else
              list_del(cpuctx_entry);
 }
 
 #else /* !CONFIG_CGROUP_PERF */

@@ -2311,6 +2325,20 @@ static int  __perf_install_in_context(void *info)
              raw_spin_lock(&task_ctx->lock);
      }
Quoted 9 lines

kernel/events/core.c static int __perf_install_in_context(void *info) · line 2332

Quoted 5 lines
kernel/events/core.c +5 -0
+#ifdef CONFIG_CGROUP_PERF
+     if (is_cgroup_event(event)) {
+             /*
+              * Only care about cgroup events.
+              *

That bit is entirely spurious, if it right after if (is_cgroup_event()), obviously this block is only for cgroup events.

Totally, : )

Quoted 11 lines

kernel/events/core.c static int __perf_install_in_context(void *info) · line 2334

Quoted 2 lines
kernel/events/core.c +2 -0
+              * If only the task belongs to cgroup of this event,
+              * we will continue the installment

And that isn't really english. I think you meant to write something like:

/*
* If the current cgroup doesn't match the event's
* cgroup, we should not try to schedule it. */

Totally again, : ) Thanks

Quoted 13 lines
Quoted 13 lines
kernel/events/core.c +7 -1
+              */
+             struct perf_cgroup *cgrp = perf_cgroup_from_task(current, ctx);
+             reprogram = cgroup_is_descendant(cgrp->css.cgroup,
+                                     event->cgrp->css.cgroup);
+     }
+#endif
+
      if (reprogram) {
              ctx_sched_out(ctx, cpuctx, EVENT_TIME);
              add_event_to_ctx(event, ctx);
--

2.8.4.31.g9ed660f

maintainer
#3 lore raw
Quoted 10 lines · Lin Xiulei
Quoted 9 lines

Review

Quoted 7 lines
(quoted diff) +7 -0
+     /*
+      * if only the cgroup is running on this cpu
+      * and cpuctx->cgrp == NULL (otherwise it would've
+      * been set with running cgroup), we put this cgroup
+      * into cpu context. Or it would case mismatch in
+      * following cgroup events at event_filter_match()
+      */

This is utterly incomprehensible, what?

Quoted 8 lines

Yes, this is bit messy. I should've made it clear. This comment was supposed to explain the reason why I modified the if statement below.

And the logic is

1) when cpuctx-> cgrp is NULL, we __must__ take care of how to set it appropriately, that means, we __have to__ check if the cgroup is running on the cpu

Yes, however, the changelog needs to explain why this was changed, and the above does not explain where the old code was wrong.

In what case do we fail to do 1 with the current code?

The existing nr_cgroups logic ensures we only continue for the first/last cgroup event for that context. Is the problem that if the first cgroup is _not_ of the current cgroup and we correctly do _not_ set cpuctx->cgrp, a second event that _is_ of the right cgroup will then not have the opportunity to set cpuctx->cgrp ?

Quoted 3 lines

2) when cpuctx-> cgrp is __NOT__ NULL. It means cpuctx->cgrp had been set appropriately by cgroup_switch() or list_update_cgroup_event() before. Therefore, We do __nothing__ here

Right, but my variant doubled checked it. If its _not_ NULL it must be the current task's cgrp, otherwise we have a problem. Same difference, more paranoia :-)

But I suppose you're right and we can avoid a bunch of looking up if cpuctx->cgrp is already set.

How is the below patch?

---
Subject: perf/core: Fix installing cgroup events on CPU
From: "leilei.lin" <[email protected]>
Date: Tue, 6 Mar 2018 17:36:37 +0800

There's two problems when installing cgroup events on CPUs: firstly list_update_cgroup_event() only tries to set cpuctx->cgrp for the first event, if that mismatches on @cgrp we'll not try again for later additions.

Secondly, when we install a cgroup event into an active context, only issue an event reprogram when the event matches the current cgroup context. This avoids a pointless event reprogramming.

Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Signed-off-by leilei.lin <[email protected]>
[peterz: Changelog and comments]
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
---
 kernel/events/core.c |   46 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 11 deletions(-)
kernel/events/core.c +35 -11
@@ -937,27 +937,39 @@ list_update_cgroup_event(struct perf_eve
 	if (!is_cgroup_event(event))
 		return;
 
-	if (add && ctx->nr_cgroups++)
-		return;
-	else if (!add && --ctx->nr_cgroups)
-		return;
 	/*
 	 * Because cgroup events are always per-cpu events,
 	 * this will always be called from the right CPU.
 	 */
 	cpuctx = __get_cpu_context(ctx);
-	cpuctx_entry = &cpuctx->cgrp_cpuctx_entry;
-	/* cpuctx->cgrp is NULL unless a cgroup event is active in this CPU .*/
-	if (add) {
+
+	/*
+	 * Since setting cpuctx->cgrp is conditional on the current @cgrp
+	 * matching the event's cgroup, we must do this for every new event,
+	 * because if the first would mismatch, the second would not try again
+	 * and we would leave cpuctx->cgrp unset.
+	 */
+	if (add && !cpuctx->cgrp) {
 		struct perf_cgroup *cgrp = perf_cgroup_from_task(current, ctx);
 
-		list_add(cpuctx_entry, this_cpu_ptr(&cgrp_cpuctx_list));
 		if (cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup))
 			cpuctx->cgrp = cgrp;
-	} else {
-		list_del(cpuctx_entry);
-		cpuctx->cgrp = NULL;
 	}
+
+	if (add && ctx->nr_cgroups++)
+		return;
+	else if (!add && --ctx->nr_cgroups)
+		return;
+
+	/* no cgroup running */
+	if (!add)
+		cpuctx->cgrp = NULL;
+
+	cpuctx_entry = &cpuctx->cgrp_cpuctx_entry;
+	if (add)
+		list_add(cpuctx_entry, this_cpu_ptr(&cgrp_cpuctx_list));
+	else
+		list_del(cpuctx_entry);
 }
 
 #else /* !CONFIG_CGROUP_PERF */

@@ -2491,6 +2503,18 @@ static int  __perf_install_in_context(vo
 		raw_spin_lock(&task_ctx->lock);
 	}
 
+#ifdef CONFIG_CGROUP_PERF
+	if (is_cgroup_event(event)) {
+		/*
+		 * If the current cgroup doesn't match the event's
+		 * cgroup, we should not try to schedule it.
+		 */
+		struct perf_cgroup *cgrp = perf_cgroup_from_task(current, ctx);
+		reprogram = cgroup_is_descendant(cgrp->css.cgroup,
+					event->cgrp->css.cgroup);
+	}
+#endif
+
 	if (reprogram) {
 		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
 		add_event_to_ctx(event, ctx);
author
#4 lore raw

2018-03-12 20:24 GMT+08:00 Peter Zijlstra <[email protected]>:

Quoted 15 lines · Lin Xiulei
Quoted 14 lines
Quoted 10 lines

Review

Quoted 8 lines
(quoted diff) +7 -0
+     /*
+      * if only the cgroup is running on this cpu
+      * and cpuctx->cgrp == NULL (otherwise it would've
+      * been set with running cgroup), we put this cgroup
+      * into cpu context. Or it would case mismatch in
+      * following cgroup events at event_filter_match()
+      */

This is utterly incomprehensible, what?

Yes, this is bit messy. I should've made it clear. This comment was supposed to explain the reason why I modified the if statement below.

Quoted 9 lines
Quoted 5 lines

And the logic is

1) when cpuctx-> cgrp is NULL, we __must__ take care of how to set it appropriately, that means, we __have to__ check if the cgroup is running on the cpu

Yes, however, the changelog needs to explain why this was changed, and the above does not explain where the old code was wrong.

Yes, it's good to be involved in community since you guys give great opinions

Quoted 2 lines

In what case do we fail to do 1 with the current code?

With current code, it doesn't check if the cgroup is running on the CPU. then problem happens when two cgroup events on the same CPU are opened in a row, which means no schedule happen between it

1) event A on cgroup A has no tasks running on this CPU
2) event B on cgroup B has some task running on the CPU

With the current code, cpuctx->cgrp would be set as cgroup A arbitrarily, then it opens event B, cpuctx->cgrp will not be set anymore, and it fails in event_filter_match() because cpuctx->cgrp != cgroup B. We have to wait schedule() happened to correct it. But in this situation, we lost a slight period that measures event B.

Quoted 6 lines

The existing nr_cgroups logic ensures we only continue for the first/last cgroup event for that context. Is the problem that if the first cgroup is _not_ of the current cgroup and we correctly do _not_ set cpuctx->cgrp, a second event that _is_ of the right cgroup will then not have the opportunity to set cpuctx->cgrp ?

Yes, You got it.

Quoted 11 lines
Quoted 3 lines

2) when cpuctx-> cgrp is __NOT__ NULL. It means cpuctx->cgrp had been set appropriately by cgroup_switch() or list_update_cgroup_event() before. Therefore, We do __nothing__ here

Right, but my variant doubled checked it. If its _not_ NULL it must be the current task's cgrp, otherwise we have a problem. Same difference, more paranoia :-)

But I suppose you're right and we can avoid a bunch of looking up if cpuctx->cgrp is already set.

Yes, actually you have a good instinct. But what my further concern is when we should set cpuctx->cgrp to NULL again when one running event is closed ? By this way, we can avoid a bunch of checking cgroup_is_descendant(). I didn't figure it out in this patch, and of course I didn't make it worse. :-)

Quoted 3 lines

How is the below patch?

You're AWESOME : )

Quoted 103 lines

---
Subject: perf/core: Fix installing cgroup events on CPU
From: "leilei.lin" <[email protected]>
Date: Tue, 6 Mar 2018 17:36:37 +0800

There's two problems when installing cgroup events on CPUs: firstly list_update_cgroup_event() only tries to set cpuctx->cgrp for the first event, if that mismatches on @cgrp we'll not try again for later additions.

Secondly, when we install a cgroup event into an active context, only issue an event reprogram when the event matches the current cgroup context. This avoids a pointless event reprogramming.

Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Signed-off-by leilei.lin <[email protected]>
[peterz: Changelog and comments]
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
---
 kernel/events/core.c |   46 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 11 deletions(-)
kernel/events/core.c +35 -11
@@ -937,27 +937,39 @@ list_update_cgroup_event(struct perf_eve
        if (!is_cgroup_event(event))
                return;
 
-       if (add && ctx->nr_cgroups++)
-               return;
-       else if (!add && --ctx->nr_cgroups)
-               return;
        /*
         * Because cgroup events are always per-cpu events,
         * this will always be called from the right CPU.
         */
        cpuctx = __get_cpu_context(ctx);
-       cpuctx_entry = &cpuctx->cgrp_cpuctx_entry;
-       /* cpuctx->cgrp is NULL unless a cgroup event is active in this CPU .*/
-       if (add) {
+
+       /*
+        * Since setting cpuctx->cgrp is conditional on the current @cgrp
+        * matching the event's cgroup, we must do this for every new event,
+        * because if the first would mismatch, the second would not try again
+        * and we would leave cpuctx->cgrp unset.
+        */
+       if (add && !cpuctx->cgrp) {
                struct perf_cgroup *cgrp = perf_cgroup_from_task(current, ctx);
 
-               list_add(cpuctx_entry, this_cpu_ptr(&cgrp_cpuctx_list));
                if (cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup))
                        cpuctx->cgrp = cgrp;
-       } else {
-               list_del(cpuctx_entry);
-               cpuctx->cgrp = NULL;
        }
+
+       if (add && ctx->nr_cgroups++)
+               return;
+       else if (!add && --ctx->nr_cgroups)
+               return;
+
+       /* no cgroup running */
+       if (!add)
+               cpuctx->cgrp = NULL;
+
+       cpuctx_entry = &cpuctx->cgrp_cpuctx_entry;
+       if (add)
+               list_add(cpuctx_entry, this_cpu_ptr(&cgrp_cpuctx_list));
+       else
+               list_del(cpuctx_entry);
 }
 
 #else /* !CONFIG_CGROUP_PERF */

@@ -2491,6 +2503,18 @@ static int  __perf_install_in_context(vo
                raw_spin_lock(&task_ctx->lock);
        }
 
+#ifdef CONFIG_CGROUP_PERF
+       if (is_cgroup_event(event)) {
+               /*
+                * If the current cgroup doesn't match the event's
+                * cgroup, we should not try to schedule it.
+                */
+               struct perf_cgroup *cgrp = perf_cgroup_from_task(current, ctx);
+               reprogram = cgroup_is_descendant(cgrp->css.cgroup,
+                                       event->cgrp->css.cgroup);
+       }
+#endif
+
        if (reprogram) {
                ctx_sched_out(ctx, cpuctx, EVENT_TIME);
                add_event_to_ctx(event, ctx);
#5 lore raw
In reply to [email protected]

kernel/events/core.c list_update_cgroup_event(struct perf_event *event, · line 954

Quoted 12 lines · * [email protected] <[email protected]> wrote:
kernel/events/core.c +7 -0
 	/*
 	 * Because cgroup events are always per-cpu events,
 	 * this will always be called from the right CPU.
 	 */
 
+	/*
+	 * if only the cgroup is running on this cpu
+	 * and cpuctx->cgrp == NULL (otherwise it would've
+	 * been set with running cgroup), we put this cgroup
+	 * into cpu context. Or it would case mismatch in
+	 * following cgroup events at event_filter_match()
+	 */

Beyond making sure that what you comment on makes sense, please also follow existing comment style, which I quoted above.

There's 3 separate mistakes in that paragraph alone.

Thanks,

Ingo

author
#6 lore raw

2018-03-12 15:53 GMT+08:00 Ingo Molnar <[email protected]>:

Quoted 16 lines · * [email protected] <[email protected]> wrote:

kernel/events/core.c list_update_cgroup_event(struct perf_event *event, · line 954

Quoted 13 lines

/*
* Because cgroup events are always per-cpu events,
* this will always be called from the right CPU. */

kernel/events/core.c +7 -0
+     /*
+      * if only the cgroup is running on this cpu
+      * and cpuctx->cgrp == NULL (otherwise it would've
+      * been set with running cgroup), we put this cgroup
+      * into cpu context. Or it would case mismatch in
+      * following cgroup events at event_filter_match()
+      */
Quoted 8 lines

Beyond making sure that what you comment on makes sense, please also follow existing comment style, which I quoted above.

There's 3 separate mistakes in that paragraph alone.

Thanks,

Ingo

Thank Ingo for pointing it out. though I'd try to follow it as possible as I could. I wish I have somewhere to check style with specific documents : )

+ /*

+        * We will set cpuctx->cgrp only if:
+        *   1) cpuctx->cgrp is NULL. Because cpuctx->cgrp was set
+        *      correctly by sched_in(). We don't need to set it again.
+        *   2) cgroup related to this event is running on this CPU.
+        *      Otherwise it will fail in event_filter_match()
+        */

Peter, did I explain that logic clearly and prove it right?

Regards

merge bot
#7 lore raw

[tip:perf/core] perf/core: Fix installing cgroup events on CPU

In reply to [email protected]
perf/core: Fix installing cgroup events on CPU
Commit-ID:  33801b94741d6c3be9713c10aa627477216c21e2
Gitweb:     https://git.kernel.org/tip/33801b94741d6c3be9713c10aa627477216c21e2
Author:     leilei.lin <[email protected]>
AuthorDate: Tue, 6 Mar 2018 17:36:37 +0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 12 Mar 2018 15:28:51 +0100

perf/core: Fix installing cgroup events on CPU

There's two problems when installing cgroup events on CPUs: firstly list_update_cgroup_event() only tries to set cpuctx->cgrp for the first event, if that mismatches on @cgrp we'll not try again for later additions.

Secondly, when we install a cgroup event into an active context, only issue an event reprogram when the event matches the current cgroup context. This avoids a pointless event reprogramming.

Signed-off-by leilei.lin <[email protected]>
[ Improved the changelog and comments. ]
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Stephane Eranian <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Vince Weaver <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 kernel/events/core.c | 46 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 11 deletions(-)
kernel/events/core.c +35 -11
index f98c0f88cc94..969f865f9f1c 100644
@@ -937,27 +937,39 @@ list_update_cgroup_event(struct perf_event *event,
 	if (!is_cgroup_event(event))
 		return;
 
-	if (add && ctx->nr_cgroups++)
-		return;
-	else if (!add && --ctx->nr_cgroups)
-		return;
 	/*
 	 * Because cgroup events are always per-cpu events,
 	 * this will always be called from the right CPU.
 	 */
 	cpuctx = __get_cpu_context(ctx);
-	cpuctx_entry = &cpuctx->cgrp_cpuctx_entry;
-	/* cpuctx->cgrp is NULL unless a cgroup event is active in this CPU .*/
-	if (add) {
+
+	/*
+	 * Since setting cpuctx->cgrp is conditional on the current @cgrp
+	 * matching the event's cgroup, we must do this for every new event,
+	 * because if the first would mismatch, the second would not try again
+	 * and we would leave cpuctx->cgrp unset.
+	 */
+	if (add && !cpuctx->cgrp) {
 		struct perf_cgroup *cgrp = perf_cgroup_from_task(current, ctx);
 
-		list_add(cpuctx_entry, this_cpu_ptr(&cgrp_cpuctx_list));
 		if (cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup))
 			cpuctx->cgrp = cgrp;
-	} else {
-		list_del(cpuctx_entry);
-		cpuctx->cgrp = NULL;
 	}
+
+	if (add && ctx->nr_cgroups++)
+		return;
+	else if (!add && --ctx->nr_cgroups)
+		return;
+
+	/* no cgroup running */
+	if (!add)
+		cpuctx->cgrp = NULL;
+
+	cpuctx_entry = &cpuctx->cgrp_cpuctx_entry;
+	if (add)
+		list_add(cpuctx_entry, this_cpu_ptr(&cgrp_cpuctx_list));
+	else
+		list_del(cpuctx_entry);
 }
 
 #else /* !CONFIG_CGROUP_PERF */

@@ -2489,6 +2501,18 @@ static int  __perf_install_in_context(void *info)
 		raw_spin_lock(&task_ctx->lock);
 	}
 
+#ifdef CONFIG_CGROUP_PERF
+	if (is_cgroup_event(event)) {
+		/*
+		 * If the current cgroup doesn't match the event's
+		 * cgroup, we should not try to schedule it.
+		 */
+		struct perf_cgroup *cgrp = perf_cgroup_from_task(current, ctx);
+		reprogram = cgroup_is_descendant(cgrp->css.cgroup,
+					event->cgrp->css.cgroup);
+	}
+#endif
+
 	if (reprogram) {
 		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
 		add_event_to_ctx(event, ctx);