Messages in this thread | | | From | Song Liu <> | Subject | Re: [PATCH v2] perf/core: install cgroup events to correct cpuctx | Date | Wed, 18 Mar 2020 07:07:29 +0000 |
| |
Hi Peter,
> On Mar 5, 2020, at 11:48 PM, Song Liu <songliubraving@fb.com> wrote: > > > >> On Jan 24, 2020, at 1:15 AM, Peter Zijlstra <peterz@infradead.org> wrote: >> >> On Wed, Jan 22, 2020 at 11:50:27AM -0800, Song Liu wrote: >>> cgroup events are always installed in the cpuctx. However, when it is not >>> installed via IPI, list_update_cgroup_event() adds it to cpuctx of current >>> CPU, which triggers the following with CONFIG_DEBUG_LIST: >>> >> >>> [ 31.777570] list_add double add: new=ffff888ff7cf0db0, prev=ffff888ff7ce82f0, next=ffff888ff7cf0db0. >> >>> To reproduce this, we can simply run: >>> perf stat -e cs -a & >>> perf stat -e cs -G anycgroup >>> >>> Fix this by installing it to cpuctx that contains event->ctx, and the >>> proper cgrp_cpuctx_list. >>> >>> Fixes: db0503e4f675 ("perf/core: Optimize perf_install_in_event()") >>> Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org> >>> Cc: Andi Kleen <andi@firstfloor.org> >>> Cc: Peter Zijlstra (Intel) <peterz@infradead.org> >>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> >>> Cc: Jiri Olsa <jolsa@redhat.com> >>> Cc: Namhyung Kim <namhyung@kernel.org> >>> Cc: Thomas Gleixner <tglx@linutronix.de> >>> Signed-off-by: Song Liu <songliubraving@fb.com> >> >> Thanks! > > I just realized this won't fully fix the problem, because later in > list_update_cgroup_event() we use "current": > > struct perf_cgroup *cgrp = perf_cgroup_from_task(current, ctx);
Could you please share your thoughts on this? I think we cannot use current in list_update_cgroup_event(), unless we call it on the target CPU.
Thanks, Song
> > I don't have a good idea to fix this cleanly. How about we just use IPI > to install cgroup events (like v1): > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index a1f8bde19b56..36e8fe27e2a1 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -2682,14 +2682,18 @@ perf_install_in_context(struct perf_event_context *ctx, > smp_store_release(&event->ctx, ctx); > > /* > - * perf_event_attr::disabled events will not run and can be initialized > - * without IPI. Except when this is the first event for the context, in > - * that case we need the magic of the IPI to set ctx->is_active. > + * perf_event_attr::disabled events will not run and can be > + * initialized without IPI. Except: > + * 1. when this is the first event for the context, in that case > + * we need the magic of the IPI to set ctx->is_active; > + * 2. cgroup event in OFF state, because it is installed in the > + * cpuctx. > * > * The IOC_ENABLE that is sure to follow the creation of a disabled > * event will issue the IPI and reprogram the hardware. > */ > - if (__perf_effective_state(event) == PERF_EVENT_STATE_OFF && ctx->nr_events) { > + if (__perf_effective_state(event) == PERF_EVENT_STATE_OFF && > + !is_cgroup_event(event) && ctx->nr_events) { > raw_spin_lock_irq(&ctx->lock); > if (ctx->task == TASK_TOMBSTONE) { > raw_spin_unlock_irq(&ctx->lock); > > Thanks, > Song > > >
| |