Re: [PATCH 2/6] sched,trace: Add a tracepoint for remote wakeups via polling

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 06/04/2014 02:29 AM, Andy Lutomirski wrote:
> Remote wakeups of polling CPUs are a valuable performance
> improvement; add a tracepoint to make it much easier to verify that
> they're working.
>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>

I don't think this trace makes sense. The polling state is x86 only and 
this trace is in the generic code.

Furthermore, you may be not in polling state but in the idle mainloop 
before or after the idle state, so the trace will be wrong for the 
purpose you are aiming.

IMO, the right place would be in 'poll_idle' but why add such trace ?

If, on x86, we exit poll_idle, we have the idle state exit trace. The 
missing information would be the origin of the 'wakeup' (irq or ipi or 
nothing). The missing informations are the IPI traces [1].

And as a sidenote, the polling state could be rare on a system with a 
cpuidle driver, it should be much more easy to restrict the idle states 
to 'poll' only and check there are no IPI_WAKEUP, no ?

   -- Daniel

[1] 
http://lists.linaro.org/pipermail/linaro-kernel/2013-November/008581.html

> ---
>   include/trace/events/sched.h | 20 ++++++++++++++++++++
>   kernel/sched/core.c          |  4 ++++
>   2 files changed, 24 insertions(+)
>
> diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
> index 67e1bbf..08f8632 100644
> --- a/include/trace/events/sched.h
> +++ b/include/trace/events/sched.h
> @@ -530,6 +530,26 @@ TRACE_EVENT(sched_swap_numa,
>   			__entry->dst_pid, __entry->dst_tgid, __entry->dst_ngid,
>   			__entry->dst_cpu, __entry->dst_nid)
>   );
> +
> +/*
> + * Tracepoint for waking a polling cpu without an IPI.
> + */
> +TRACE_EVENT(sched_wake_polling_cpu,
> +
> +	TP_PROTO(int cpu),
> +
> +	TP_ARGS(cpu),
> +
> +	TP_STRUCT__entry(
> +		__field(	int,	cpu	)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->cpu	= cpu;
> +	),
> +
> +	TP_printk("cpu=%d", __entry->cpu)
> +);
>   #endif /* _TRACE_SCHED_H */
>
>   /* This part must be outside protection */
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 321d800..18bebfc 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -564,6 +564,8 @@ void resched_task(struct task_struct *p)
>
>   	if (set_nr_and_not_polling(p))
>   		smp_send_reschedule(cpu);
> +	else
> +		trace_sched_wake_polling_cpu(cpu);
>   }
>
>   void resched_cpu(int cpu)
> @@ -647,6 +649,8 @@ static void wake_up_idle_cpu(int cpu)
>   	smp_mb();
>   	if (!tsk_is_polling(rq->idle))
>   		smp_send_reschedule(cpu);
> +	else
> +		trace_sched_wake_polling_cpu(cpu);
>   }
>
>   static bool wake_up_full_nohz_cpu(int cpu)
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog