From: Peter Zijlstra <peterz@infradead.org>
To: Chengming Zhou <zhouchengming@bytedance.com>
Cc: mingo@redhat.com, acme@kernel.org, mark.rutland@arm.com,
alexander.shishkin@linux.intel.com, jolsa@kernel.org,
namhyung@kernel.org, eranian@google.com,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
duanxiongchun@bytedance.com, songmuchun@bytedance.com
Subject: Re: [External] Re: [PATCH v2 2/6] perf/core: Introduce percpu perf_cgroup
Date: Wed, 23 Mar 2022 14:17:44 +0100 [thread overview]
Message-ID: <20220323131744.GY8939@worktop.programming.kicks-ass.net> (raw)
In-Reply-To: <f6a46509-a373-5c7a-8694-8eaf0ebc69ab@bytedance.com>
On Wed, Mar 23, 2022 at 09:07:01PM +0800, Chengming Zhou wrote:
> On 2022/3/23 8:51 下午, Peter Zijlstra wrote:
> > On Tue, Mar 22, 2022 at 08:08:30PM +0800, Chengming Zhou wrote:
> >
> >> 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
> >
> >> @@ -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;
So this compares the cpu thing against the task thing, if matching, we
bail.
> >> +
> >> + __this_cpu_write(cpu_perf_cgroup, cgrp);
Then we set cpu thing.
> >> +
> >> 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)
> >> + continue;
> >> +
> >> perf_pmu_disable(cpuctx->ctx.pmu);
> >>
> >> cpu_ctx_sched_out(cpuctx, EVENT_ALL);
> >> + cpuctx->cgrp = cgrp
But here we already have exactly the same pattern, we match cpuctx thing
against task thing and skip/set etc.
> > Also, I really don't see the point of cpu_perf_cgroup, cpuctx->cgrp
> > should be able to do this.
>
> But the problem is that we have two cpuctx on the percpu list, do you
> mean we should use perf_cgroup of the first cpuctx to compare with
> the next task's perf_cgroup ?
>
> Or we should delete the cgrp in cpuctx, and use this new percpu cpu_perf_cgroup?
I'm a bit confused, per the above, you already do exactly what the new
cpu_perf_cgroup does on the cpuctx->cgrp variable. AFAICT the only think
the new per-cpu variable does is avoid a lock, howveer:
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -833,6 +833,7 @@ static DEFINE_PER_CPU(struct list_head,
> > */
> > 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 +844,20 @@ static void perf_cgroup_switch(struct ta
> > */
> > local_irq_save(flags);
> >
> > + cgrp = perf_cgroup_from_task(task, NULL);
> > +
> > 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);
> >
> > + if (READ_ONCE(cpuctx->cgrp == cgrp))
> > + continue
I think we can avoid that by doing an early check, hmm?
> > +
> > perf_ctx_lock(cpuctx, cpuctx->task_ctx);
> > +
> > + if (cpuctx->cgrp == cgrp)
> > + goto next;
> > +
> > perf_pmu_disable(cpuctx->ctx.pmu);
> >
> > cpu_ctx_sched_out(cpuctx, EVENT_ALL);
> > @@ -855,50 +865,22 @@ static void perf_cgroup_switch(struct ta
> > * must not be done before ctxswout due
> > * to event_filter_match() in event_sched_out()
> > */
> > - cpuctx->cgrp = perf_cgroup_from_task(task,
> > - &cpuctx->ctx);
> > + WRITE_ONCE(cpuctx->cgrp, cgrp);
next prev parent reply other threads:[~2022-03-23 13:18 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
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 [this message]
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=20220323131744.GY8939@worktop.programming.kicks-ass.net \
--to=peterz@infradead.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=namhyung@kernel.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).