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

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 <[email protected]>

---
  tools/perf/builtin-sched.c | 19 ++++++++-----------
  1 file changed, 8 insertions(+), 11 deletions(-)

tools/perf/builtin-sched.c +8 -11

index d3fb0ed..5b2fc62 100644
@@ -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,

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 :)

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 <[email protected]>

---
 tools/perf/builtin-sched.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

tools/perf/builtin-sched.c +8 -11

index d3fb0ed..5b2fc62 100644
@@ -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,

Signature · 1 line

1.8.2.1

any ACKs?

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

Quoted 1 line · Peter Zijlstra

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

Always 0, or random value?

Thanks,

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

Quoted 4 lines · Ingo Molnar

Quoted 1 line · Peter Zijlstra

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.

Quoted 7 lines · Dongsheng Yang

Quoted 4 lines · Ingo Molnar

Quoted 1 line · Peter Zijlstra

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

Always 0, or random value?

Hi Ingo,

Quoted 7 lines

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.

Quoted 4 lines · Peter Zijlstra

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.

Quoted 8 lines · Dongsheng Yang

Quoted 4 lines · Ingo Molnar

Quoted 1 line · Peter Zijlstra

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

Always 0, or random value?

Hi Ingo,

Quoted 7 lines

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.

Quoted 27 lines · Peter Zijlstra

Quoted 8 lines · Dongsheng Yang

Quoted 4 lines · Ingo Molnar

Quoted 1 line · Peter Zijlstra

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

Always 0, or random value?

Hi Ingo,

Quoted 7 lines

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.

Quoted 1 line

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.

Quoted 1 line · Peter Zijlstra

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

Always 0, or random value?