Mailing List Archive: Linux: Kernel

[RFC PATCH 4/4] perf/core: Schedule every pinned events before the the non-pinned

Index | Next | Previous | View Threaded

fweisbec at gmail

Nov 8, 2009, 12:13 PM

Post #1 of 3 (107 views)
Permalink

[RFC PATCH 4/4] perf/core: Schedule every pinned events before the the non-pinned

Currently, the order to schedule events is as follows:

- cpu context pinned events
- cpu context non-pinned events
- task context pinned events
- task context non-pinned events

What we want instead is to schedule every pinned events first because
those have a higher priority.

This is what does this patch in each task tick. If the approach is
agreed, we may want to expand this to task-only sched in (where the
cpu events are not sched out), fork, exec, etc... So that we guarantee
the pinned priority every time the task is scheduled in.

Signed-off-by: Frederic Weisbecker <fweisbec [at] gmail>
Cc: Peter Zijlstra <peterz [at] infradead>
Cc: Arnaldo Carvalho de Melo <acme [at] redhat>
Cc: Mike Galbraith <efault [at] gmx>
Cc: Paul Mackerras <paulus [at] samba>
Cc: Thomas Gleixner <tglx [at] linutronix>
---
kernel/perf_event.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 50f2997..f32aec4 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -1327,6 +1327,41 @@ __perf_event_sched_in(struct perf_event_context *ctx,
spin_unlock(&ctx->lock);
}

+static void
+__perf_event_sched_in_all(struct perf_event_context *ctx,
+ struct perf_cpu_context *cpuctx, int cpu)
+{
+ struct perf_event_context *cpu_ctx = &cpuctx->ctx;
+
+ /* May require different classes between cpu and task lock */
+ spin_lock(&cpu_ctx->lock);
+ spin_lock(&ctx->lock);
+ cpu_ctx->is_active = ctx->is_active = 1;
+
+ ctx->timestamp = cpu_ctx->timestamp = perf_clock();
+
+ perf_disable();
+
+ if (cpu_ctx->nr_events)
+ __perf_event_sched_in_pinned(cpu_ctx, cpuctx, cpu);
+
+ if (ctx->nr_events)
+ __perf_event_sched_in_pinned(cpu_ctx, cpuctx, cpu);
+
+ if (cpu_ctx->nr_events)
+ __perf_event_sched_in_volatile(cpu_ctx, cpuctx, cpu);
+
+ if (ctx->nr_events)
+ __perf_event_sched_in_volatile(cpu_ctx, cpuctx, cpu);
+
+ cpuctx->task_ctx = ctx;
+
+ perf_enable();
+
+ spin_unlock(&ctx->lock);
+ spin_lock(&cpu_ctx->lock);
+}
+
/*
* Called from scheduler to add the events of the current task
* with interrupts disabled.
@@ -1477,6 +1512,16 @@ static void rotate_ctx(struct perf_event_context *ctx)
spin_unlock(&ctx->lock);
}

+static void
+perf_event_sched_in_all(struct perf_event_context *ctx,
+ struct perf_cpu_context *cpuctx, int cpu)
+{
+ if (!ctx || ctx == cpuctx->task_ctx)
+ perf_event_cpu_sched_in(cpuctx, cpu);
+ else
+ __perf_event_sched_in_all(ctx, cpuctx, cpu);
+}
+
void perf_event_task_tick(struct task_struct *curr, int cpu)
{
struct perf_cpu_context *cpuctx;
@@ -1500,9 +1545,7 @@ void perf_event_task_tick(struct task_struct *curr, int cpu)
if (ctx)
rotate_ctx(ctx);

- perf_event_cpu_sched_in(cpuctx, cpu);
- if (ctx)
- perf_event_task_sched_in(curr, cpu);
+ perf_event_sched_in_all(ctx, cpuctx, cpu);
}

static void __perf_event_enable_on_exec(struct perf_event *event,
--
1.6.2.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

peterz at infradead

Nov 10, 2009, 2:10 AM

Post #2 of 3 (95 views)
Permalink

Re: [RFC PATCH 4/4] perf/core: Schedule every pinned events before the the non-pinned [In reply to]

On Sun, 2009-11-08 at 21:13 +0100, Frederic Weisbecker wrote:

> +static void
> +__perf_event_sched_in_all(struct perf_event_context *ctx,
> + struct perf_cpu_context *cpuctx, int cpu)
> +{
> + struct perf_event_context *cpu_ctx = &cpuctx->ctx;
> +
> + /* May require different classes between cpu and task lock */
> + spin_lock(&cpu_ctx->lock);
> + spin_lock(&ctx->lock);

