perf, core: disable pmu while context rotation only if needed

RFC PATCH In review Gleb Natapov 2 participants 6 messages Nov 15, 2011 – Nov 22, 2011 perf events

Share & tools

Gleb Natapov

author

2011-11-15 11:34 UTC #0 lore raw

RFC PATCH 1/1 perf, core: disable pmu while context rotation only if needed

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 <[email protected]>

kernel/events/core.c +13 -5

index bdcd413..83c87d8 100644
@@ -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.

Peter Zijlstra

2011-11-15 12:07 UTC #1 lore raw

Quoted 6 lines · Gleb Natapov

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(-)

include/linux/perf_event.h +1 -0

index 1e9ebe5..92773aa 100644
@@ -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;

kernel/events/core.c +25 -5

index 7f693e9..d8f2f38 100644
@@ -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)

Gleb Natapov

author

2011-11-15 12:38 UTC #2 lore raw

Quoted 9 lines · Peter Zijlstra

Quoted 6 lines · Gleb Natapov
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.

Quoted 3 lines

I detest asymmetric locking like that, does something like the below also work for you?

It does.

kernel/events/core.c static void perf_rotate_context(struct perf_cpu_context *cpuctx) · line 2416

Quoted 10 lines

kernel/events/core.c +7 -0

+	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) {

kernel/events/core.c static void perf_rotate_context(struct perf_cpu_context *cpuctx) · line 2419

Quoted 3 lines

kernel/events/core.c +0 -0

 	perf_ctx_adjust_freq(&cpuctx->ctx, interval);
 	if (ctx)
 		perf_ctx_adjust_freq(ctx, interval);

And the same with next goto.

Quoted 15 lines

kernel/events/core.c +5 -1

+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);

kernel/events/core.c static void perf_rotate_context(struct perf_cpu_context *cpuctx) · line 2444

Quoted 10 lines

kernel/events/core.c +1 -3

+
 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.

Peter Zijlstra

2011-11-15 12:55 UTC #3 lore raw

Quoted 10 lines · Gleb Natapov

Quoted 9 lines · Peter Zijlstra
Quoted 6 lines · Gleb Natapov
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.

Quoted 4 lines

Quoted 3 lines
I detest asymmetric locking like that, does something like the below also work for you?
It does.

ok, great.

Quoted 12 lines

kernel/events/core.c static void perf_rotate_context(struct perf_cpu_context *cpuctx) · line 2416
Quoted 10 lines
kernel/events/core.c +7 -0
+	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

Quoted 7 lines

	if (freq) {
kernel/events/core.c static void perf_rotate_context(struct perf_cpu_context *cpuctx) · line 2419
Quoted 3 lines
kernel/events/core.c +0 -0
 	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!

Gleb Natapov

author

2011-11-22 10:56 UTC #4 lore raw

Hi Peter,

Quoted 34 lines · Peter Zijlstra

Quoted 10 lines · Gleb Natapov
Quoted 9 lines · Peter Zijlstra
Quoted 6 lines · Gleb Natapov
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.
Quoted 4 lines
Quoted 3 lines
I detest asymmetric locking like that, does something like the below also work for you?
It does.
ok, great.
Quoted 16 lines
kernel/events/core.c static void perf_rotate_context(struct perf_cpu_context *cpuctx) · line 2416
Quoted 10 lines
kernel/events/core.c +7 -0
+	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) {
Quoted 3 lines
kernel/events/core.c +0 -0
 	perf_ctx_adjust_freq(&cpuctx->ctx, interval);
 	if (ctx)
 		perf_ctx_adjust_freq(ctx, interval);

Quoted 6 lines

Quoted 3 lines
        }
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.

Peter Zijlstra

2011-11-22 11:00 UTC #5 lore raw

Quoted 2 lines · Gleb Natapov

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