Re: [RFC][PATCH 06/11] sched: Fix trace_sched_switch()

[Steven Rostedt]

On Tue, 29 Sep 2015 11:28:31 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> __trace_sched_switch_state() is the last remaining PREEMPT_ACTIVE
> user, move trace_sched_switch() from prepare_task_switch() to
> __schedule() and propagate the @preempt argument.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
>  include/trace/events/sched.h      |   22 +++++++++-------------
>  kernel/sched/core.c               |    2 +-
>  kernel/trace/ftrace.c             |    2 +-
>  kernel/trace/trace_sched_switch.c |    3 ++-
>  kernel/trace/trace_sched_wakeup.c |    2 +-
>  5 files changed, 14 insertions(+), 17 deletions(-)
> 
> --- a/include/trace/events/sched.h
> +++ b/include/trace/events/sched.h
> @@ -104,22 +104,17 @@ DEFINE_EVENT(sched_wakeup_template, sche
>  	     TP_ARGS(p));
>  
>  #ifdef CREATE_TRACE_POINTS
> -static inline long __trace_sched_switch_state(struct task_struct *p)
> +static inline long __trace_sched_switch_state(bool preempt, struct task_struct *p)
>  {
> -	long state = p->state;
> -
> -#ifdef CONFIG_PREEMPT
>  #ifdef CONFIG_SCHED_DEBUG
>  	BUG_ON(p != current);
>  #endif /* CONFIG_SCHED_DEBUG */
> +
>  	/*
> -	 * For all intents and purposes a preempted task is a running task.
> +	 * Preemption ignores task state, therefore preempted tasks are always
> +	 * RUNNING (we will not have dequeued if state != RUNNING).
>  	 */
> -	if (preempt_count() & PREEMPT_ACTIVE)
> -		state = TASK_RUNNING | TASK_STATE_MAX;
> -#endif /* CONFIG_PREEMPT */
> -
> -	return state;
> +	return preempt ? TASK_RUNNING | TASK_STATE_MAX : p->state;
>  }

Hmm, this original change screwed up kernelshark, as it used the
state to determine if something was preempted or not. Because now you
always show a task as running, it can't do that anymore. I think I
bitched about this before.

What about nuking the above and just export to the sched_switch
tracepoint the fact that it was preempted. We now have that information
passed to it.

As everything should be using the parsing files, it should not break
any tools to export it.

As for this patch and with the current state, I see nothing wrong with
it.

Reviewed-by: Steven Rostedt <rostedt@xxxxxxxxxxx>

-- Steve

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