* [PATCH v2] perf/core: Fix installing cgroup event into cpu
@ 2018-01-24 7:50 linxiulei
2018-01-24 8:20 ` Peter Zijlstra
0 siblings, 1 reply; 7+ messages in thread
From: linxiulei @ 2018-01-24 7:50 UTC (permalink / raw)
To: peterz, jolsa, mingo, acme, alexander.shishkin, linux-kernel,
tglx, eranian, torvalds, linux-perf-users, brendan.d.gregg
Cc: yang_oliver, jinli.zjl, leilei.lin
From: "leilei.lin" <leilei.lin@alibaba-inc.com>
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.
Signed-off-by: leilei.lin <leilei.lin@alibaba-inc.com>
---
kernel/events/core.c | 44 +++++++++++++++++++++++++++++++-------------
1 file changed, 31 insertions(+), 13 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 4df5b69..f766b60 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -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);
--
2.8.4.31.g9ed660f
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] perf/core: Fix installing cgroup event into cpu
2018-01-24 7:50 [PATCH v2] perf/core: Fix installing cgroup event into cpu linxiulei
@ 2018-01-24 8:20 ` Peter Zijlstra
2018-01-24 8:32 ` Lin Xiulei
0 siblings, 1 reply; 7+ messages in thread
From: Peter Zijlstra @ 2018-01-24 8:20 UTC (permalink / raw)
To: linxiulei
Cc: jolsa, mingo, acme, alexander.shishkin, linux-kernel, tglx,
eranian, torvalds, linux-perf-users, brendan.d.gregg,
yang_oliver, jinli.zjl, leilei.lin
On Wed, Jan 24, 2018 at 03:50:10PM +0800, linxiulei@gmail.com wrote:
> From: "leilei.lin" <leilei.lin@alibaba-inc.com>
>
> 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.
> Signed-off-by: leilei.lin <leilei.lin@alibaba-inc.com>
> ---
> kernel/events/core.c | 44 +++++++++++++++++++++++++++++++-------------
> 1 file changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4df5b69..f766b60 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -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++)
> + 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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] perf/core: Fix installing cgroup event into cpu
2018-01-24 8:20 ` Peter Zijlstra
@ 2018-01-24 8:32 ` Lin Xiulei
2018-01-24 9:14 ` Peter Zijlstra
0 siblings, 1 reply; 7+ messages in thread
From: Lin Xiulei @ 2018-01-24 8:32 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Jiri Olsa, mingo, acme, alexander.shishkin, linux-kernel, tglx,
Stephane Eranian, torvalds, linux-perf-users, Brendan Gregg,
yang_oliver, jinli.zjl, leilei.lin
2018-01-24 16:20 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Wed, Jan 24, 2018 at 03:50:10PM +0800, linxiulei@gmail.com wrote:
>> From: "leilei.lin" <leilei.lin@alibaba-inc.com>
>>
>> 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
>
>> Signed-off-by: leilei.lin <leilei.lin@alibaba-inc.com>
>> ---
>> kernel/events/core.c | 44 +++++++++++++++++++++++++++++++-------------
>> 1 file changed, 31 insertions(+), 13 deletions(-)
>>
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 4df5b69..f766b60 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -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++)
>> + 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.
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()
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] perf/core: Fix installing cgroup event into cpu
2018-01-24 8:32 ` Lin Xiulei
@ 2018-01-24 9:14 ` Peter Zijlstra
2018-01-24 9:19 ` Lin Xiulei
0 siblings, 1 reply; 7+ messages in thread
From: Peter Zijlstra @ 2018-01-24 9:14 UTC (permalink / raw)
To: Lin Xiulei
Cc: Jiri Olsa, mingo, acme, alexander.shishkin, linux-kernel, tglx,
Stephane Eranian, torvalds, linux-perf-users, Brendan Gregg,
yang_oliver, jinli.zjl, leilei.lin
On Wed, Jan 24, 2018 at 04:32:38PM +0800, Lin Xiulei wrote:
> >> kernel/events/core.c | 44 +++++++++++++++++++++++++++++++-------------
> >> 1 file changed, 31 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/kernel/events/core.c b/kernel/events/core.c
> >> index 4df5b69..f766b60 100644
> >> --- a/kernel/events/core.c
> >> +++ b/kernel/events/core.c
> >> @@ -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++)
> >> + 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.
>
> 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?
> 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()
OK, I think I can see that happening. Please clarify the Changelog and
maybe put a comment in the code as well.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] perf/core: Fix installing cgroup event into cpu
2018-01-24 9:14 ` Peter Zijlstra
@ 2018-01-24 9:19 ` Lin Xiulei
2018-01-24 9:46 ` Peter Zijlstra
0 siblings, 1 reply; 7+ messages in thread
From: Lin Xiulei @ 2018-01-24 9:19 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Jiri Olsa, mingo, acme, alexander.shishkin, linux-kernel, tglx,
Stephane Eranian, torvalds, linux-perf-users, Brendan Gregg,
yang_oliver, jinli.zjl, leilei.lin
2018-01-24 17:14 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Wed, Jan 24, 2018 at 04:32:38PM +0800, Lin Xiulei wrote:
>> >> kernel/events/core.c | 44 +++++++++++++++++++++++++++++++-------------
>> >> 1 file changed, 31 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> >> index 4df5b69..f766b60 100644
>> >> --- a/kernel/events/core.c
>> >> +++ b/kernel/events/core.c
>> >> @@ -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++)
>> >> + 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.
>>
>> 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?
>
>> 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()
>
> 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?". : )
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] perf/core: Fix installing cgroup event into cpu
2018-01-24 9:19 ` Lin Xiulei
@ 2018-01-24 9:46 ` Peter Zijlstra
2018-01-24 9:59 ` Lin Xiulei
0 siblings, 1 reply; 7+ messages in thread
From: Peter Zijlstra @ 2018-01-24 9:46 UTC (permalink / raw)
To: Lin Xiulei
Cc: Jiri Olsa, mingo, acme, alexander.shishkin, linux-kernel, tglx,
Stephane Eranian, torvalds, linux-perf-users, Brendan Gregg,
yang_oliver, jinli.zjl, leilei.lin
On Wed, Jan 24, 2018 at 05:19:34PM +0800, Lin Xiulei wrote:
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] perf/core: Fix installing cgroup event into cpu
2018-01-24 9:46 ` Peter Zijlstra
@ 2018-01-24 9:59 ` Lin Xiulei
0 siblings, 0 replies; 7+ messages in thread
From: Lin Xiulei @ 2018-01-24 9:59 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Jiri Olsa, mingo, acme, alexander.shishkin, linux-kernel, tglx,
Stephane Eranian, torvalds, linux-perf-users, Brendan Gregg,
yang_oliver, jinli.zjl, leilei.lin
2018-01-24 17:46 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Wed, Jan 24, 2018 at 05:19:34PM +0800, Lin Xiulei wrote:
>> 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 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
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-01-24 9:59 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-24 7:50 [PATCH v2] perf/core: Fix installing cgroup event into cpu linxiulei
2018-01-24 8:20 ` Peter Zijlstra
2018-01-24 8:32 ` Lin Xiulei
2018-01-24 9:14 ` Peter Zijlstra
2018-01-24 9:19 ` Lin Xiulei
2018-01-24 9:46 ` Peter Zijlstra
2018-01-24 9:59 ` Lin Xiulei
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.