lkml.org 
[lkml]   [2018]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH RESEND v4] perf/core: Fix installing cgroup event into cpu
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
>

\
 
 \ /
  Last update: 2018-02-09 02:53    [W:0.064 / U:37.208 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site