* [PATCH RFC] perf, core: disable pmu while context rotation only if needed
@ 2011-11-15 11:34 Gleb Natapov
2011-11-15 12:07 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Gleb Natapov @ 2011-11-15 11:34 UTC (permalink / raw)
To: linux-kernel; +Cc: Peter Zijlstra, Ingo Molnar
Currently pmu is disabled and re-enabled on each timer interrupt even
when no rotation or frequency adjustment is needed. On Intel CPU this
results in two writes into PERF_GLOBAL_CTRL MSR per tick. On bare metal
it does not cause significant slowdown, but when running perf in a virtual
machine it leads to 20% slowdown on my machine.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/kernel/events/core.c b/kernel/events/core.c
index bdcd413..83c87d8 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2317,7 +2317,7 @@ static void perf_adjust_period(struct perf_event *event, u64 nsec, u64 count)
}
}
-static void perf_ctx_adjust_freq(struct perf_event_context *ctx, u64 period)
+static bool perf_ctx_adjust_freq(struct perf_event_context *ctx, u64 period, bool pmu_disabled)
{
struct perf_event *event;
struct hw_perf_event *hwc;
@@ -2347,6 +2347,11 @@ static void perf_ctx_adjust_freq(struct perf_event_context *ctx, u64 period)
if (!event->attr.freq || !event->attr.sample_freq)
continue;
+ if (!pmu_disabled) {
+ perf_pmu_disable(ctx->pmu);
+ pmu_disabled = true;
+ }
+
event->pmu->read(event);
now = local64_read(&event->count);
delta = now - hwc->freq_count_stamp;
@@ -2355,6 +2360,8 @@ static void perf_ctx_adjust_freq(struct perf_event_context *ctx, u64 period)
if (delta > 0)
perf_adjust_period(event, period, delta);
}
+
+ return pmu_disabled;
}
/*
@@ -2380,6 +2387,7 @@ static void perf_rotate_context(struct perf_cpu_context *cpuctx)
u64 interval = (u64)cpuctx->jiffies_interval * TICK_NSEC;
struct perf_event_context *ctx = NULL;
int rotate = 0, remove = 1;
+ bool pmu_disabled = false;
if (cpuctx->ctx.nr_events) {
remove = 0;
@@ -2395,10 +2403,9 @@ static void perf_rotate_context(struct perf_cpu_context *cpuctx)
}
perf_ctx_lock(cpuctx, cpuctx->task_ctx);
- perf_pmu_disable(cpuctx->ctx.pmu);
- perf_ctx_adjust_freq(&cpuctx->ctx, interval);
+ pmu_disabled = perf_ctx_adjust_freq(&cpuctx->ctx, interval, pmu_disabled);
if (ctx)
- perf_ctx_adjust_freq(ctx, interval);
+ pmu_disabled = perf_ctx_adjust_freq(ctx, interval, pmu_disabled);
if (!rotate)
goto done;
@@ -2417,7 +2424,8 @@ done:
if (remove)
list_del_init(&cpuctx->rotation_list);
- perf_pmu_enable(cpuctx->ctx.pmu);
+ if (pmu_disabled)
+ perf_pmu_enable(cpuctx->ctx.pmu);
perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
}
--
Gleb.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] perf, core: disable pmu while context rotation only if needed
2011-11-15 11:34 [PATCH RFC] perf, core: disable pmu while context rotation only if needed Gleb Natapov
@ 2011-11-15 12:07 ` Peter Zijlstra
2011-11-15 12:38 ` Gleb Natapov
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2011-11-15 12:07 UTC (permalink / raw)
To: Gleb Natapov; +Cc: linux-kernel, Ingo Molnar
On Tue, 2011-11-15 at 13:34 +0200, Gleb Natapov wrote:
>
> Currently pmu is disabled and re-enabled on each timer interrupt even
> when no rotation or frequency adjustment is needed. On Intel CPU this
> results in two writes into PERF_GLOBAL_CTRL MSR per tick. On bare metal
> it does not cause significant slowdown, but when running perf in a virtual
> machine it leads to 20% slowdown on my machine.
I detest asymmetric locking like that, does something like the below
also work for you?
---
include/linux/perf_event.h | 1 +
kernel/events/core.c | 30 +++++++++++++++++++++++++-----
2 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 1e9ebe5..92773aa 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -889,6 +889,7 @@ struct perf_event_context {
int nr_active;
int is_active;
int nr_stat;
+ int nr_freq;
int rotate_disable;
atomic_t refcount;
struct task_struct *task;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7f693e9..d8f2f38 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1127,6 +1127,8 @@ event_sched_out(struct perf_event *event,
if (!is_software_event(event))
cpuctx->active_oncpu--;
ctx->nr_active--;
+ if (event->attr.freq && event->attr.sample_freq)
+ ctx->nr_freq--;
if (event->attr.exclusive || !cpuctx->active_oncpu)
cpuctx->exclusive = 0;
}
@@ -1403,6 +1405,8 @@ event_sched_in(struct perf_event *event,
if (!is_software_event(event))
cpuctx->active_oncpu++;
ctx->nr_active++;
+ if (event->attr.freq && event->attr.sample_freq)
+ ctx->nr_freq++;
if (event->attr.exclusive)
cpuctx->exclusive = 1;
@@ -2324,6 +2328,9 @@ static void perf_ctx_adjust_freq(struct perf_event_context *ctx, u64 period)
u64 interrupts, now;
s64 delta;
+ if (!ctx->nr_freq)
+ return;
+
list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
if (event->state != PERF_EVENT_STATE_ACTIVE)
continue;
@@ -2379,12 +2386,14 @@ static void perf_rotate_context(struct perf_cpu_context *cpuctx)
{
u64 interval = (u64)cpuctx->jiffies_interval * TICK_NSEC;
struct perf_event_context *ctx = NULL;
- int rotate = 0, remove = 1;
+ int rotate = 0, remove = 1, freq = 0;
if (cpuctx->ctx.nr_events) {
remove = 0;
if (cpuctx->ctx.nr_events != cpuctx->ctx.nr_active)
rotate = 1;
+ if (cpuctx->ctx.nr_freq)
+ freq = 1;
}
ctx = cpuctx->task_ctx;
@@ -2392,16 +2401,26 @@ static void perf_rotate_context(struct perf_cpu_context *cpuctx)
remove = 0;
if (ctx->nr_events != ctx->nr_active)
rotate = 1;
+ if (ctx->nr_freq)
+ freq = 1;
}
+ if (!rotate && !freq)
+ goto done;
+
perf_ctx_lock(cpuctx, cpuctx->task_ctx);
perf_pmu_disable(cpuctx->ctx.pmu);
+
+ if (!freq)
+ goto rotate;
+
perf_ctx_adjust_freq(&cpuctx->ctx, interval);
if (ctx)
perf_ctx_adjust_freq(ctx, interval);
+rotate:
if (!rotate)
- goto done;
+ goto unlock;
cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
if (ctx)
@@ -2413,12 +2432,13 @@ static void perf_rotate_context(struct perf_cpu_context *cpuctx)
perf_event_sched_in(cpuctx, ctx, current);
+unlock:
+ perf_pmu_enable(cpuctx->ctx.pmu);
+ perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
+
done:
if (remove)
list_del_init(&cpuctx->rotation_list);
-
- perf_pmu_enable(cpuctx->ctx.pmu);
- perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
}
void perf_event_task_tick(void)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] perf, core: disable pmu while context rotation only if needed
2011-11-15 12:07 ` Peter Zijlstra
@ 2011-11-15 12:38 ` Gleb Natapov
2011-11-15 12:55 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Gleb Natapov @ 2011-11-15 12:38 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: linux-kernel, Ingo Molnar
On Tue, Nov 15, 2011 at 01:07:13PM +0100, Peter Zijlstra wrote:
> On Tue, 2011-11-15 at 13:34 +0200, Gleb Natapov wrote:
> >
> > Currently pmu is disabled and re-enabled on each timer interrupt even
> > when no rotation or frequency adjustment is needed. On Intel CPU this
> > results in two writes into PERF_GLOBAL_CTRL MSR per tick. On bare metal
> > it does not cause significant slowdown, but when running perf in a virtual
> > machine it leads to 20% slowdown on my machine.
>
>
> I detest asymmetric locking like that, does something like the below
> also work for you?
>
It does.
>
> + if (!rotate && !freq)
> + goto done;
> +
> perf_ctx_lock(cpuctx, cpuctx->task_ctx);
> perf_pmu_disable(cpuctx->ctx.pmu);
> +
> + if (!freq)
> + goto rotate;
> +
Why goto, why not
if (freq) {
> perf_ctx_adjust_freq(&cpuctx->ctx, interval);
> if (ctx)
> perf_ctx_adjust_freq(ctx, interval);
}
And the same with next goto.
>
> +rotate:
> if (!rotate)
> - goto done;
> + goto unlock;
>
> cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
> if (ctx)
> @@ -2413,12 +2432,13 @@ static void perf_rotate_context(struct perf_cpu_context *cpuctx)
>
> perf_event_sched_in(cpuctx, ctx, current);
>
> +unlock:
> + perf_pmu_enable(cpuctx->ctx.pmu);
> + perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
> +
> done:
> if (remove)
> list_del_init(&cpuctx->rotation_list);
> -
> - perf_pmu_enable(cpuctx->ctx.pmu);
> - perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
> }
>
> void perf_event_task_tick(void)
--
Gleb.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] perf, core: disable pmu while context rotation only if needed
2011-11-15 12:38 ` Gleb Natapov
@ 2011-11-15 12:55 ` Peter Zijlstra
2011-11-22 10:56 ` Gleb Natapov
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2011-11-15 12:55 UTC (permalink / raw)
To: Gleb Natapov; +Cc: linux-kernel, Ingo Molnar
On Tue, 2011-11-15 at 14:38 +0200, Gleb Natapov wrote:
> On Tue, Nov 15, 2011 at 01:07:13PM +0100, Peter Zijlstra wrote:
> > On Tue, 2011-11-15 at 13:34 +0200, Gleb Natapov wrote:
> > >
> > > Currently pmu is disabled and re-enabled on each timer interrupt even
> > > when no rotation or frequency adjustment is needed. On Intel CPU this
> > > results in two writes into PERF_GLOBAL_CTRL MSR per tick. On bare metal
> > > it does not cause significant slowdown, but when running perf in a virtual
> > > machine it leads to 20% slowdown on my machine.
> >
> >
> > I detest asymmetric locking like that, does something like the below
> > also work for you?
> >
> It does.
ok, great.
> >
> > + if (!rotate && !freq)
> > + goto done;
> > +
> > perf_ctx_lock(cpuctx, cpuctx->task_ctx);
> > perf_pmu_disable(cpuctx->ctx.pmu);
> > +
> > + if (!freq)
> > + goto rotate;
> > +
> Why goto, why not
>
> if (freq) {
> > perf_ctx_adjust_freq(&cpuctx->ctx, interval);
> > if (ctx)
> > perf_ctx_adjust_freq(ctx, interval);
> }
>
> And the same with next goto.
Because, uhm,. dunno. Let me make that if()s and commit the thing.
Thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] perf, core: disable pmu while context rotation only if needed
2011-11-15 12:55 ` Peter Zijlstra
@ 2011-11-22 10:56 ` Gleb Natapov
2011-11-22 10:59 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Gleb Natapov @ 2011-11-22 10:56 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: linux-kernel, Ingo Molnar
Hi Peter,
On Tue, Nov 15, 2011 at 01:55:18PM +0100, Peter Zijlstra wrote:
> On Tue, 2011-11-15 at 14:38 +0200, Gleb Natapov wrote:
> > On Tue, Nov 15, 2011 at 01:07:13PM +0100, Peter Zijlstra wrote:
> > > On Tue, 2011-11-15 at 13:34 +0200, Gleb Natapov wrote:
> > > >
> > > > Currently pmu is disabled and re-enabled on each timer interrupt even
> > > > when no rotation or frequency adjustment is needed. On Intel CPU this
> > > > results in two writes into PERF_GLOBAL_CTRL MSR per tick. On bare metal
> > > > it does not cause significant slowdown, but when running perf in a virtual
> > > > machine it leads to 20% slowdown on my machine.
> > >
> > >
> > > I detest asymmetric locking like that, does something like the below
> > > also work for you?
> > >
> > It does.
>
> ok, great.
>
> > >
> > > + if (!rotate && !freq)
> > > + goto done;
> > > +
> > > perf_ctx_lock(cpuctx, cpuctx->task_ctx);
> > > perf_pmu_disable(cpuctx->ctx.pmu);
> > > +
> > > + if (!freq)
> > > + goto rotate;
> > > +
> > Why goto, why not
> >
> > if (freq) {
> > > perf_ctx_adjust_freq(&cpuctx->ctx, interval);
> > > if (ctx)
> > > perf_ctx_adjust_freq(ctx, interval);
> > }
> >
> > And the same with next goto.
>
> Because, uhm,. dunno. Let me make that if()s and commit the thing.
> Thanks!
Have you committed it somewhere? I do not see it in tip.git, but may be
I am looking in a wrong place.
--
Gleb.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] perf, core: disable pmu while context rotation only if needed
2011-11-22 10:56 ` Gleb Natapov
@ 2011-11-22 10:59 ` Peter Zijlstra
0 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2011-11-22 10:59 UTC (permalink / raw)
To: Gleb Natapov; +Cc: linux-kernel, Ingo Molnar
On Tue, 2011-11-22 at 12:56 +0200, Gleb Natapov wrote:
> Have you committed it somewhere? I do not see it in tip.git, but may be
> I am looking in a wrong place.
Its still in my queue, I guess I should poke mingo again to pick up.
Also, I really ought to get my k.org account back up and write a script
to push this queue of mine into a git tree for ease of use.
For now you can have a peek at it through:
http://programming.kicks-ass.net/sekrit/patches.tar.bz2
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-11-22 11:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-15 11:34 [PATCH RFC] perf, core: disable pmu while context rotation only if needed Gleb Natapov
2011-11-15 12:07 ` Peter Zijlstra
2011-11-15 12:38 ` Gleb Natapov
2011-11-15 12:55 ` Peter Zijlstra
2011-11-22 10:56 ` Gleb Natapov
2011-11-22 10:59 ` Peter Zijlstra
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.