perf,tools: Remove usage of trace_sched_wakeup(.success)

perf,tools: Remove usage of trace_sched_wakeup(.success)

[Peter Zijlstra]

trace_sched_wakeup(.success) is a dead argument and has been for ages,
the only reason its still there is because of brain dead software, which
apparently includes perf tools

There's a few more instances in pearly snake shit, but that's not
supported as far as I care anyhow, so let that bitrot.

Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 tools/perf/builtin-sched.c        | 7 +------
 tools/perf/tests/evsel-tp-sched.c | 3 ---
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index d3fb0ed7240a..ef33a2632abe 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1007,17 +1007,12 @@ static int latency_wakeup_event(struct perf_sched *sched,
 				struct perf_sample *sample,
 				struct machine *machine)
 {
-	const u32 pid	  = perf_evsel__intval(evsel, sample, "pid"),
-		  success = perf_evsel__intval(evsel, sample, "success");
+	const u32 pid	  = perf_evsel__intval(evsel, sample, "pid");
 	struct work_atoms *atoms;
 	struct work_atom *atom;
 	struct thread *wakee;
 	u64 timestamp = sample->time;
 
-	/* Note for later, it may be interesting to observe the failing cases */
-	if (!success)
-		return 0;
-
 	wakee = machine__findnew_thread(machine, 0, pid);
 	atoms = thread_atoms_search(&sched->atom_root, wakee, &sched->cmp_pid);
 	if (!atoms) {
diff --git a/tools/perf/tests/evsel-tp-sched.c b/tools/perf/tests/evsel-tp-sched.c
index 4774f7fbb758..35d7fdb2328d 100644
--- a/tools/perf/tests/evsel-tp-sched.c
+++ b/tools/perf/tests/evsel-tp-sched.c
@@ -74,9 +74,6 @@ int test__perf_evsel__tp_sched_test(void)
 	if (perf_evsel__test_field(evsel, "prio", 4, true))
 		ret = -1;
 
-	if (perf_evsel__test_field(evsel, "success", 4, true))
-		ret = -1;
-
 	if (perf_evsel__test_field(evsel, "target_cpu", 4, true))
 		ret = -1;
 

[PATCH] perf_tools/sched: Remove nr_state_machine_bugs in perf latency.

[Dongsheng Yang]

As we do not use .success in sched_wakeup event any more, then
we can not guarantee that the task when wakeup event happen is
out of run queue. So the message of nr_state_machine_bugs is
not correct.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/builtin-sched.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index d3fb0ed..5b2fc62 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -149,7 +149,6 @@ struct perf_sched {
 	unsigned long	 nr_runs;
 	unsigned long	 nr_timestamps;
 	unsigned long	 nr_unordered_timestamps;
-	unsigned long	 nr_state_machine_bugs;
 	unsigned long	 nr_context_switch_bugs;
 	unsigned long	 nr_events;
 	unsigned long	 nr_lost_chunks;
@@ -1037,12 +1036,18 @@ static int latency_wakeup_event(struct perf_sched *sched,
 	atom = list_entry(atoms->work_list.prev, struct work_atom, list);
 
 	/*
+	 * As we do not guarantee the wakeup event happens when
+	 * task is out of run queue, also may happen when task is
+	 * on run queue and wakeup only change ->state to TASK_RUNNING,
+	 * then we should not set the ->wake_up_time when wake up a
+	 * task which is on run queue. 
+	 *
 	 * You WILL be missing events if you've recorded only
 	 * one CPU, or are only looking at only one, so don't
-	 * make useless noise.
+	 * skip in this case.
 	 */
 	if (sched->profile_cpu == -1 && atom->state != THREAD_SLEEPING)
-		sched->nr_state_machine_bugs++;
+		return 0;
 
 	sched->nr_timestamps++;
 	if (atom->sched_out_time > timestamp) {
@@ -1496,14 +1501,6 @@ static void print_bad_events(struct perf_sched *sched)
 			(double)sched->nr_lost_events/(double)sched->nr_events * 100.0,
 			sched->nr_lost_events, sched->nr_events, sched->nr_lost_chunks);
 	}
-	if (sched->nr_state_machine_bugs && sched->nr_timestamps) {
-		printf("  INFO: %.3f%% state machine bugs (%ld out of %ld)",
-			(double)sched->nr_state_machine_bugs/(double)sched->nr_timestamps*100.0,
-			sched->nr_state_machine_bugs, sched->nr_timestamps);
-		if (sched->nr_lost_events)
-			printf(" (due to lost events?)");
-		printf("\n");
-	}
 	if (sched->nr_context_switch_bugs && sched->nr_timestamps) {
 		printf("  INFO: %.3f%% context switch bugs (%ld out of %ld)",
 			(double)sched->nr_context_switch_bugs/(double)sched->nr_timestamps*100.0,
-- 
1.8.2.1

Re: [PATCH] perf_tools/sched: Remove nr_state_machine_bugs in perf latency.

[Dongsheng Yang]

Hi jiri or Arnaldo,
     It seems Peter really do not like the usage of 
sched_wakeup(.success), and
don't plan to support it in scheduler any more.  Please consider to 
append this patch
too when you take the patch from Peter.  Thanx :)

On 05/13/2014 10:38 AM, Dongsheng Yang wrote:
> As we do not use .success in sched_wakeup event any more, then
> we can not guarantee that the task when wakeup event happen is
> out of run queue. So the message of nr_state_machine_bugs is
> not correct.
>
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>   tools/perf/builtin-sched.c | 19 ++++++++-----------
>   1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index d3fb0ed..5b2fc62 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -149,7 +149,6 @@ struct perf_sched {
>   	unsigned long	 nr_runs;
>   	unsigned long	 nr_timestamps;
>   	unsigned long	 nr_unordered_timestamps;
> -	unsigned long	 nr_state_machine_bugs;
>   	unsigned long	 nr_context_switch_bugs;
>   	unsigned long	 nr_events;
>   	unsigned long	 nr_lost_chunks;
> @@ -1037,12 +1036,18 @@ static int latency_wakeup_event(struct perf_sched *sched,
>   	atom = list_entry(atoms->work_list.prev, struct work_atom, list);
>   
>   	/*
> +	 * As we do not guarantee the wakeup event happens when
> +	 * task is out of run queue, also may happen when task is
> +	 * on run queue and wakeup only change ->state to TASK_RUNNING,
> +	 * then we should not set the ->wake_up_time when wake up a
> +	 * task which is on run queue.
> +	 *
>   	 * You WILL be missing events if you've recorded only
>   	 * one CPU, or are only looking at only one, so don't
> -	 * make useless noise.
> +	 * skip in this case.
>   	 */
>   	if (sched->profile_cpu == -1 && atom->state != THREAD_SLEEPING)
> -		sched->nr_state_machine_bugs++;
> +		return 0;
>   
>   	sched->nr_timestamps++;
>   	if (atom->sched_out_time > timestamp) {
> @@ -1496,14 +1501,6 @@ static void print_bad_events(struct perf_sched *sched)
>   			(double)sched->nr_lost_events/(double)sched->nr_events * 100.0,
>   			sched->nr_lost_events, sched->nr_events, sched->nr_lost_chunks);
>   	}
> -	if (sched->nr_state_machine_bugs && sched->nr_timestamps) {
> -		printf("  INFO: %.3f%% state machine bugs (%ld out of %ld)",
> -			(double)sched->nr_state_machine_bugs/(double)sched->nr_timestamps*100.0,
> -			sched->nr_state_machine_bugs, sched->nr_timestamps);
> -		if (sched->nr_lost_events)
> -			printf(" (due to lost events?)");
> -		printf("\n");
> -	}
>   	if (sched->nr_context_switch_bugs && sched->nr_timestamps) {
>   		printf("  INFO: %.3f%% context switch bugs (%ld out of %ld)",
>   			(double)sched->nr_context_switch_bugs/(double)sched->nr_timestamps*100.0,

Re: [PATCH] perf_tools/sched: Remove nr_state_machine_bugs in perf latency.

[Jiri Olsa]

On Tue, May 13, 2014 at 10:42:12AM +0900, Dongsheng Yang wrote:
> Hi jiri or Arnaldo,
>     It seems Peter really do not like the usage of
> sched_wakeup(.success), and
> don't plan to support it in scheduler any more.  Please consider to
> append this patch
> too when you take the patch from Peter.  Thanx :)

sure, any acks anyone? ;-)

thanks,
jirka

Re: [PATCH] perf_tools/sched: Remove nr_state_machine_bugs in perf latency.

[Jiri Olsa]

any ACKs?

thanks,
jirka

On Tue, May 13, 2014 at 10:38:21AM +0900, Dongsheng Yang wrote:
> As we do not use .success in sched_wakeup event any more, then
> we can not guarantee that the task when wakeup event happen is
> out of run queue. So the message of nr_state_machine_bugs is
> not correct.
> 
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>  tools/perf/builtin-sched.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index d3fb0ed..5b2fc62 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -149,7 +149,6 @@ struct perf_sched {
>  	unsigned long	 nr_runs;
>  	unsigned long	 nr_timestamps;
>  	unsigned long	 nr_unordered_timestamps;
> -	unsigned long	 nr_state_machine_bugs;
>  	unsigned long	 nr_context_switch_bugs;
>  	unsigned long	 nr_events;
>  	unsigned long	 nr_lost_chunks;
> @@ -1037,12 +1036,18 @@ static int latency_wakeup_event(struct perf_sched *sched,
>  	atom = list_entry(atoms->work_list.prev, struct work_atom, list);
>  
>  	/*
> +	 * As we do not guarantee the wakeup event happens when
> +	 * task is out of run queue, also may happen when task is
> +	 * on run queue and wakeup only change ->state to TASK_RUNNING,
> +	 * then we should not set the ->wake_up_time when wake up a
> +	 * task which is on run queue. 
> +	 *
>  	 * You WILL be missing events if you've recorded only
>  	 * one CPU, or are only looking at only one, so don't
> -	 * make useless noise.
> +	 * skip in this case.
>  	 */
>  	if (sched->profile_cpu == -1 && atom->state != THREAD_SLEEPING)
> -		sched->nr_state_machine_bugs++;
> +		return 0;
>  
>  	sched->nr_timestamps++;
>  	if (atom->sched_out_time > timestamp) {
> @@ -1496,14 +1501,6 @@ static void print_bad_events(struct perf_sched *sched)
>  			(double)sched->nr_lost_events/(double)sched->nr_events * 100.0,
>  			sched->nr_lost_events, sched->nr_events, sched->nr_lost_chunks);
>  	}
> -	if (sched->nr_state_machine_bugs && sched->nr_timestamps) {
> -		printf("  INFO: %.3f%% state machine bugs (%ld out of %ld)",
> -			(double)sched->nr_state_machine_bugs/(double)sched->nr_timestamps*100.0,
> -			sched->nr_state_machine_bugs, sched->nr_timestamps);
> -		if (sched->nr_lost_events)
> -			printf(" (due to lost events?)");
> -		printf("\n");
> -	}
>  	if (sched->nr_context_switch_bugs && sched->nr_timestamps) {
>  		printf("  INFO: %.3f%% context switch bugs (%ld out of %ld)",
>  			(double)sched->nr_context_switch_bugs/(double)sched->nr_timestamps*100.0,
> -- 
> 1.8.2.1
> 

Re: [PATCH] perf_tools/sched: Remove nr_state_machine_bugs in perf latency.

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 128 bytes --]

On Mon, May 19, 2014 at 01:59:57PM +0200, Jiri Olsa wrote:
> any ACKs?
> 

Acked-by: Peter Zijlstra <peterz@infradead.org>

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[tip:perf/core] perf sched: Remove nr_state_machine_bugs in perf latency

[tip-bot for Dongsheng Yang]

Commit-ID:  67d6259dd021006ade25d67b045ad2089b5aba96
Gitweb:     http://git.kernel.org/tip/67d6259dd021006ade25d67b045ad2089b5aba96
Author:     Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
AuthorDate: Tue, 13 May 2014 10:38:21 +0900
Committer:  Jiri Olsa <jolsa@kernel.org>
CommitDate: Fri, 16 May 2014 09:17:36 +0200

perf sched: Remove nr_state_machine_bugs in perf latency

As we do not use .success in sched_wakeup event any more, then
we can not guarantee that the task when wakeup event happen is
out of run queue. So the message of nr_state_machine_bugs is
not correct.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Link: http://lkml.kernel.org/r/1399945101-21736-1-git-send-email-yangds.fnst@cn.fujitsu.com
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-sched.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index a3320f1..0b4fe53 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -149,7 +149,6 @@ struct perf_sched {
 	unsigned long	 nr_runs;
 	unsigned long	 nr_timestamps;
 	unsigned long	 nr_unordered_timestamps;
-	unsigned long	 nr_state_machine_bugs;
 	unsigned long	 nr_context_switch_bugs;
 	unsigned long	 nr_events;
 	unsigned long	 nr_lost_chunks;
@@ -1032,12 +1031,18 @@ static int latency_wakeup_event(struct perf_sched *sched,
 	atom = list_entry(atoms->work_list.prev, struct work_atom, list);
 
 	/*
+	 * As we do not guarantee the wakeup event happens when
+	 * task is out of run queue, also may happen when task is
+	 * on run queue and wakeup only change ->state to TASK_RUNNING,
+	 * then we should not set the ->wake_up_time when wake up a
+	 * task which is on run queue.
+	 *
 	 * You WILL be missing events if you've recorded only
 	 * one CPU, or are only looking at only one, so don't
-	 * make useless noise.
+	 * skip in this case.
 	 */
 	if (sched->profile_cpu == -1 && atom->state != THREAD_SLEEPING)
-		sched->nr_state_machine_bugs++;
+		return 0;
 
 	sched->nr_timestamps++;
 	if (atom->sched_out_time > timestamp) {
@@ -1496,14 +1501,6 @@ static void print_bad_events(struct perf_sched *sched)
 			(double)sched->nr_lost_events/(double)sched->nr_events * 100.0,
 			sched->nr_lost_events, sched->nr_events, sched->nr_lost_chunks);
 	}
-	if (sched->nr_state_machine_bugs && sched->nr_timestamps) {
-		printf("  INFO: %.3f%% state machine bugs (%ld out of %ld)",
-			(double)sched->nr_state_machine_bugs/(double)sched->nr_timestamps*100.0,
-			sched->nr_state_machine_bugs, sched->nr_timestamps);
-		if (sched->nr_lost_events)
-			printf(" (due to lost events?)");
-		printf("\n");
-	}
 	if (sched->nr_context_switch_bugs && sched->nr_timestamps) {
 		printf("  INFO: %.3f%% context switch bugs (%ld out of %ld)",
 			(double)sched->nr_context_switch_bugs/(double)sched->nr_timestamps*100.0,

Re: perf,tools: Remove usage of trace_sched_wakeup(.success)

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> trace_sched_wakeup(.success) is a dead argument and has been for ages,

Always 0, or random value?

Thanks,

	Ingo

Re: perf,tools: Remove usage of trace_sched_wakeup(.success)

[Dongsheng Yang]

On 05/13/2014 04:22 PM, Ingo Molnar wrote:
> * Peter Zijlstra <peterz@infradead.org> wrote:
>
>> trace_sched_wakeup(.success) is a dead argument and has been for ages,
> Always 0, or random value?

Hi Ingo,

It is always 1 currently.

Peter believe that .success is not useful and I pointed that perf sched 
latency
is using it now. Then he post this patch to remove the usage here.

Please go to the following link for more about this issue.

https://lkml.org/lkml/2014/5/12/77

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

Re: perf,tools: Remove usage of trace_sched_wakeup(.success)

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1026 bytes --]

On Tue, May 13, 2014 at 03:34:32PM +0900, Dongsheng Yang wrote:
> On 05/13/2014 04:22 PM, Ingo Molnar wrote:
> >* Peter Zijlstra <peterz@infradead.org> wrote:
> >
> >>trace_sched_wakeup(.success) is a dead argument and has been for ages,
> >Always 0, or random value?
> 
> Hi Ingo,
> 
> It is always 1 currently.
> 
> Peter believe that .success is not useful and I pointed that perf sched
> latency
> is using it now. Then he post this patch to remove the usage here.
> 
> Please go to the following link for more about this issue.

It is _not_ usable. You're proposing to abuse the existing parameter. A
wakeup doing an enqueue or not has nothing _WHAT_SO_EVER_ to do with
success.

Now what I think you wanted to do is make it easier to match
trace_sched_switch() statements with trace_sched_wakeup() statements.
And since you only get the trace_sched_switch() on dequeue, you want to
know which trace_sched_wakeup() calls did an enqueue.

But that's completely and utterly unrelated to success.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: perf,tools: Remove usage of trace_sched_wakeup(.success)

[Dongsheng Yang]

On 05/13/2014 07:03 PM, Peter Zijlstra wrote:
> On Tue, May 13, 2014 at 03:34:32PM +0900, Dongsheng Yang wrote:
>> On 05/13/2014 04:22 PM, Ingo Molnar wrote:
>>> * Peter Zijlstra <peterz@infradead.org> wrote:
>>>
>>>> trace_sched_wakeup(.success) is a dead argument and has been for ages,
>>> Always 0, or random value?
>> Hi Ingo,
>>
>> It is always 1 currently.
>>
>> Peter believe that .success is not useful and I pointed that perf sched
>> latency
>> is using it now. Then he post this patch to remove the usage here.
>>
>> Please go to the following link for more about this issue.
> It is _not_ usable. You're proposing to abuse the existing parameter. A
> wakeup doing an enqueue or not has nothing _WHAT_SO_EVER_ to do with
> success.
>
> Now what I think you wanted to do is make it easier to match
> trace_sched_switch() statements with trace_sched_wakeup() statements.
> And since you only get the trace_sched_switch() on dequeue, you want to
> know which trace_sched_wakeup() calls did an enqueue.

Ha, yes, indeed. In perf sched latency, we need to know the timestamp
when a task enqueue and then we can calculate the delay time.
So I want to take the use of success parameter in trace_sched_wakeup()
to indicate that *this* wakeup did an enqueue.

But now I think it is okey if you really mind adding more tracepoints in
scheduler. And I posted a patch after your patch in this thread to make
perf sched latency work well.
>
> But that's completely and utterly unrelated to success.

Re: perf,tools: Remove usage of trace_sched_wakeup(.success)

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

On Tue, May 13, 2014 at 07:10:02PM +0900, Dongsheng Yang wrote:
> On 05/13/2014 07:03 PM, Peter Zijlstra wrote:

> >Now what I think you wanted to do is make it easier to match
> >trace_sched_switch() statements with trace_sched_wakeup() statements.
> >And since you only get the trace_sched_switch() on dequeue, you want to
> >know which trace_sched_wakeup() calls did an enqueue.
> 
> Ha, yes, indeed. In perf sched latency, we need to know the timestamp
> when a task enqueue and then we can calculate the delay time.
> So I want to take the use of success parameter in trace_sched_wakeup()
> to indicate that *this* wakeup did an enqueue.
> 
> But now I think it is okey if you really mind adding more tracepoints in
> scheduler. And I posted a patch after your patch in this thread to make
> perf sched latency work well.

I don't mind adding them per-se, as long as there's a reasonable effort
showing it doesn't slow down the wakeup-path.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: perf,tools: Remove usage of trace_sched_wakeup(.success)

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, May 13, 2014 at 03:34:32PM +0900, Dongsheng Yang wrote:
> > On 05/13/2014 04:22 PM, Ingo Molnar wrote:
> > >* Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > >>trace_sched_wakeup(.success) is a dead argument and has been for ages,
> > >Always 0, or random value?
> > 
> > Hi Ingo,
> > 
> > It is always 1 currently.
> > 
> > Peter believe that .success is not useful and I pointed that perf 
> > sched latency is using it now. Then he post this patch to remove 
> > the usage here.
> > 
> > Please go to the following link for more about this issue.
> 
> It is _not_ usable. You're proposing to abuse the existing 
> parameter. A wakeup doing an enqueue or not has nothing 
> _WHAT_SO_EVER_ to do with success.
> 
> Now what I think you wanted to do is make it easier to match 
> trace_sched_switch() statements with trace_sched_wakeup() 
> statements. And since you only get the trace_sched_switch() on 
> dequeue, you want to know which trace_sched_wakeup() calls did an 
> enqueue.
> 
> But that's completely and utterly unrelated to success.

So I always considered it 'the enqueue was successful' - that's I 
think why I added it to 'perf sched' originally - to be able to trace 
wakeups from originator to target.

Thans,

	Ingo

Re: perf,tools: Remove usage of trace_sched_wakeup(.success)

[Peter Zijlstra]

On Tue, May 13, 2014 at 07:14:36PM +0200, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Tue, May 13, 2014 at 03:34:32PM +0900, Dongsheng Yang wrote:
> > > On 05/13/2014 04:22 PM, Ingo Molnar wrote:
> > > >* Peter Zijlstra <peterz@infradead.org> wrote:
> > > >
> > > >>trace_sched_wakeup(.success) is a dead argument and has been for ages,
> > > >Always 0, or random value?
> > > 
> > > Hi Ingo,
> > > 
> > > It is always 1 currently.
> > > 
> > > Peter believe that .success is not useful and I pointed that perf 
> > > sched latency is using it now. Then he post this patch to remove 
> > > the usage here.
> > > 
> > > Please go to the following link for more about this issue.
> > 
> > It is _not_ usable. You're proposing to abuse the existing 
> > parameter. A wakeup doing an enqueue or not has nothing 
> > _WHAT_SO_EVER_ to do with success.
> > 
> > Now what I think you wanted to do is make it easier to match 
> > trace_sched_switch() statements with trace_sched_wakeup() 
> > statements. And since you only get the trace_sched_switch() on 
> > dequeue, you want to know which trace_sched_wakeup() calls did an 
> > enqueue.
> > 
> > But that's completely and utterly unrelated to success.
> 
> So I always considered it 'the enqueue was successful' - that's I 
> think why I added it to 'perf sched' originally - to be able to trace 
> wakeups from originator to target.

So back when it still sometimes returned 0 it was the try_to_wake_up()
return value, it still is, except when we re factored ttwu we moved the
tracepoint such that its not on the 0 return path.

I would've removed it then, except I couldn't because some crap
userspace was using it -- and that's the only reason its still in there,
its been a pointless rudiment for years.

Anyway, the return value is true if it (potentially) did an actual
wakeup, and that is unrelated to having done an enqueue.

And in general you want wakeups to happen before we do the dequeue, as
the enqueue-less wakeups are _way_ faster. I've tried optimistic
yield(), but since not all schedule() site can deal with spurious
wakeups that fell flat :/

Re: perf,tools: Remove usage of trace_sched_wakeup(.success)

[Peter Zijlstra]

Anyway, ideally we'd do something like the below, except of course that
it'll break userspace :-(

And that is why I loathe tracepoints.

Also, I've not looked at the code difference and or performance
implications.

---
Subject: sched: Add enqueue information to trace_sched_wakeup()

trace_sched_switch() shows task dequeues, trace_sched_wakeup() shows all
wakeups, including those that do not enqueue (because the task is
still enqueued).

In order to easy matching dequeue to enqueue, add enqueue information to
trace_sched_wakeup().

Replace the trace_sched_wakeup() success argument which is the
try_to_wake_up() return value and always true (since we removed
tracing the false path).

Not-signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 include/trace/events/sched.h | 16 ++++++++--------
 kernel/sched/core.c          | 42 ++++++++++++++++++++++--------------------
 kernel/sched/sched.h         |  3 ++-
 3 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index 67e1bbf83695..5e5037f794f2 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -55,15 +55,15 @@ TRACE_EVENT(sched_kthread_stop_ret,
  */
 DECLARE_EVENT_CLASS(sched_wakeup_template,
 
-	TP_PROTO(struct task_struct *p, int success),
+	TP_PROTO(struct task_struct *p, int enqueue),
 
-	TP_ARGS(__perf_task(p), success),
+	TP_ARGS(__perf_task(p), enqueue),
 
 	TP_STRUCT__entry(
 		__array(	char,	comm,	TASK_COMM_LEN	)
 		__field(	pid_t,	pid			)
 		__field(	int,	prio			)
-		__field(	int,	success			)
+		__field(	int,	enqueue			)
 		__field(	int,	target_cpu		)
 	),
 
@@ -71,24 +71,24 @@ DECLARE_EVENT_CLASS(sched_wakeup_template,
 		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
 		__entry->pid		= p->pid;
 		__entry->prio		= p->prio;
-		__entry->success	= success;
+		__entry->enqueue	= enqueue;
 		__entry->target_cpu	= task_cpu(p);
 	),
 
-	TP_printk("comm=%s pid=%d prio=%d success=%d target_cpu=%03d",
+	TP_printk("comm=%s pid=%d prio=%d enqueue=%d target_cpu=%03d",
 		  __entry->comm, __entry->pid, __entry->prio,
-		  __entry->success, __entry->target_cpu)
+		  __entry->enqueue, __entry->target_cpu)
 );
 
 DEFINE_EVENT(sched_wakeup_template, sched_wakeup,
-	     TP_PROTO(struct task_struct *p, int success),
+	     TP_PROTO(struct task_struct *p, int enqueue),
 	     TP_ARGS(p, success));
 
 /*
  * Tracepoint for waking up a new task:
  */
 DEFINE_EVENT(sched_wakeup_template, sched_wakeup_new,
-	     TP_PROTO(struct task_struct *p, int success),
+	     TP_PROTO(struct task_struct *p, int enqueue),
 	     TP_ARGS(p, success));
 
 #ifdef CREATE_TRACE_POINTS
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4ea7b3f1a087..e29b87748680 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1461,10 +1461,22 @@ static void ttwu_activate(struct rq *rq, struct task_struct *p, int en_flags)
  * Mark the task runnable and perform wakeup-preemption.
  */
 static void
-ttwu_do_wakeup(struct rq *rq, struct task_struct *p, int wake_flags)
+ttwu_do_wakeup(struct rq *rq, struct task_struct *p,
+	       int wake_flags, en_flags)
 {
+	bool enqueue = wake_flags & WF_ENQUEUE;
+
+	if (enqueue) {
+#ifdef CONFIG_SMP
+		if (p->sched_contributes_to_load)
+			rq->nr_uninterruptible--;
+#endif
+
+		ttwu_activate(rq, p, en_flags);
+	}
+
 	check_preempt_curr(rq, p, wake_flags);
-	trace_sched_wakeup(p, true);
+	trace_sched_wakeup(p, enqueue);
 
 	p->state = TASK_RUNNING;
 #ifdef CONFIG_SMP
@@ -1485,18 +1497,6 @@ ttwu_do_wakeup(struct rq *rq, struct task_struct *p, int wake_flags)
 #endif
 }
 
-static void
-ttwu_do_activate(struct rq *rq, struct task_struct *p, int wake_flags)
-{
-#ifdef CONFIG_SMP
-	if (p->sched_contributes_to_load)
-		rq->nr_uninterruptible--;
-#endif
-
-	ttwu_activate(rq, p, ENQUEUE_WAKEUP | ENQUEUE_WAKING);
-	ttwu_do_wakeup(rq, p, wake_flags);
-}
-
 /*
  * Called in case the task @p isn't fully descheduled from its runqueue,
  * in this case we must do a remote wakeup. Its a 'light' wakeup though,
@@ -1512,7 +1512,7 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
 	if (p->on_rq) {
 		/* check_preempt_curr() may use rq clock */
 		update_rq_clock(rq);
-		ttwu_do_wakeup(rq, p, wake_flags);
+		ttwu_do_wakeup(rq, p, wake_flags, 0);
 		ret = 1;
 	}
 	__task_rq_unlock(rq);
@@ -1532,7 +1532,8 @@ static void sched_ttwu_pending(void)
 	while (llist) {
 		p = llist_entry(llist, struct task_struct, wake_entry);
 		llist = llist_next(llist);
-		ttwu_do_activate(rq, p, 0);
+		ttwu_do_wakeup(rq, p, WF_ENQUEUE, 
+			       ENQUEUE_WAKEUP | ENQUEUE_WAKING);
 	}
 
 	raw_spin_unlock(&rq->lock);
@@ -1604,7 +1605,7 @@ static void ttwu_queue(struct task_struct *p, int cpu)
 #endif
 
 	raw_spin_lock(&rq->lock);
-	ttwu_do_activate(rq, p, 0);
+	ttwu_do_wakeup(rq, p, WF_ENQUEUE, ENQUEUE_WAKEUP | ENQUEUE_WAKING);
 	raw_spin_unlock(&rq->lock);
 }
 
@@ -1707,10 +1708,11 @@ static void try_to_wake_up_local(struct task_struct *p)
 	if (!(p->state & TASK_NORMAL))
 		goto out;
 
-	if (!p->on_rq)
-		ttwu_activate(rq, p, ENQUEUE_WAKEUP);
+	if (p->on_rq)
+		ttwu_do_wakeup(rq, p, 0, 0);
+	else
+		ttwu_do_wakeup(rq, p, WF_ENQUEUE, ENQUEUE_WAKEUP);
 
-	ttwu_do_wakeup(rq, p, 0);
 	ttwu_stat(p, smp_processor_id(), 0);
 out:
 	raw_spin_unlock(&p->pi_lock);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b2cbe81308af..d865415ea1d2 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1026,7 +1026,8 @@ static inline void finish_lock_switch(struct rq *rq, struct task_struct *prev)
  */
 #define WF_SYNC		0x01		/* waker goes to sleep after wakeup */
 #define WF_FORK		0x02		/* child wakeup after fork */
-#define WF_MIGRATED	0x4		/* internal use, task got migrated */
+#define WF_MIGRATED	0x04		/* internal use, task got migrated */
+#define WF_ENQUEUE	0x08
 
 /*
  * To aid in avoiding the subversion of "niceness" due to uneven distribution

Re: perf,tools: Remove usage of trace_sched_wakeup(.success)

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

On Tue, May 13, 2014 at 09:22:08AM +0200, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > trace_sched_wakeup(.success) is a dead argument and has been for ages,
> 
> Always 0, or random value?

always true, we couldn't remove it because it would break userspace :-(

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[tip:perf/core] perf tools: Remove usage of trace_sched_wakeup( .success)

[tip-bot for Peter Zijlstra]

Commit-ID:  0680ee7db16de9c02d1d4b1a935a5daf754fe8a1
Gitweb:     http://git.kernel.org/tip/0680ee7db16de9c02d1d4b1a935a5daf754fe8a1
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Mon, 12 May 2014 20:19:46 +0200
Committer:  Jiri Olsa <jolsa@kernel.org>
CommitDate: Mon, 12 May 2014 21:13:44 +0200

perf tools: Remove usage of trace_sched_wakeup(.success)

trace_sched_wakeup(.success) is a dead argument and has been for ages,
the only reason its still there is because of brain dead software, which
apparently includes perf tools

There's a few more instances in pearly snake shit, but that's not
supported as far as I care anyhow, so let that bitrot.

Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20140512181946.GG13467@laptop.programming.kicks-ass.net
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-sched.c        | 7 +------
 tools/perf/tests/evsel-tp-sched.c | 3 ---
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 2579215..a3320f1 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1007,17 +1007,12 @@ static int latency_wakeup_event(struct perf_sched *sched,
 				struct perf_sample *sample,
 				struct machine *machine)
 {
-	const u32 pid	  = perf_evsel__intval(evsel, sample, "pid"),
-		  success = perf_evsel__intval(evsel, sample, "success");
+	const u32 pid	  = perf_evsel__intval(evsel, sample, "pid");
 	struct work_atoms *atoms;
 	struct work_atom *atom;
 	struct thread *wakee;
 	u64 timestamp = sample->time;
 
-	/* Note for later, it may be interesting to observe the failing cases */
-	if (!success)
-		return 0;
-
 	wakee = machine__findnew_thread(machine, 0, pid);
 	atoms = thread_atoms_search(&sched->atom_root, wakee, &sched->cmp_pid);
 	if (!atoms) {
diff --git a/tools/perf/tests/evsel-tp-sched.c b/tools/perf/tests/evsel-tp-sched.c
index 4774f7f..35d7fdb 100644
--- a/tools/perf/tests/evsel-tp-sched.c
+++ b/tools/perf/tests/evsel-tp-sched.c
@@ -74,9 +74,6 @@ int test__perf_evsel__tp_sched_test(void)
 	if (perf_evsel__test_field(evsel, "prio", 4, true))
 		ret = -1;
 
-	if (perf_evsel__test_field(evsel, "success", 4, true))
-		ret = -1;
-
 	if (perf_evsel__test_field(evsel, "target_cpu", 4, true))
 		ret = -1;