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

[PATCH RESEND v4] 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 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 <leilei.lin@alibaba-inc.com>
---
 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

 kernel/events/core.c | 50 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 13 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 4df5b69..fd28d61 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -933,31 +933,41 @@ 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))
+	/*
+	 * if only the cgroup is running on this cpu,
+	 * we put/remove this cgroup into cpu context.
+	 * Or it would case mismatch in following cgroup
+	 * events at event_filter_match()
+	 */
+	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 */
@@ -2311,6 +2321,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);
-- 
2.8.4.31.g9ed660f

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

[Jiri Olsa]

On Thu, Feb 08, 2018 at 11:33:44AM +0800, linxiulei@gmail.com wrote:
> From: "leilei.lin" <leilei.lin@alibaba-inc.com>
> 
> 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 <leilei.lin@alibaba-inc.com>
> ---
>  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
> 
>  kernel/events/core.c | 50 +++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4df5b69..fd28d61 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -933,31 +933,41 @@ 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;

I might be missing something, but should this check stay on
the top regardles of the cgroup_is_descendant check below?

you could put NULL into cpuctx->cgrp on context with cgroup
event in the list

thanks,
jirka

>  	/*
>  	 * 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))
> +	/*
> +	 * if only the cgroup is running on this cpu,
> +	 * we put/remove this cgroup into cpu context.
> +	 * Or it would case mismatch in following cgroup
> +	 * events at event_filter_match()
> +	 */
> +	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;

SNIP

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

[Lin Xiulei]

2018-02-08 23:36 GMT+08:00 Jiri Olsa <jolsa@redhat.com>:
>
> On Thu, Feb 08, 2018 at 11:33:44AM +0800, linxiulei@gmail.com wrote:
> > From: "leilei.lin" <leilei.lin@alibaba-inc.com>
> >
> > 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 <leilei.lin@alibaba-inc.com>
> > ---
> >  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
> >
> >  kernel/events/core.c | 50 +++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 37 insertions(+), 13 deletions(-)
> >
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 4df5b69..fd28d61 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -933,31 +933,41 @@ 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;
>
> I might be missing something, but should this check stay on
> the top regardles of the cgroup_is_descendant check below?
>

I don't think so,  if event A on cgroup A is opened and immediately
followed by a event B opened on cgroup B, then
"if (add && ctx->nr_cgroups++)" would __return__ with
 cpuctx->cgrp = cgroup A, that is incorrect.

And previous thread is here https://lkml.org/lkml/2018/1/24/79

>
> you could put NULL into cpuctx->cgrp on context with cgroup
> event in the list
>

what's the harm? It's invoked by perf_remove_from_context() when
an event is ready to be released. And whenever process/cgroup is
scheduled, cpuctx->cgrp will be set

In case that the patch was not sorted well, I put the patched code

```
static inline void
list_update_cgroup_event(struct perf_event *event,
             struct perf_event_context *ctx, bool add)
{
    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);

    /*
     * if only the cgroup is running on this cpu,
     * we put/remove this cgroup into cpu context.
     * Or it would case mismatch in following cgroup
     * events at event_filter_match()
     */
    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);
}
```

thanks

>
> thanks,
> jirka
>
> >       /*
> >        * 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))
> > +     /*
> > +      * if only the cgroup is running on this cpu,
> > +      * we put/remove this cgroup into cpu context.
> > +      * Or it would case mismatch in following cgroup
> > +      * events at event_filter_match()
> > +      */
> > +     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;
>
> SNIP
>

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

[Peter Zijlstra]

On Thu, Feb 08, 2018 at 11:33:44AM +0800, linxiulei@gmail.com wrote:
> From: "leilei.lin" <leilei.lin@alibaba-inc.com>
> 
> 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 <leilei.lin@alibaba-inc.com>
> ---
>  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
> 
>  kernel/events/core.c | 50 +++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4df5b69..fd28d61 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -933,31 +933,41 @@ 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);
>  
> +	/*
> +	 * if only the cgroup is running on this cpu,
> +	 * we put/remove this cgroup into cpu context.
> +	 * Or it would case mismatch in following cgroup
> +	 * events at event_filter_match()
> +	 */
> +	if (cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup)) {
> +		if (add)
>  			cpuctx->cgrp = cgrp;
> +		else
> +			cpuctx->cgrp = NULL;
>  	}

I am still not convinced this is correct.

Suppose we have

   R
  / \
 A   B
    / \
   C

And our current task is of B, and B has an event.

We then install an event in C, if we then destroy our event in C, it
would clear cpuctx->cgrp, which is wrong, since there is still an event
in B.

Simpler still, if B were to have 2 events, and we'd remove one, that
would still clear cpuctx->cgrp, even though there is an event left.

This is the exact issue I pointed out last time, and I still don't see
how it would now be correct.

Northing explains why its ok to have NULL cpuctx->cgrp when there are in
fact still cgroup events on the CPU.

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

[Lin Xiulei]

2018-02-09 18:11 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Thu, Feb 08, 2018 at 11:33:44AM +0800, linxiulei@gmail.com wrote:
>> From: "leilei.lin" <leilei.lin@alibaba-inc.com>
>>
>> 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 <leilei.lin@alibaba-inc.com>
>> ---
>>  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
>>
>>  kernel/events/core.c | 50 +++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 37 insertions(+), 13 deletions(-)
>>
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 4df5b69..fd28d61 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -933,31 +933,41 @@ 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);
>>
>> +     /*
>> +      * if only the cgroup is running on this cpu,
>> +      * we put/remove this cgroup into cpu context.
>> +      * Or it would case mismatch in following cgroup
>> +      * events at event_filter_match()
>> +      */
>> +     if (cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup)) {
>> +             if (add)
>>                       cpuctx->cgrp = cgrp;
>> +             else
>> +                     cpuctx->cgrp = NULL;
>>       }
>
> I am still not convinced this is correct.
>
> Suppose we have
>
>    R
>   / \
>  A   B
>     / \
>    C
>
> And our current task is of B, and B has an event.
>
> We then install an event in C, if we then destroy our event in C, it
> would clear cpuctx->cgrp, which is wrong, since there is still an event
> in B.
>
> Simpler still, if B were to have 2 events, and we'd remove one, that
> would still clear cpuctx->cgrp, even though there is an event left.
>
> This is the exact issue I pointed out last time, and I still don't see
> how it would now be correct.
>
> Northing explains why its ok to have NULL cpuctx->cgrp when there are in
> fact still cgroup events on the CPU.

I got your point now, sorry for misunderstanding it last time. I wanna
confirm it
that logic in __add__ is correct and I'd like to make a slight improvement

```
/* We only have to care about the first time of initiating cpuctx->cgrp,
 * which is when cpuctx->cgrp == NULL, otherwise cpuctx->cgrp was set
 * in perf_cgroup_switch() correctly.
 */
if (add && !cpuctx->cgrp &&
     cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup))
    cpuctx->cgrp = cgrp;
```

And logic in __del__ should be rolled back to previous code that once
ctx->nr_cgroups == 0, set cpuctx->cgrp to NULL.

thanks