perf/core: Fix installing cgroup event into cpu
PATCH v2 1/1 perf/core: Fix installing cgroup event into cpu
From: "leilei.lin" <[email protected]>
Do not install cgroup event into the CPU context 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.
--- kernel/events/core.c | 44 +++++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-)
kernel/events/core.c +31 -13
@@ -933,31 +933,36 @@ 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); -list_add (cpuctx_entry,this_cpu_ptr (&cgrp_cpuctx_list)); -if (cgroup_is_descendant (cgrp->css.cgroup, event->cgrp->css.cgroup)) +/* cpuctx->cgrp is NULL unless a cgroup event is running in this CPU .*/ +if (cgroup_is_descendant (cgrp->css.cgroup, event->cgrp->css.cgroup)) { +if (add) cpuctx->cgrp = cgrp; - }else { -list_del (cpuctx_entry); - cpuctx->cgrp =NULL ; +else + cpuctx->cgrp =NULL ; } + +if (add && ctx->nr_cgroups++) +return ; +else if (!add && --ctx->nr_cgroups) +return ; + + 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 */
@@ -2284,6 +2289,7 @@ static int __perf_install_in_context(void *info)struct perf_event_context *ctx = event->ctx;struct perf_cpu_context *cpuctx =__get_cpu_context (ctx);struct perf_event_context *task_ctx = cpuctx->task_ctx; +struct perf_cgroup *cgrp;bool reprogram = true;int ret =0 ;
@@ -2311,6 +2317,18 @@ static int __perf_install_in_context(void *info)raw_spin_lock (&task_ctx->lock); } +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 +*/ + cgrp =perf_cgroup_from_task (current, ctx); + reprogram =cgroup_is_descendant (cgrp->css.cgroup, + event->cgrp->css.cgroup); + } +if (reprogram) {ctx_sched_out (ctx, cpuctx,EVENT_TIME );add_event_to_ctx (event, ctx);
Signature · 1 line
2.8.4.31.g9ed660f
Peter Zijlstra
maintainerQuoted 8 lines · On Wed, Jan 24, 2018 at 03:50:10PM +0800, [email protected] wrote:
From: "leilei.lin" <[email protected]>
Do not install cgroup event into the CPU context 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 changelog doesn't really cover the extend of the changes done.
Quoted 34 lines
Signed-off-by leilei.lin <[email protected]>--- kernel/events/core.c | 44 +++++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-)kernel/events/core.c +9
-0@@ -933,31 +933,36 @@ 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. */ cpuctx =__get_cpu_context (ctx); + cgrp =perf_cgroup_from_task (current, ctx); +/* cpuctx->cgrp is NULL unless a cgroup event is running in this CPU .*/ +if (cgroup_is_descendant (cgrp->css.cgroup, event->cgrp->css.cgroup)) { +if (add) cpuctx->cgrp = cgrp; +else + cpuctx->cgrp =NULL ; } + +if (add && ctx->nr_cgroups++)
kernel/events/core.c list_update_cgroup_event(struct perf_event *event, · line 966
Quoted 10 lines
kernel/events/core.c +9
-0+return ; + else if (!add && --ctx->nr_cgroups) +return ; + + cpuctx_entry = &cpuctx->cgrp_cpuctx_entry; +if (add) +list_add (cpuctx_entry,this_cpu_ptr (&cgrp_cpuctx_list)); +else +list_del (cpuctx_entry); }
I'm a little confused; you unconditionally set cpuctx->cgrp for every add/delete.
So if we have >1 cgroup events on, and we remove one, you still clear cpuctx->cgrp, that seems wrong.
Why did you change that? The Changelog doesn't include enough clues for me to know what you were trying to do.
2018-01-24 16:20 GMT+08:00 Peter Zijlstra <[email protected]>:
Quoted 12 lines · On Wed, Jan 24, 2018 at 03:50:10PM +0800, [email protected] wrote:
Quoted 8 lines
From: "leilei.lin" <[email protected]>
Do not install cgroup event into the CPU context 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 changelog doesn't really cover the extend of the changes done.
See below please
Quoted 49 lines
Quoted 35 lines
Signed-off-by leilei.lin <[email protected]>--- kernel/events/core.c | 44 +++++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-)kernel/events/core.c +9
-0@@ -933,31 +933,36 @@ 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. */ cpuctx =__get_cpu_context (ctx); + cgrp =perf_cgroup_from_task (current, ctx); +/* cpuctx->cgrp is NULL unless a cgroup event is running in this CPU .*/ +if (cgroup_is_descendant (cgrp->css.cgroup, event->cgrp->css.cgroup)) { +if (add) cpuctx->cgrp = cgrp; +else + cpuctx->cgrp =NULL ; } + +if (add && ctx->nr_cgroups++)kernel/events/core.c list_update_cgroup_event(struct perf_event *event, · line 966
Quoted 10 lines
kernel/events/core.c +9
-0+return ; + else if (!add && --ctx->nr_cgroups) +return ; + + cpuctx_entry = &cpuctx->cgrp_cpuctx_entry; +if (add) +list_add (cpuctx_entry,this_cpu_ptr (&cgrp_cpuctx_list)); +else +list_del (cpuctx_entry); }I'm a little confused; you unconditionally set
cpuctx->cgrpfor every add/delete.
Quoted 5 lines
So if we have >1 cgroup events on, and we remove one, you still clear
cpuctx->cgrp, that seems wrong.Why did you change that? The Changelog doesn't include enough clues for me to know what you were trying to do.
if we have > 1 cgroup events on, whenever a cgroup was really to be deleted, only if
this cgroup is the same as the cgroup running on this cpu, I would clear cpuctx->cgrp.
Reverse is the same.
Here is the problem, previous version didn't set cpuctx->cgrp anymore if ctx->nr_cgroups > 1,
which cases a second event would not be activated immediately because cpuctx->cgrp isn't equal
to event->cgrp at event_filter_match()
Peter Zijlstra
maintainerQuoted 49 lines · Lin Xiulei
Quoted 49 lines
Quoted 32 lines
kernel/events/core.c | 44 +++++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-)kernel/events/core.c +9
-0@@ -933,31 +933,36 @@ 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. */ cpuctx =__get_cpu_context (ctx); + cgrp =perf_cgroup_from_task (current, ctx); +/* cpuctx->cgrp is NULL unless a cgroup event is running in this CPU .*/ +if (cgroup_is_descendant (cgrp->css.cgroup, event->cgrp->css.cgroup)) { +if (add) cpuctx->cgrp = cgrp; +else + cpuctx->cgrp =NULL ; } + +if (add && ctx->nr_cgroups++)kernel/events/core.c list_update_cgroup_event(struct perf_event *event, · line 966
Quoted 10 lines
kernel/events/core.c +9
-0+return ; + else if (!add && --ctx->nr_cgroups) +return ; + + cpuctx_entry = &cpuctx->cgrp_cpuctx_entry; +if (add) +list_add (cpuctx_entry,this_cpu_ptr (&cgrp_cpuctx_list)); +else +list_del (cpuctx_entry); }I'm a little confused; you unconditionally set
cpuctx->cgrpfor every add/delete.So if we have >1 cgroup events on, and we remove one, you still clear
cpuctx->cgrp, that seems wrong.
Quoted 6 lines
Quoted 2 lines
Why did you change that? The Changelog doesn't include enough clues for me to know what you were trying to do.
if we have > 1 cgroup events on, whenever a cgroup was really to be deleted, only if this cgroup is the same as the cgroup running on this cpu, I would clear
cpuctx->cgrp.
But that might still be too early, we might still have more cgroup events active.
What goes wrong if we leave it set?
Quoted 4 lines
Here is the problem, previous version didn't set
cpuctx->cgrpanymore ifctx->nr_cgroups> 1, which cases a second event would not be activated immediately becausecpuctx->cgrpisn't equal toevent->cgrpatevent_filter_match()
OK, I think I can see that happening. Please clarify the Changelog and maybe put a comment in the code as well.
2018-01-24 17:14 GMT+08:00 Peter Zijlstra <[email protected]>:
Quoted 62 lines · Lin Xiulei
Quoted 49 lines
Quoted 49 lines
Quoted 32 lines
kernel/events/core.c | 44 +++++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-)kernel/events/core.c +9
-0@@ -933,31 +933,36 @@ 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. */ cpuctx =__get_cpu_context (ctx); + cgrp =perf_cgroup_from_task (current, ctx); +/* cpuctx->cgrp is NULL unless a cgroup event is running in this CPU .*/ +if (cgroup_is_descendant (cgrp->css.cgroup, event->cgrp->css.cgroup)) { +if (add) cpuctx->cgrp = cgrp; +else + cpuctx->cgrp =NULL ; } + +if (add && ctx->nr_cgroups++)kernel/events/core.c list_update_cgroup_event(struct perf_event *event, · line 966
Quoted 10 lines
kernel/events/core.c +9
-0+return ; + else if (!add && --ctx->nr_cgroups) +return ; + + cpuctx_entry = &cpuctx->cgrp_cpuctx_entry; +if (add) +list_add (cpuctx_entry,this_cpu_ptr (&cgrp_cpuctx_list)); +else +list_del (cpuctx_entry); }I'm a little confused; you unconditionally set
cpuctx->cgrpfor every add/delete.So if we have >1 cgroup events on, and we remove one, you still clear
cpuctx->cgrp, that seems wrong.Quoted 6 lines
Quoted 2 lines
Why did you change that? The Changelog doesn't include enough clues for me to know what you were trying to do.
if we have > 1 cgroup events on, whenever a cgroup was really to be deleted, only if this cgroup is the same as the cgroup running on this cpu, I would clear
cpuctx->cgrp.But that might still be too early, we might still have more cgroup events active.
What goes wrong if we leave it set?
Quoted 7 lines
Quoted 4 lines
Here is the problem, previous version didn't set
cpuctx->cgrpanymore ifctx->nr_cgroups> 1, which cases a second event would not be activated immediately becausecpuctx->cgrpisn't equal toevent->cgrpatevent_filter_match()OK, I think I can see that happening. Please clarify the Changelog and maybe put a comment in the code as well.
Sure, and I consider this "OK" works for "What goes wrong if we leave it set?". : )
Peter Zijlstra
maintainerQuoted 2 lines · Lin Xiulei
Sure, and I consider this "OK" works for "What goes wrong if we leave it set?". : )
It would be good if you inspect the code for the case of leaving cpuctx->cgrp set with no cgroup events left -- AND -- put a blurb about what you found in your new Changelog.
I suspect it works out and something like perf_cgroup_switch() will fix things up for us later, but double check and test.
2018-01-24 17:46 GMT+08:00 Peter Zijlstra <[email protected]>:
Quoted 8 lines · Lin Xiulei
Quoted 2 lines
Sure, and I consider this "OK" works for "What goes wrong if we leave it set?". : )
It would be good if you inspect the code for the case of leaving
cpuctx->cgrpset with no cgroup events left -- AND -- put a blurb about what you found in your new Changelog.
I have some test cases for this issue, I don't know if it's good to put those in changelog
reproduction as below
Step 1
Create program for measuring, write below to the file d.py
```
while True:
sumup = 0
for i in range(10000000):
sumup += i
```Step 2
Create cgroup path and run relative program
``` mkdir /sys/fs/cgroup/perf_event/test1 mkdir /sys/fs/cgroup/perf_event/test2 python d.py & echo $! > /sys/fs/cgroup/perf_event/test1/cgroup.procs python d.py & echo $! > /sys/fs/cgroup/perf_event/test2/cgroup.procs ```
Step 3
``` perf stat -e cycles -G test1 -e cycles -G test2 -a sleep 1 ```
You would see output like below
```
Performance counter stats for 'system wide':
2,161,022,123 cycles test1
138,626,073 cycles test2
1.001858328 seconds time elapsed
```The result of test2 is much less than test1, which happens commonly. Just because of what I mentioned above that a second event couldn't be activated immediately, that cases some loss
Quoted 2 lines
I suspect it works out and something like
perf_cgroup_switch()will fix things up for us later, but double check and test.
exactly, the case above wouldn't have any result if no perf_cgroup_switch() happened.