Messages in this thread Patch in this message | | | From | Song Liu <> | Subject | Re: [PATCH v2] perf/core: install cgroup events to correct cpuctx | Date | Fri, 6 Mar 2020 07:48:56 +0000 |
| |
> 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);
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
| |