From: Frederic Weisbecker on
On Tue, May 04, 2010 at 02:38:35PM +0200, Tejun Heo wrote:
> Instead of calling perf_sw_event() directly from set_task_cpu(),
> implement perf_event_task_migrate() which takes the same arguments as
> trace_sched_migrate_task() and invokes perf_sw_event() is the task is
> really migrating (cur_cpu != new_cpu). This will help unifying
> notifiers in sched.
>
> Signed-off-by: Tejun Heo <tj(a)kernel.org>
> Cc: Peter Zijlstra <a.p.zijlstra(a)chello.nl>
> Cc: Paul Mackerras <paulus(a)samba.org>
> Cc: Ingo Molnar <mingo(a)elte.hu>
> Cc: Arnaldo Carvalho de Melo <acme(a)redhat.com>
> ---
> include/linux/perf_event.h | 3 +++
> kernel/perf_event.c | 11 +++++++++++
> kernel/sched.c | 5 ++---
> 3 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index c8e3754..a5eec48 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -754,6 +754,7 @@ extern int perf_max_events;
>
> extern const struct pmu *hw_perf_event_init(struct perf_event *event);
>
> +extern void perf_event_task_migrate(struct task_struct *task, int new_cpu);
> extern void perf_event_task_sched_in(struct task_struct *task);
> extern void perf_event_task_sched_out(struct task_struct *task, struct task_struct *next);
> extern void perf_event_task_tick(struct task_struct *task);
> @@ -949,6 +950,8 @@ extern void perf_event_enable(struct perf_event *event);
> extern void perf_event_disable(struct perf_event *event);
> #else
> static inline void
> +perf_event_task_migrate(struct task_struct *task, int new_cpu) { }
> +static inline void
> perf_event_task_sched_in(struct task_struct *task) { }
> static inline void
> perf_event_task_sched_out(struct task_struct *task,
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index 3d1552d..a01ba31 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -1148,6 +1148,17 @@ static void perf_event_sync_stat(struct perf_event_context *ctx,
> }
>
> /*
> + * Called from scheduler set_task_cpu() to notify migration events.
> + * If the task is moving to a different cpu, generate a migration sw
> + * event.
> + */
> +void perf_event_task_migrate(struct task_struct *task, int new_cpu)
> +{
> + if (task_cpu(task) != new_cpu)
> + perf_sw_event(PERF_COUNT_SW_CPU_MIGRATIONS, 1, 1, NULL, 0);
> +}



This needs to be static and inline (I haven't seem external users in this
patchset).

And we want it to be inlined because we save the caller address and the frame
pointer from perf_sw_event(), and a new level of call is not wanted here.



> +
> +/*
> * Called from scheduler to remove the events of the current task,
> * with interrupts disabled.
> *
> diff --git a/kernel/sched.c b/kernel/sched.c
> index c20fd31..2568911 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -2084,11 +2084,10 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
> #endif
>
> trace_sched_migrate_task(p, new_cpu);
> + perf_event_task_migrate(p, new_cpu);
>
> - if (task_cpu(p) != new_cpu) {
> + if (task_cpu(p) != new_cpu)



In fact why not moving both tracing calls under this check.
This is going to fix the migrate trace event that gets called
even on "spurious" migrations, and you avoid the duplicate check
in the perf callback.

Thanks.


> p->se.nr_migrations++;
> - perf_sw_event(PERF_COUNT_SW_CPU_MIGRATIONS, 1, 1, NULL, 0);
> - }
>
> __set_task_cpu(p, new_cpu);
> }
> --
> 1.6.4.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on
Hello,

On 05/05/2010 07:08 AM, Frederic Weisbecker wrote:
>> /*
>> + * Called from scheduler set_task_cpu() to notify migration events.
>> + * If the task is moving to a different cpu, generate a migration sw
>> + * event.
>> + */
>> +void perf_event_task_migrate(struct task_struct *task, int new_cpu)
>> +{
>> + if (task_cpu(task) != new_cpu)
>> + perf_sw_event(PERF_COUNT_SW_CPU_MIGRATIONS, 1, 1, NULL, 0);
>> +}
>
> This needs to be static and inline (I haven't seem external users in this
> patchset).

Hmm... after the last patch, this one becomes a TP probe function
which can't be inlined.

> And we want it to be inlined because we save the caller address and the frame
> pointer from perf_sw_event(), and a new level of call is not wanted here.

Oh I see. So, to use it with TP, I would need increase the @skip
parameter to perf_fetch_caller_regs() somehow, right? Also, it
probably would be a good idea to add a comment w/ big fat warning to
perf_sw_event().

>> @@ -2084,11 +2084,10 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
>> #endif
>>
>> trace_sched_migrate_task(p, new_cpu);
>> + perf_event_task_migrate(p, new_cpu);
>>
>> - if (task_cpu(p) != new_cpu) {
>> + if (task_cpu(p) != new_cpu)
>
> In fact why not moving both tracing calls under this check.
> This is going to fix the migrate trace event that gets called
> even on "spurious" migrations, and you avoid the duplicate check
> in the perf callback.

Yeah, that would be my preferred choice too. I just didn't know that
could be changed. Cool. I'll.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on
On Wed, 2010-05-05 at 07:08 +0200, Frederic Weisbecker wrote:
>
> In fact why not moving both tracing calls under this check.
> This is going to fix the migrate trace event that gets called
> even on "spurious" migrations, and you avoid the duplicate check
> in the perf callback.

I kept the tracepoint out of that because I wanted to see how often it
attempts silly migrations :-)



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on
On 05/05/2010 11:11 AM, Peter Zijlstra wrote:
> On Wed, 2010-05-05 at 07:08 +0200, Frederic Weisbecker wrote:
>>
>> In fact why not moving both tracing calls under this check.
>> This is going to fix the migrate trace event that gets called
>> even on "spurious" migrations, and you avoid the duplicate check
>> in the perf callback.
>
> I kept the tracepoint out of that because I wanted to see how often it
> attempts silly migrations :-)

Do you still need to keep it or is it okay to move it under the
silliness check?

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on
On Wed, 2010-05-05 at 11:37 +0200, Tejun Heo wrote:
> On 05/05/2010 11:11 AM, Peter Zijlstra wrote:
> > On Wed, 2010-05-05 at 07:08 +0200, Frederic Weisbecker wrote:
> >>
> >> In fact why not moving both tracing calls under this check.
> >> This is going to fix the migrate trace event that gets called
> >> even on "spurious" migrations, and you avoid the duplicate check
> >> in the perf callback.
> >
> > I kept the tracepoint out of that because I wanted to see how often it
> > attempts silly migrations :-)
>
> Do you still need to keep it or is it okay to move it under the
> silliness check?

I guess you can move it, I often end up adding tons of trace_printk()
anyway when hunting funnies, might as well add one here when its
relevant.

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