lkml.org 
[lkml]   [2020]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH v2] perf/core: install cgroup events to correct cpuctx
    Date


    > 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



    \
     
     \ /
      Last update: 2020-03-06 08:50    [W:8.349 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site