Would be good to know for sure, running with lockdep enabled ought to
tell you that pretty quick ;-)

> + cpu_ctx->is_active = ctx->is_active = 1;
> +
> + ctx->timestamp = cpu_ctx->timestamp = perf_clock();
> +
> + perf_disable();
> +
> + if (cpu_ctx->nr_events)
> + __perf_event_sched_in_pinned(cpu_ctx, cpuctx, cpu);
> +
> + if (ctx->nr_events)
> + __perf_event_sched_in_pinned(cpu_ctx, cpuctx, cpu);
> +
> + if (cpu_ctx->nr_events)
> + __perf_event_sched_in_volatile(cpu_ctx, cpuctx, cpu);
> +
> + if (ctx->nr_events)
> + __perf_event_sched_in_volatile(cpu_ctx, cpuctx, cpu);
> +
> + cpuctx->task_ctx = ctx;
> +
> + perf_enable();
> +
> + spin_unlock(&ctx->lock);
> + spin_lock(&cpu_ctx->lock);

I'm pretty sure that ought to be spin_unlock() ;-)

> +}

Like Ingo I don't really like the volatile name.

Can't we simply have 2 lists per cpu a pinned and normal list, and first
schedule all the pinned and RR the normal events?

I guess one could implement that by adding the task context events to
the cpu context events on sched_in and removing them on sched_out. That
would clear up a lot of funny scheduling details.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

fweisbec at gmail

Nov 10, 2009, 2:34 AM

Post #3 of 3 (94 views)
Permalink

Re: [RFC PATCH 4/4] perf/core: Schedule every pinned events before the the non-pinned [In reply to]

On Tue, Nov 10, 2009 at 11:10:13AM +0100, Peter Zijlstra wrote:
> On Sun, 2009-11-08 at 21:13 +0100, Frederic Weisbecker wrote:
>
> > +static void
> > +__perf_event_sched_in_all(struct perf_event_context *ctx,
> > + struct perf_cpu_context *cpuctx, int cpu)
> > +{
> > + struct perf_event_context *cpu_ctx = &cpuctx->ctx;
> > +
> > + /* May require different classes between cpu and task lock */
> > + spin_lock(&cpu_ctx->lock);
> > + spin_lock(&ctx->lock);
>
> Would be good to know for sure, running with lockdep enabled ought to
> tell you that pretty quick ;-)

That's about sure I guess :)
I just wanted to take care of that after your comments.

> > + cpu_ctx->is_active = ctx->is_active = 1;
> > +
> > + ctx->timestamp = cpu_ctx->timestamp = perf_clock();
> > +
> > + perf_disable();
> > +
> > + if (cpu_ctx->nr_events)
> > + __perf_event_sched_in_pinned(cpu_ctx, cpuctx, cpu);
> > +
> > + if (ctx->nr_events)
> > + __perf_event_sched_in_pinned(cpu_ctx, cpuctx, cpu);
> > +
> > + if (cpu_ctx->nr_events)
> > + __perf_event_sched_in_volatile(cpu_ctx, cpuctx, cpu);
> > +
> > + if (ctx->nr_events)
> > + __perf_event_sched_in_volatile(cpu_ctx, cpuctx, cpu);
> > +
> > + cpuctx->task_ctx = ctx;
> > +
> > + perf_enable();
> > +
> > + spin_unlock(&ctx->lock);
> > + spin_lock(&cpu_ctx->lock);
>
> I'm pretty sure that ought to be spin_unlock() ;-)

Indeed :)

> > +}
>
>
> Like Ingo I don't really like the volatile name.
>
> Can't we simply have 2 lists per cpu a pinned and normal list, and first
> schedule all the pinned and RR the normal events?
>
> I guess one could implement that by adding the task context events to
> the cpu context events on sched_in and removing them on sched_out. That
> would clear up a lot of funny scheduling details.

I thought about doing that, but didn't expand the idea that much,
because of the list manipulation that induces.

But you're right, that would be be indeed more proper.
I can just save the "real" cpu event group tail in the
struct perf_cpu_context so that I can keep track of the real
state and (un)glue the queues easily.

Yeah, I'll try that, thanks!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads