linux-kernel - Re: [RFC][PATCH 06/11] sched: Fix trace_sched

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150929113812.0bc594d0@gandalf.local.home>
Date:	Tue, 29 Sep 2015 11:38:12 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	mingo@...nel.org, linux-kernel@...r.kernel.org,
	torvalds@...ux-foundation.org, fweisbec@...il.com, oleg@...hat.com,
	umgwanakikbuti@...il.com, tglx@...utronix.de
Subject: Re: [RFC][PATCH 06/11] sched: Fix trace_sched_switch()

On Tue, 29 Sep 2015 11:28:31 +0200
Peter Zijlstra <peterz@...radead.org> 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@...radead.org>
> ---
>  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@...dmis.org>

-- Steve

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