From: Namhyung Kim <namhyung@kernel.org>
To: Chengming Zhou <zhouchengming@bytedance.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>,
Stephane Eranian <eranian@google.com>,
linux-perf-users <linux-perf-users@vger.kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
duanxiongchun@bytedance.com, songmuchun@bytedance.com
Subject: Re: [PATCH v2 2/6] perf/core: Introduce percpu perf_cgroup
Date: Tue, 22 Mar 2022 15:18:45 -0700 [thread overview]
Message-ID: <CAM9d7cjwuQmuxwMG7HP3QDw0ckKcUqfT5A8JGg2JkcrZuQhWUg@mail.gmail.com> (raw)
In-Reply-To: <20220322120834.98637-3-zhouchengming@bytedance.com>
On Tue, Mar 22, 2022 at 5:10 AM Chengming Zhou
<zhouchengming@bytedance.com> wrote:
>
> Although we don't have incosistency problem any more, we can
> have other problem like:
>
> CPU1 CPU2
> (in context_switch) (attach running task)
> prev->cgroups = cgrp2
> perf_cgroup_sched_switch(prev, next)
> cgrp2 == cgrp2 is True
>
> If perf_cgroup of prev task changes from cgrp1 to cgrp2,
> perf_cgroup_sched_switch() will skip perf_cgroup_switch(),
> so the CPU would still schedule the cgrp1 events, but we should
> schedule the cgrp2 events.
Ah ok, now I see the problem in changing prev->cgroup too.
>
> The reason of this problem is that we shouldn't use the changeable
> prev->cgroups to decide whether skip perf_cgroup_switch().
>
> This patch introduces a percpu perf_cgroup to cache the perf_cgroup
> that scheduled in cpuctxes, which later used to compare with the
> perf_cgroup of next task to decide whether skip perf_cgroup_switch().
>
> Since the perf_cgroup_switch() can be called after the context switch,
> the cgroup events might be scheduled already. So we put the comparison
> of perf_cgroups in perf_cgroup_switch(), and delete the unused function
> perf_cgroup_sched_switch().
>
> We must clear the percpu perf_cgroup cache when the last cgroup event
> disabled.
>
> Fixes: a8d757ef076f ("perf events: Fix slow and broken cgroup context switch code")
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
> kernel/events/core.c | 63 ++++++++++++++++----------------------------
> 1 file changed, 22 insertions(+), 41 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 8b5cf2aedfe6..848a3bfa9513 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_cgroup);
> static DEFINE_PER_CPU(struct list_head, cgrp_cpuctx_list);
>
> /*
> @@ -833,6 +834,7 @@ static DEFINE_PER_CPU(struct list_head, cgrp_cpuctx_list);
> */
> 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;
> @@ -843,11 +845,21 @@ static void perf_cgroup_switch(struct task_struct *task)
> */
> local_irq_save(flags);
>
> + cgrp = perf_cgroup_from_task(task, NULL);
> + if (cgrp == __this_cpu_read(cpu_perf_cgroup))
> + goto out;
> +
> + __this_cpu_write(cpu_perf_cgroup, 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);
>
> perf_ctx_lock(cpuctx, cpuctx->task_ctx);
> +
> + if (cpuctx->cgrp == cgrp)
Missing perf_ctx_unlock().
Thanks,
Namhyung
> + continue;
> +
> perf_pmu_disable(cpuctx->ctx.pmu);
>
> cpu_ctx_sched_out(cpuctx, EVENT_ALL);
> @@ -855,14 +867,11 @@ static void perf_cgroup_switch(struct task_struct *task)
> * must not be done before ctxswout due
> * to event_filter_match() in event_sched_out()
> */
> - cpuctx->cgrp = perf_cgroup_from_task(task,
> - &cpuctx->ctx);
> + cpuctx->cgrp = 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 cgroup events are only per-cpu
> */
> cpu_ctx_sched_in(cpuctx, EVENT_ALL, task);
>
> @@ -870,35 +879,10 @@ static void perf_cgroup_switch(struct task_struct *task)
> perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
> }
>
> +out:
> local_irq_restore(flags);
> }
next prev parent reply other threads:[~2022-03-22 22:19 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-22 12:08 [PATCH v2 0/6] perf/core: Fixes and cleanup for cgroup events Chengming Zhou
2022-03-22 12:08 ` [PATCH v2 1/6] perf/core: Fix incosistency between cgroup sched_out and sched_in Chengming Zhou
2022-03-22 12:59 ` Peter Zijlstra
2022-03-22 13:38 ` [External] " Chengming Zhou
2022-03-22 14:54 ` Peter Zijlstra
2022-03-22 15:16 ` Chengming Zhou
2022-03-22 15:28 ` Chengming Zhou
2022-03-22 22:06 ` Namhyung Kim
2022-03-23 8:11 ` Peter Zijlstra
2022-03-22 12:08 ` [PATCH v2 2/6] perf/core: Introduce percpu perf_cgroup Chengming Zhou
2022-03-22 13:01 ` Peter Zijlstra
2022-03-22 16:33 ` [External] " Chengming Zhou
2022-03-23 8:13 ` Peter Zijlstra
2022-03-23 12:58 ` Chengming Zhou
2022-03-22 22:21 ` Namhyung Kim
2022-03-22 22:18 ` Namhyung Kim [this message]
2022-03-23 1:27 ` [Phishing Risk] [External] " Chengming Zhou
2022-03-23 12:51 ` Peter Zijlstra
2022-03-23 13:07 ` [External] " Chengming Zhou
2022-03-23 13:17 ` Peter Zijlstra
2022-03-23 13:37 ` Chengming Zhou
2022-03-23 14:05 ` Peter Zijlstra
2022-03-23 15:44 ` Chengming Zhou
2022-03-22 12:08 ` [PATCH v2 3/6] perf/core: Don't pass task around when ctx sched in Chengming Zhou
2022-03-22 13:01 ` Peter Zijlstra
2022-03-22 12:08 ` [PATCH v2 4/6] perf/core: Use stable cpuctx->cgrp when update perf cgroup time Chengming Zhou
2022-03-22 13:03 ` Peter Zijlstra
2022-03-22 12:08 ` [PATCH v2 5/6] perf/core: Always set cpuctx cgrp when enable cgroup event Chengming Zhou
2022-03-22 12:08 ` [PATCH v2 6/6] perf/core: Don't need event_filter_match when merge_sched_in() Chengming Zhou
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAM9d7cjwuQmuxwMG7HP3QDw0ckKcUqfT5A8JGg2JkcrZuQhWUg@mail.gmail.com \
--to=namhyung@kernel.org \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=duanxiongchun@bytedance.com \
--cc=eranian@google.com \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=songmuchun@bytedance.com \
--cc=zhouchengming@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).