[PATCH v2] perf/core: Fix installing cgroup event into cpu

[PATCH v2] perf/core: Fix installing cgroup event into cpu

[linxiulei]

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

Re: [PATCH v2] perf/core: Fix installing cgroup event into cpu

[Peter Zijlstra]

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.

Re: [PATCH v2] perf/core: Fix installing cgroup event into cpu

[Lin Xiulei]

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()

Re: [PATCH v2] perf/core: Fix installing cgroup event into cpu

[Peter Zijlstra]

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.

Re: [PATCH v2] perf/core: Fix installing cgroup event into cpu

[Lin Xiulei]

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?". : )

Re: [PATCH v2] perf/core: Fix installing cgroup event into cpu

[Peter Zijlstra]

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.

Re: [PATCH v2] perf/core: Fix installing cgroup event into cpu

[Lin Xiulei]

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.