From: Namhyung Kim <namhyung@kernel.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Kan Liang <kan.liang@linux.intel.com>,
Ingo Molnar <mingo@kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
Stephane Eranian <eranian@google.com>,
Ian Rogers <irogers@google.com>, Gabriel Marin <gmx@google.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Jiri Olsa <jolsa@redhat.com>, Andi Kleen <ak@linux.intel.com>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Michael Ellerman <mpe@ellerman.id.au>
Subject: Re: [PATCH V2 3/3] perf: Optimize sched_task() in a context switch
Date: Fri, 4 Dec 2020 16:14:33 +0900 [thread overview]
Message-ID: <CAM9d7ciukm4RAH+44YWhZRummKzk1HTbnZ0Sc4Xd5ZyCo=x0xQ@mail.gmail.com> (raw)
In-Reply-To: <20201201172903.GT3040@hirez.programming.kicks-ass.net>
Hi Peter,
On Wed, Dec 2, 2020 at 2:29 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Nov 30, 2020 at 11:38:42AM -0800, kan.liang@linux.intel.com wrote:
> > From: Kan Liang <kan.liang@linux.intel.com>
> >
> > Some calls to sched_task() in a context switch can be avoided. For
> > example, large PEBS only requires flushing the buffer in context switch
> > out. The current code still invokes the sched_task() for large PEBS in
> > context switch in.
>
> I still hate this one, how's something like this then?
> Which I still don't really like.. but at least its simpler.
>
> (completely untested, may contain spurious edits, might ICE the
> compiler and set your pets on fire if it doesn't)
I've tested this version... and it worked well besides the optimization.. :)
[SNIP]
> +static void context_sched_task(struct perf_cpu_context *cpuctx,
> + struct perf_event_context *ctx,
> + bool sched_in)
> +{
> + struct pmu *pmu = ctx->pmu;
> +
> + if (cpuctx->sched_cb_dir[sched_in] && pmu->sched_task)
> + pmu->sched_task(ctx, false);
applied: s/false/sched_in/
> +}
> +
> static void perf_event_context_sched_out(struct task_struct *task, int ctxn,
> struct task_struct *next)
> {
> @@ -3424,9 +3433,7 @@ static void perf_event_context_sched_out
> WRITE_ONCE(next_ctx->task, task);
>
> perf_pmu_disable(pmu);
> -
> - if (cpuctx->sched_cb_usage && pmu->sched_task)
> - pmu->sched_task(ctx, false);
> + context_sched_task(cpuctx, ctx, false);
>
> /*
> * PMU specific parts of task perf context can require
> @@ -3465,8 +3472,7 @@ static void perf_event_context_sched_out
> raw_spin_lock(&ctx->lock);
> perf_pmu_disable(pmu);
>
> - if (cpuctx->sched_cb_usage && pmu->sched_task)
> - pmu->sched_task(ctx, false);
> + context_sched_task(cpuctx, ctx, false);
> task_ctx_sched_out(cpuctx, ctx, EVENT_ALL);
>
> perf_pmu_enable(pmu);
[SNIP]
> @@ -3563,8 +3582,7 @@ void __perf_event_task_sched_out(struct
> {
> int ctxn;
>
> - if (__this_cpu_read(perf_sched_cb_usage))
> - perf_pmu_sched_task(task, next, false);
> + perf_pmu_sched_task(task, next, false);
I think the reason is this change. It now calls perf_pmu_sched_task()
without checking the counter. And this is for per-cpu events.
>
> if (atomic_read(&nr_switch_events))
> perf_event_switch(task, next, false);
> @@ -3828,8 +3846,7 @@ static void perf_event_context_sched_in(
> cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
> perf_event_sched_in(cpuctx, ctx, task);
>
> - if (cpuctx->sched_cb_usage && pmu->sched_task)
> - pmu->sched_task(cpuctx->task_ctx, true);
> + context_sched_task(cpuctx, ctx, true);
>
> perf_pmu_enable(pmu);
>
> @@ -3875,8 +3892,7 @@ void __perf_event_task_sched_in(struct t
> if (atomic_read(&nr_switch_events))
> perf_event_switch(task, prev, true);
>
> - if (__this_cpu_read(perf_sched_cb_usage))
> - perf_pmu_sched_task(prev, task, true);
> + perf_pmu_sched_task(prev, task, true);
Ditto.
> }
>
> static u64 perf_calculate_period(struct perf_event *event, u64 nsec, u64 count)
So I made a change like below.. and it could bring the optimization back.
Thanks,
Namhyung
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9107e7c3ccfb..a30243a9fab5 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3528,6 +3528,9 @@ static void __perf_pmu_sched_task(struct
perf_cpu_context *cpuctx, bool sched_in
{
struct pmu *pmu;
+ if (!cpuctx->sched_cb_dir[sched_in])
+ return;
+
pmu = cpuctx->ctx.pmu; /* software PMUs will not have sched_task */
if (WARN_ON_ONCE(!pmu->sched_task))
next prev parent reply other threads:[~2020-12-04 7:15 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-30 19:38 [PATCH V2 1/3] perf/core: Flush PMU internal buffers for per-CPU events kan.liang
2020-11-30 19:38 ` [PATCH V2 2/3] perf/x86/intel: Set PERF_ATTACH_SCHED_CB for large PEBS and LBR kan.liang
2021-03-01 10:16 ` [tip: perf/urgent] " tip-bot2 for Kan Liang
2021-03-06 11:54 ` tip-bot2 for Kan Liang
2020-11-30 19:38 ` [PATCH V2 3/3] perf: Optimize sched_task() in a context switch kan.liang
2020-12-01 17:29 ` Peter Zijlstra
2020-12-02 10:26 ` Peter Zijlstra
2020-12-02 14:40 ` Namhyung Kim
2020-12-04 7:14 ` Namhyung Kim [this message]
2020-12-10 7:13 ` Namhyung Kim
2020-12-10 13:52 ` Liang, Kan
2020-12-10 14:25 ` Peter Zijlstra
2021-01-18 7:04 ` Namhyung Kim
2021-01-27 4:41 ` Namhyung Kim
2021-02-22 9:46 ` Namhyung Kim
2020-12-01 17:21 ` [PATCH V2 1/3] perf/core: Flush PMU internal buffers for per-CPU events Peter Zijlstra
2021-03-01 10:16 ` [tip: perf/urgent] " tip-bot2 for Kan Liang
2021-03-06 11:54 ` tip-bot2 for Kan Liang
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='CAM9d7ciukm4RAH+44YWhZRummKzk1HTbnZ0Sc4Xd5ZyCo=x0xQ@mail.gmail.com' \
--to=namhyung@kernel.org \
--cc=acme@kernel.org \
--cc=ak@linux.intel.com \
--cc=benh@kernel.crashing.org \
--cc=eranian@google.com \
--cc=gmx@google.com \
--cc=irogers@google.com \
--cc=jolsa@redhat.com \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=mpe@ellerman.id.au \
--cc=paulus@samba.org \
--cc=peterz@infradead.org \
/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).