Re: [RFC PATCH] perf/core: fix cpuctx cgrp warning

[Namhyung Kim]

Hello,

On Tue, Mar 8, 2022 at 6:00 AM Chengming Zhou
<zhouchengming@xxxxxxxxxxxxx> wrote:
>
> There is a race problem that can trigger WARN_ON_ONCE(cpuctx->cgrp)
> in perf_cgroup_switch().
>
> CPU1                                    CPU2
> (in context_switch)                     (attach running task)
> perf_cgroup_sched_out(task, next)
>         if (cgrp1 != cgrp2) True
>                                         task->cgroups = xxx
>                                         perf_cgroup_attach()
> perf_cgroup_sched_in(prev, task)
>         if (cgrp1 != cgrp2) False

But perf_cgroup_switch will be synchronized as the context switch
disables the interrupt.  And right, it still can see the task->cgroups
is changing in the middle.

>
> The commit a8d757ef076f ("perf events: Fix slow and broken cgroup
> context switch code") would save cpuctx switch in/out when the
> perf_cgroup of "prev" and "next" are the same.
>
> But perf_cgroup of task can change in concurrent with context_switch.
> If cgrp1 == cgrp2 in sched_out(), cpuctx won't do switch out, then
> task perf_cgroup changed cause cgrp1 != cgrp2 in sched_in(), cpuctx
> will do switch in, and trigger WARN_ON_ONCE(cpuctx->cgrp).
>
> The perf_cgroup of "prev" and "next" can be changed at any time, so we
> first have to combine perf_cgroup_sched_in() into perf_cgroup_sched_out(),
> so we can get a consistent value of condition (cgrp1 == cgrp2).
>
> And we introduce a percpu "cpu_perf_cgroups" to track the current used
> perf_cgroup, instead of using the unstable perf_cgroup of "prev", which
> maybe not the cpuctx->cgrp we used to schedule cgroup events on cpu.

Is this really needed?  I think the warning comes because the two
cgroups were the same when in sched-out, but they became
different when in sched-in.  So just combining sched-in/out should
be ok, isn't it?

>
> Fixes: a8d757ef076f ("perf events: Fix slow and broken cgroup context
> switch code")
> Signed-off-by: Chengming Zhou <zhouchengming@xxxxxxxxxxxxx>
> ---
>  kernel/events/core.c | 95 +++++++++++---------------------------------
>  1 file changed, 23 insertions(+), 72 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 6859229497b1..f3bc2841141f 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -826,6 +826,7 @@ perf_cgroup_set_timestamp(struct task_struct *task,
>         }
>  }
>
> +static DEFINE_PER_CPU(struct perf_cgroup *, cpu_perf_cgroups);
>  static DEFINE_PER_CPU(struct list_head, cgrp_cpuctx_list);
>
>  #define PERF_CGROUP_SWOUT      0x1 /* cgroup switch out every event */
> @@ -837,8 +838,9 @@ static DEFINE_PER_CPU(struct list_head, cgrp_cpuctx_list);
>   * mode SWOUT : schedule out everything
>   * mode SWIN : schedule in based on cgroup for next

You can remove this comment now.

>   */
> -static void perf_cgroup_switch(struct task_struct *task, int mode)
> +static void perf_cgroup_switch(struct task_struct *task)
>  {
> +       struct perf_cgroup *cgrp;
>         struct perf_cpu_context *cpuctx, *tmp;
>         struct list_head *list;
>         unsigned long flags;
> @@ -849,6 +851,9 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
>          */
>         local_irq_save(flags);
>
> +       cgrp = perf_cgroup_from_task(task, NULL);
> +       __this_cpu_write(cpu_perf_cgroups, cgrp);
> +
>         list = this_cpu_ptr(&cgrp_cpuctx_list);
>         list_for_each_entry_safe(cpuctx, tmp, list, cgrp_cpuctx_entry) {
>                 WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0);
> @@ -856,28 +861,15 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
>                 perf_ctx_lock(cpuctx, cpuctx->task_ctx);
>                 perf_pmu_disable(cpuctx->ctx.pmu);
>
> -               if (mode & PERF_CGROUP_SWOUT) {
> -                       cpu_ctx_sched_out(cpuctx, EVENT_ALL);
> -                       /*
> -                        * must not be done before ctxswout due
> -                        * to event_filter_match() in event_sched_out()

Unrelated, but I don't see the event_filter_match() in
event_sched_out() anymore.  Does it sched-out all
non-cgroup cpu events here?

> -                        */
> -                       cpuctx->cgrp = NULL;
> -               }
> +               cpu_ctx_sched_out(cpuctx, EVENT_ALL);
> +               /*
> +                * must not be done before ctxswout due
> +                * to event_filter_match() in event_sched_out()
> +                */
> +               cpuctx->cgrp = cgrp;

Maybe we can check cpuctx->cgrp is the same as task's
cgroup before accessing the pmu.  As in the commit message
it can call perf_cgroup_switch() after the context switch so
the cgroup events might be scheduled already.

Thanks,
Namhyung


> +
> +               cpu_ctx_sched_in(cpuctx, EVENT_ALL, task);
>
> -               if (mode & PERF_CGROUP_SWIN) {
> -                       WARN_ON_ONCE(cpuctx->cgrp);
> -                       /*
> -                        * set cgrp before ctxsw in to allow
> -                        * event_filter_match() to not have to pass
> -                        * task around
> -                        * we pass the cpuctx->ctx to perf_cgroup_from_task()
> -                        * because cgorup events are only per-cpu
> -                        */
> -                       cpuctx->cgrp = perf_cgroup_from_task(task,
> -                                                            &cpuctx->ctx);
> -                       cpu_ctx_sched_in(cpuctx, EVENT_ALL, task);
> -               }
>                 perf_pmu_enable(cpuctx->ctx.pmu);
>                 perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
>         }