Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1339735 > unrolled thread

[PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Daniel Bristot de Oliveira <bristot@redhat.com> - 2016-02-22 18:10 +0100
    Re: [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Peter Zijlstra <peterz@infradead.org> - 2016-02-22 18:40 +0100
      Re: [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Steven Rostedt <rostedt@goodmis.org> - 2016-02-22 18:50 +0100
        Re: [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Daniel Bristot de Oliveira <bristot@redhat.com> - 2016-02-22 21:20 +0100
        Re: [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Peter Zijlstra <peterz@infradead.org> - 2016-02-22 22:40 +0100
          Re: [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Steven Rostedt <rostedt@goodmis.org> - 2016-02-22 23:40 +0100
            Re: [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Juri Lelli <juri.lelli@arm.com> - 2016-02-23 11:40 +0100
            Re: [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Peter Zijlstra <peterz@infradead.org> - 2016-02-23 11:50 +0100
              Re: [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Steven Rostedt <rostedt@goodmis.org> - 2016-02-23 14:20 +0100
                Re: [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Ingo Molnar <mingo@kernel.org> - 2016-02-24 09:50 +0100
              Re: [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Peter Zijlstra <peterz@infradead.org> - 2016-02-23 15:30 +0100
              Re: [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Daniel Bristot de Oliveira <bristot@redhat.com> - 2016-02-23 17:20 +0100
              Re: [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler Daniel Bristot de Oliveira <bristot@redhat.com> - 2016-02-24 03:30 +0100

#1339735 — [PATCH 3/4] sched/deadline: Tracepoints for deadline scheduler

Deadline tasks behave differently of other tasks because deadline
task's also depend on their period, deadline and runtime.

Hence, the well known sched:sched_wakeup and sched:sched_switch
tracepoints are not always enough to precisely explain the behavior of a
deadline task with respect to the task's period, deadline, and runtime
consumption and replenishment.

In order to provide more precise information about the scheduling of
deadline tasks, this patch implements the following tracepoints:

- sched:sched_deadline_replenish: Informs the runtime replenishment of
a deadline task. Trace example:

  <idle>-0     [010] d.h.   268.428878: sched_deadline_replenish: \
	comm=y pid=1608 now=268.428876113 deadline=268.458863627  \
	runtime=20000000

The task y-1608 was replenished with 20000000 ns at 268.428876113
to be used until the deadline at 268.458863627.

- sched:sched_deadline_yield: Informs that a deadline task called
sched_yield(), and will wait for the next period. Trace example:

  y-1608  [010] d...   268.428892: sched_deadline_yield: 	\
	now=268.428891932 deadline=268.458863627		\
	remaining_runtime=19995653

The task y-1608 yielded before its deadline, with 19995653 ns of
remaining runtime.

- sched:sched_deadline_throttle: Informs that a task consumed all its
available runtime and was throttled. Trace example:

  t-1613  [011] d.h.   280.419823: sched_deadline_throttle:	\
	now=280.419823282 deadline=280.430683206 		\
	remaining_runtime=-13280

The task t-1613 overrun its runtime by 13280 ns and was throttled.

- sched:sched_deadline_block: Informs that a deadline task went to sleep
waiting to be awakened by another task. Trace example:

  b-1611  [011] d...   276.228298: sched_deadline_block: 	\
	now=276.228295889 deadline=276.258262555
	remaining_runtime=19966666

The task b-1611 blocked waiting for an external event. Its deadline is at
276.258262555, and it stills have 19966666 ns of remaining runtime on the
current period.

Signed-off-by: Daniel Bristot de Oliveira <bristot@redhat.com>
---
 include/trace/events/sched.h | 87 ++++++++++++++++++++++++++++++++++++++++++++
 kernel/sched/deadline.c      | 16 +++++++-
 2 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index 9b90c57..1847c29 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -562,6 +562,93 @@ TRACE_EVENT(sched_wake_idle_without_ipi,
 
 	TP_printk("cpu=%d", __entry->cpu)
 );
+
+/*
+ * Tracepoint for runtime replenishment of deadline tasks:
+ */
+TRACE_EVENT(sched_deadline_replenish,
+
+	TP_PROTO(struct sched_dl_entity *dl_se),
+
+	TP_ARGS(dl_se),
+
+	TP_STRUCT__entry(
+		__array(	char,	comm,	TASK_COMM_LEN	)
+		__field(	pid_t,	pid			)
+		__field(	u64,	now			)
+		__field(	u64,	deadline		)
+		__field(	s64,	runtime			)
+	),
+
+	TP_fast_assign(
+		memcpy(__entry->comm, dl_task_of(dl_se)->comm, TASK_COMM_LEN);
+		__entry->now		= rq_clock(rq_of_dl_rq(dl_rq_of_se(dl_se)));
+		__entry->pid		= dl_task_of(dl_se)->pid;
+		__entry->deadline	= dl_se->deadline;
+		__entry->runtime	= dl_se->runtime;
+	),
+
+	TP_printk("comm=%s pid=%d now=%llu.%09u deadline=%llu.%09u runtime=%lld",
+		  __entry->comm,
+		  __entry->pid,
+		  __print_ns_to_secs(__entry->now),
+		  __print_ns_without_secs(__entry->now),
+		  __print_ns_to_secs(__entry->deadline),
+		  __print_ns_without_secs(__entry->deadline),
+		  __entry->runtime)
+);
+
+DECLARE_EVENT_CLASS(sched_deadline_template,
+
+	TP_PROTO(struct sched_dl_entity *dl_se),
+
+	TP_ARGS(dl_se),
+
+	TP_STRUCT__entry(
+		__field(	u64,	now			)
+		__field(	u64,	deadline		)
+		__field(	s64,	runtime			)
+	),
+
+	TP_fast_assign(
+		__entry->now		= rq_clock(rq_of_dl_rq(dl_rq_of_se(dl_se)));
+		__entry->deadline	= dl_se->deadline;
+		__entry->runtime	= dl_se->runtime;
+	),
+
+	TP_printk("now=%llu.%09u deadline=%llu.%09u remaining_runtime=%lld",
+		  __print_ns_to_secs(__entry->now),
+		  __print_ns_without_secs(__entry->now),
+		  __print_ns_to_secs(__entry->deadline),
+		  __print_ns_without_secs(__entry->deadline),
+		  __entry->runtime)
+);
+
+/*
+ * Tracepoint for sched_yield() of a deadline task (the task
+ * went to sleep waiting for the next period)
+ */
+DEFINE_EVENT(sched_deadline_template, sched_deadline_yield,
+	     TP_PROTO(struct sched_dl_entity *dl_se),
+	     TP_ARGS(dl_se));
+
+/*
+ * Tracepoint for throttling of a deadline task that consumed all its
+ * runtime
+ */
+DEFINE_EVENT_CONDITION(sched_deadline_template, sched_deadline_throttle,
+	     TP_PROTO(struct sched_dl_entity *dl_se),
+	     TP_ARGS(dl_se),
+	     TP_CONDITION(!dl_se->dl_yielded));
+
+/*
+ * Tracepoint for blocking of a deadline task. The deadline task was
+ * dequeued, but neither by being throttled nor yielding.
+ */
+DEFINE_EVENT_CONDITION(sched_deadline_template, sched_deadline_block,
+	     TP_PROTO(struct sched_dl_entity *dl_se),
+	     TP_ARGS(dl_se),
+	     TP_CONDITION(!dl_se->dl_yielded && !dl_se->dl_throttled));
 #endif /* _TRACE_SCHED_H */
 
 /* This part must be outside protection */
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 2f6d25a..56d0d09 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -17,6 +17,7 @@
 #include "sched.h"
 
 #include <linux/slab.h>
+#include <trace/events/sched.h>
 
 struct dl_bandwidth def_dl_bandwidth;
 
@@ -344,6 +345,7 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
 	dl_se->deadline = rq_clock(rq) + pi_se->dl_deadline;
 	dl_se->runtime = pi_se->dl_runtime;
 	dl_se->dl_new = 0;
+	trace_sched_deadline_replenish(dl_se);
 }
 
 /*
@@ -407,6 +409,8 @@ static void replenish_dl_entity(struct sched_dl_entity *dl_se,
 		dl_se->runtime = pi_se->dl_runtime;
 	}
 
+	trace_sched_deadline_replenish(dl_se);
+
 	if (dl_se->dl_yielded)
 		dl_se->dl_yielded = 0;
 	if (dl_se->dl_throttled)
@@ -495,6 +499,7 @@ static void update_dl_entity(struct sched_dl_entity *dl_se,
 	    dl_entity_overflow(dl_se, pi_se, rq_clock(rq))) {
 		dl_se->deadline = rq_clock(rq) + pi_se->dl_deadline;
 		dl_se->runtime = pi_se->dl_runtime;
+		trace_sched_deadline_replenish(dl_se);
 	}
 }
 
@@ -734,6 +739,7 @@ static void update_curr_dl(struct rq *rq)
 	dl_se->runtime -= dl_se->dl_yielded ? 0 : delta_exec;
 	if (dl_runtime_exceeded(dl_se)) {
 		dl_se->dl_throttled = 1;
+		trace_sched_deadline_throttle(dl_se);
 		__dequeue_task_dl(rq, curr, 0);
 		if (unlikely(dl_se->dl_boosted || !start_dl_timer(curr)))
 			enqueue_task_dl(rq, curr, ENQUEUE_REPLENISH);
@@ -910,6 +916,7 @@ enqueue_dl_entity(struct sched_dl_entity *dl_se,
 static void dequeue_dl_entity(struct sched_dl_entity *dl_se)
 {
 	__dequeue_dl_entity(dl_se);
+	trace_sched_deadline_block(dl_se);
 }
 
 static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
@@ -978,6 +985,7 @@ static void yield_task_dl(struct rq *rq)
 {
 	struct task_struct *p = rq->curr;
 
+	update_rq_clock(rq);
 	/*
 	 * We make the task go to sleep until its current deadline by
 	 * forcing its runtime to zero. This way, update_curr_dl() stops
@@ -986,9 +994,15 @@ static void yield_task_dl(struct rq *rq)
 	 */
 	if (p->dl.runtime > 0) {
 		rq->curr->dl.dl_yielded = 1;
+		if (trace_sched_deadline_yield_enabled()) {
+			u64 delta_exec = rq_clock_task(rq) - p->se.exec_start;
+			/* Subtract the last run till now */
+			if (likely((s64)delta_exec > 0))
+				p->dl.runtime -= delta_exec;
+			trace_sched_deadline_yield(&p->dl);
+		}
 		p->dl.runtime = 0;
 	}
-	update_rq_clock(rq);
 	update_curr_dl(rq);
 	/*
 	 * Tell update_rq_clock() that we've just updated,
-- 
2.5.0

[toc] | [next] | [standalone]

#1339752

On Mon, Feb 22, 2016 at 02:08:21PM -0300, Daniel Bristot de Oliveira wrote:
> Deadline tasks behave differently of other tasks because deadline
> task's also depend on their period, deadline and runtime.
> 
> Hence, the well known sched:sched_wakeup and sched:sched_switch
> tracepoints are not always enough to precisely explain the behavior of a
> deadline task with respect to the task's period, deadline, and runtime
> consumption and replenishment.
> 
> In order to provide more precise information about the scheduling of
> deadline tasks, this patch implements the following tracepoints:
> 
> - sched:sched_deadline_replenish: Informs the runtime replenishment of
> a deadline task. Trace example:
> 
>   <idle>-0     [010] d.h.   268.428878: sched_deadline_replenish: \
> 	comm=y pid=1608 now=268.428876113 deadline=268.458863627  \
> 	runtime=20000000
> 
> The task y-1608 was replenished with 20000000 ns at 268.428876113
> to be used until the deadline at 268.458863627.
> 
> - sched:sched_deadline_yield: Informs that a deadline task called
> sched_yield(), and will wait for the next period. Trace example:
> 
>   y-1608  [010] d...   268.428892: sched_deadline_yield: 	\
> 	now=268.428891932 deadline=268.458863627		\
> 	remaining_runtime=19995653
> 
> The task y-1608 yielded before its deadline, with 19995653 ns of
> remaining runtime.
> 
> - sched:sched_deadline_throttle: Informs that a task consumed all its
> available runtime and was throttled. Trace example:
> 
>   t-1613  [011] d.h.   280.419823: sched_deadline_throttle:	\
> 	now=280.419823282 deadline=280.430683206 		\
> 	remaining_runtime=-13280
> 
> The task t-1613 overrun its runtime by 13280 ns and was throttled.
> 
> - sched:sched_deadline_block: Informs that a deadline task went to sleep
> waiting to be awakened by another task. Trace example:
> 
>   b-1611  [011] d...   276.228298: sched_deadline_block: 	\
> 	now=276.228295889 deadline=276.258262555
> 	remaining_runtime=19966666
> 
> The task b-1611 blocked waiting for an external event. Its deadline is at
> 276.258262555, and it stills have 19966666 ns of remaining runtime on the
> current period.

So I'm a bit allergic to tracepoints and this is very flimsy on reasons
why I would want to do this.

As it stands, the existing tracepoint have already been an ABI
trainwreck, why would I want to add more?

[toc] | [prev] | [next] | [standalone]

#1339754

On Mon, 22 Feb 2016 18:32:59 +0100
Peter Zijlstra <peterz@infradead.org> wrote:


> So I'm a bit allergic to tracepoints and this is very flimsy on reasons
> why I would want to do this.

Because there's no way to know if SCHED_DEADLINE tasks are doing what
they suppose to without hacking the kernel and adding your own
tracepoints.

> 
> As it stands, the existing tracepoint have already been an ABI
> trainwreck, why would I want to add more?

Yes, this may become a type of ABI, but even the sched switch
tracepoints haven't been that bad. Has it really prevented us from
changing anything?

"trainwreck" is a harsh word, and the information of scheduling
tracepoints have been crucial to finding bugs and such. And has been
tremendously useful in loads of cases.

But let me ask, what would you recommend to finding out if the kernel
has really given your tasks the recommended runtime within a given
period? We can't expect users of SCHED_DEADLINE to be modifying the
kernel themselves.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1339891

On 02/22/2016 02:48 PM, Steven Rostedt wrote:
> On Mon, 22 Feb 2016 18:32:59 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> 
>> > So I'm a bit allergic to tracepoints and this is very flimsy on reasons
>> > why I would want to do this.
> Because there's no way to know if SCHED_DEADLINE tasks are doing what
> they suppose to without hacking the kernel and adding your own
> tracepoints.

That is the point. A potential deadline user would have to become a
sched deadline developer to be able to debug its deadline tasks.

Given that many deadline scheduler users are expected from application
field, e.g. automation, and that these users generally do not even know
how to compile a kernel, having a "ready to use" way to observe sched
deadline specific points of interest is essential for deadline
scheduler adoption.

For example, the runtime is an estimated value because there is no
deterministic way define it, so be able to observe the remaining runtime
at the end of an activation is very useful to define a tighter runtime.
On the other hand, it is possible to defined that runtime was under
estimated just by observing that a task was throttled.

Another example: be able to see that a task is blocking in the middle of
an activation is useful on the identification of an unexpected behavior
of a deadline task.

Although it is possible to guess many things by doing user-space
measurements, by using a set of other tracepoints, or even by using both
together. The debug process will not be as easy and precise as using
these four tracepoints.

-- Daniel

[toc] | [prev] | [next] | [standalone]

#1339947

On Mon, Feb 22, 2016 at 12:48:54PM -0500, Steven Rostedt wrote:

> > As it stands, the existing tracepoint have already been an ABI
> > trainwreck, why would I want to add more?
> 
> Yes, this may become a type of ABI, but even the sched switch
> tracepoints haven't been that bad. Has it really prevented us from
> changing anything?

The whole wakeup thing where we _still_ have a dummy argument, and have
been lying about the value for a long time really stinks.

> But let me ask, what would you recommend to finding out if the kernel
> has really given your tasks the recommended runtime within a given
> period? We can't expect users of SCHED_DEADLINE to be modifying the
> kernel themselves.

So why are these deadline specific tracepoint? Why not extend the ones
we already have?

Also, will these tracepoints still work if we implement SCHED_DEADLINE
using Least-Laxity-First or Pfair or some other exotic algorithm? Or
will be forever be bound to EDF just because tracepoint ABI shite?

Worse, the proposed tracepoints are atrocious, look at crap like this:

> +		if (trace_sched_deadline_yield_enabled()) {
> +			u64 delta_exec = rq_clock_task(rq) - p->se.exec_start;
> +			/* Subtract the last run till now */
> +			if (likely((s64)delta_exec > 0))
> +				p->dl.runtime -= delta_exec;
> +			trace_sched_deadline_yield(&p->dl);
> +		}

tracepoints should _NEVER_ change state, ever.

And there's the whole COND tracepoint muck, which also doesn't win any
prices.

So tell me why these specific tracepoints and why the existing ones
could not be extended to include this information. For example, why a
trace_sched_dealine_yield, and not a generic trace_sched_yield() that
works for all classes.

Tell me that the information presented does not pin the implementation.

And clean up the crap.

Then I might maybe consider this.

But do not present me with a bunch of random arse, hacked together
tracepoints and tell me they might be useful, maybe.

[toc] | [prev] | [next] | [standalone]

#1340027

On Mon, 22 Feb 2016 22:30:17 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Feb 22, 2016 at 12:48:54PM -0500, Steven Rostedt wrote:
> 
> > > As it stands, the existing tracepoint have already been an ABI
> > > trainwreck, why would I want to add more?  
> > 
> > Yes, this may become a type of ABI, but even the sched switch
> > tracepoints haven't been that bad. Has it really prevented us from
> > changing anything?  
> 
> The whole wakeup thing where we _still_ have a dummy argument, and have
> been lying about the value for a long time really stinks.

Having a dummy argument is not that bad. Yes, it's inconvenient, and
I'm not sure who even uses it (can we delete it without breaking
anything?) But it doesn't prevent us from going forward with
development.

> 
> > But let me ask, what would you recommend to finding out if the kernel
> > has really given your tasks the recommended runtime within a given
> > period? We can't expect users of SCHED_DEADLINE to be modifying the
> > kernel themselves.  
> 
> So why are these deadline specific tracepoint? Why not extend the ones
> we already have?

I'm not sure how to do that and be able to report when a period has
elapsed, and when the next period is coming.

> 
> Also, will these tracepoints still work if we implement SCHED_DEADLINE
> using Least-Laxity-First or Pfair or some other exotic algorithm? Or
> will be forever be bound to EDF just because tracepoint ABI shite?

Can we come up with generic numbers? I mean, the user that asks for
their task to have a specific runtime within a specific
period/deadline, these seem to be generic already. I'll have to read up
on those that you mention, but do that not have a "replenish" for when
the period starts again? And then a yield, showing the task has given
up its remaining time, or a block, where a task is scheduled out
because it blocked on a lock?

> 
> Worse, the proposed tracepoints are atrocious, look at crap like this:
> 
> > +		if (trace_sched_deadline_yield_enabled()) {
> > +			u64 delta_exec = rq_clock_task(rq) - p->se.exec_start;
> > +			/* Subtract the last run till now */
> > +			if (likely((s64)delta_exec > 0))
> > +				p->dl.runtime -= delta_exec;
> > +			trace_sched_deadline_yield(&p->dl);
> > +		}  
> 
> tracepoints should _NEVER_ change state, ever.

Heh, it's not really changing state. The code directly after this is:

	p->dl.runtime = 0;

Without updating dl.runtime, you the tracepoint would report inaccurate
remaining time. Without that, you would get reports of yielding with
full runtimes, making it look like you never ran at all.


> 
> And there's the whole COND tracepoint muck, which also doesn't win any
> prices.

It keeps us from adding flags that may end up going away and us
maintaining a dummy field forever ;-)

> 
> So tell me why these specific tracepoints and why the existing ones
> could not be extended to include this information. For example, why a
> trace_sched_dealine_yield, and not a generic trace_sched_yield() that
> works for all classes.

But what about reporting actual runtime, and when the next period will
come. That only matters for deadline.

> 
> Tell me that the information presented does not pin the implementation.
> 
> And clean up the crap.
> 
> Then I might maybe consider this.
> 
> But do not present me with a bunch of random arse, hacked together
> tracepoints and tell me they might be useful, maybe.


They ARE useful. These are the tracepoints I'm currently using to
debug the deadline scheduler with. They have been indispensable for my
current work.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1340514

Hi,

On 22/02/16 17:30, Steven Rostedt wrote:
> On Mon, 22 Feb 2016 22:30:17 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Mon, Feb 22, 2016 at 12:48:54PM -0500, Steven Rostedt wrote:
> > 

[...]

> > 
> > > But let me ask, what would you recommend to finding out if the kernel
> > > has really given your tasks the recommended runtime within a given
> > > period? We can't expect users of SCHED_DEADLINE to be modifying the
> > > kernel themselves.  
> > 
> > So why are these deadline specific tracepoint? Why not extend the ones
> > we already have?
> 
> I'm not sure how to do that and be able to report when a period has
> elapsed, and when the next period is coming.
> 
> > 
> > Also, will these tracepoints still work if we implement SCHED_DEADLINE
> > using Least-Laxity-First or Pfair or some other exotic algorithm? Or
> > will be forever be bound to EDF just because tracepoint ABI shite?
> 
> Can we come up with generic numbers? I mean, the user that asks for
> their task to have a specific runtime within a specific
> period/deadline, these seem to be generic already. I'll have to read up
> on those that you mention, but do that not have a "replenish" for when
> the period starts again? And then a yield, showing the task has given
> up its remaining time, or a block, where a task is scheduled out
> because it blocked on a lock?
> 

AFAICT throttle, yield and block seem fairly generic. How we make the
definition generic w.r.t. the arguments we want to print is a different
matter, though. :-/

I should refresh my memory about the above mentioned algorithms, and
"replenish" might be particular to the current implementation. However,
shouldn't any algo that has a "throttle" event also have a corresponding
"un-throttle (replenish)" event? I guess being able to throttle
misbehaving tasks is a property that we will always desire.

> > 
> > Worse, the proposed tracepoints are atrocious, look at crap like this:
> > 
> > > +		if (trace_sched_deadline_yield_enabled()) {
> > > +			u64 delta_exec = rq_clock_task(rq) - p->se.exec_start;
> > > +			/* Subtract the last run till now */
> > > +			if (likely((s64)delta_exec > 0))
> > > +				p->dl.runtime -= delta_exec;
> > > +			trace_sched_deadline_yield(&p->dl);
> > > +		}  
> > 
> > tracepoints should _NEVER_ change state, ever.
> 
> Heh, it's not really changing state. The code directly after this is:
> 
> 	p->dl.runtime = 0;
> 
> Without updating dl.runtime, you the tracepoint would report inaccurate
> remaining time. Without that, you would get reports of yielding with
> full runtimes, making it look like you never ran at all.
> 

Right, I guess that might be useful to understand if you over-
dimensioned the reservation. Can't we make this a macro or do the
computation local to the tracepoint itself so that code looks nicer?

[...]

> > 
> > So tell me why these specific tracepoints and why the existing ones
> > could not be extended to include this information. For example, why a
> > trace_sched_dealine_yield, and not a generic trace_sched_yield() that
> > works for all classes.
> 
> But what about reporting actual runtime, and when the next period will
> come. That only matters for deadline.
> 

As said above, the event looks generic enough to me. Not sure how to
make printed arguments generic, though.

> > 
> > Tell me that the information presented does not pin the implementation.
> > 
> > And clean up the crap.
> > 
> > Then I might maybe consider this.
> > 
> > But do not present me with a bunch of random arse, hacked together
> > tracepoints and tell me they might be useful, maybe.
> 
> 
> They ARE useful. These are the tracepoints I'm currently using to
> debug the deadline scheduler with. They have been indispensable for my
> current work.
> 

I also think they are very useful. I always had some sort of tracepoints
that I continue forward porting during the development of
SCHED_DEADLINE, and I couldn't really understand what was going on
without those. There is value if we can agree on the mainline
incarnation of such tracepoints now, IMHO.

Best,

- Juri

[toc] | [prev] | [next] | [standalone]

#1340523

On Mon, Feb 22, 2016 at 05:30:43PM -0500, Steven Rostedt wrote:
> On Mon, 22 Feb 2016 22:30:17 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Mon, Feb 22, 2016 at 12:48:54PM -0500, Steven Rostedt wrote:
> > 
> > > > As it stands, the existing tracepoint have already been an ABI
> > > > trainwreck, why would I want to add more?  
> > > 
> > > Yes, this may become a type of ABI, but even the sched switch
> > > tracepoints haven't been that bad. Has it really prevented us from
> > > changing anything?  
> > 
> > The whole wakeup thing where we _still_ have a dummy argument, and have
> > been lying about the value for a long time really stinks.
> 
> Having a dummy argument is not that bad. Yes, it's inconvenient, and
> I'm not sure who even uses it (can we delete it without breaking
> anything?) But it doesn't prevent us from going forward with
> development.

No it very much illustrates the problem and is a very clear indication
that tracepoints are an ABI.

Yes, we more or less got away with it this time, but it does prohibit us
from doing sane things, see below.

> > > But let me ask, what would you recommend to finding out if the kernel
> > > has really given your tasks the recommended runtime within a given
> > > period? We can't expect users of SCHED_DEADLINE to be modifying the
> > > kernel themselves.  
> > 
> > So why are these deadline specific tracepoint? Why not extend the ones
> > we already have?
> 
> I'm not sure how to do that and be able to report when a period has
> elapsed, and when the next period is coming.

Seriously? Sit still for a minute and think perhaps?

> > Also, will these tracepoints still work if we implement SCHED_DEADLINE
> > using Least-Laxity-First or Pfair or some other exotic algorithm? Or
> > will be forever be bound to EDF just because tracepoint ABI shite?
> 
> Can we come up with generic numbers? I mean, the user that asks for
> their task to have a specific runtime within a specific
> period/deadline, these seem to be generic already. I'll have to read up
> on those that you mention, but do that not have a "replenish" for when
> the period starts again? And then a yield, showing the task has given
> up its remaining time, or a block, where a task is scheduled out
> because it blocked on a lock?

You tell me; why do you expect me to think for you? These should all
have been questions you asked yourself before you even considered
posting tracepoints.

ABI remember? We're careful with that.

> > Worse, the proposed tracepoints are atrocious, look at crap like this:
> > 
> > > +		if (trace_sched_deadline_yield_enabled()) {
> > > +			u64 delta_exec = rq_clock_task(rq) - p->se.exec_start;
> > > +			/* Subtract the last run till now */
> > > +			if (likely((s64)delta_exec > 0))
> > > +				p->dl.runtime -= delta_exec;
> > > +			trace_sched_deadline_yield(&p->dl);
> > > +		}  
> > 
> > tracepoints should _NEVER_ change state, ever.
> 
> Heh, it's not really changing state. The code directly after this is:
> 
> 	p->dl.runtime = 0;

Yes, it more or less 'works', but its still atrocious shite. Its the
worst kind of anti pattern possible.

Suppose someone comes and removes that line, and ignores the tracepoint
stuff, because, hell its a tracepoint, those don't modify stuff.

Its just really, utterly bad practice.

You've done this tracing code long enough, you really should _KNOW_
this.

> > So tell me why these specific tracepoints and why the existing ones
> > could not be extended to include this information. For example, why a
> > trace_sched_dealine_yield, and not a generic trace_sched_yield() that
> > works for all classes.
> 
> But what about reporting actual runtime, and when the next period will
> come. That only matters for deadline.

How is that an answer to the question? Are you implying a generic
trace_sched_yield() call could not do this?

> > But do not present me with a bunch of random arse, hacked together
> > tracepoints and tell me they might be useful, maybe.
> 
> 
> They ARE useful. These are the tracepoints I'm currently using to
> debug the deadline scheduler with. They have been indispensable for my
> current work.

They are, most obviously, a hacked together debug session for sure. This
is _NOT_ what you commit.

Now ideally we'd do something like the below, but because trainwreck, we
cannot actually do this I think :-(

It gets you about half of what your patch does, but shows how to also
do a generic sched_yield(). The replenish might have to remain special,
although both CFS and RT also have replenishes, albeit significantly
different.

I really dislike tracepoints, and I'm >.< close to proposing a patch
removing them all from the scheduler.

---
 include/trace/events/sched.h | 97 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 86 insertions(+), 11 deletions(-)

diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index 9b90c57517a9..b902eb71830b 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -103,9 +103,15 @@ DEFINE_EVENT(sched_wakeup_template, sched_wakeup_new,
 	     TP_PROTO(struct task_struct *p),
 	     TP_ARGS(p));
 
+#define TASK_STATE_PREEMPT	(TASK_STATE_MAX << 0)
+#define TASK_STATE_THROTTLED	(TASK_STATE_MAX << 1)
+#define TASK_STATE_YIELDED	(TASK_STATE_MAX << 2)
+
 #ifdef CREATE_TRACE_POINTS
 static inline long __trace_sched_switch_state(bool preempt, struct task_struct *p)
 {
+	long state = p->state;
+
 #ifdef CONFIG_SCHED_DEBUG
 	BUG_ON(p != current);
 #endif /* CONFIG_SCHED_DEBUG */
@@ -114,10 +120,49 @@ static inline long __trace_sched_switch_state(bool preempt, struct task_struct *
 	 * Preemption ignores task state, therefore preempted tasks are always
 	 * RUNNING (we will not have dequeued if state != RUNNING).
 	 */
-	return preempt ? TASK_RUNNING | TASK_STATE_MAX : p->state;
+	if (preempt) {
+		state = TASK_RUNNING | TASK_STATE_MAX;
+	} else if (dl_task(p)) {
+		if (p->dl.dl_throttled)
+			state |= TASK_STATE_THROTTLED;
+		else if (p->dl.dl_yielded)
+			state |= TASK_STATE_YIELDED;
+	}
+
+	return state;
 }
 #endif /* CREATE_TRACE_POINTS */
 
+#define __trace_sched_switch_fields(name)	do {			\
+	__entry->name##_policy = name->policy;				\
+	switch (name->policy) {						\
+	case SCHED_IDLE:						\
+	case SCHED_BATCH:						\
+	case SCHED_NORMAL:						\
+		__entry->name##_f1 = PRIO_TO_NICE(name->static_prio);	\
+		__entry->name##_f2 = 0;					\
+		__entry->name##_f3 = 0;					\
+		break;							\
+	case SCHED_RR:							\
+	case SCHED_FIFO:						\
+		__entry->name##_f1 = USER_PRIO(name->normal_prio);	\
+		__entry->name##_f2 = 0;					\
+		__entry->name##_f3 = 0;					\
+		break;							\
+	case SCHED_DEADLINE:						\
+		__entry->name##_f1 = name->dl.runtime;			\
+		__entry->name##_f2 = name->dl.deadline;			\
+		__entry->name##_f3 = name->dl.dl_period;		\
+		break;							\
+	default:							\
+		__entry->name##_f1 = 0;					\
+		__entry->name##_f2 = 0;					\
+		__entry->name##_f3 = 0;					\
+		break;							\
+	}								\
+} while (0)
+
+
 /*
  * Tracepoint for task switches, performed by the scheduler:
  */
@@ -132,33 +177,63 @@ TRACE_EVENT(sched_switch,
 	TP_STRUCT__entry(
 		__array(	char,	prev_comm,	TASK_COMM_LEN	)
 		__field(	pid_t,	prev_pid			)
-		__field(	int,	prev_prio			)
+		__field(	int,	prev_policy			)
+		__field(	s64,	prev_f1				)
+		__field(	u64,	prev_f2				)
+		__field(	u64,	prev_f3				)
 		__field(	long,	prev_state			)
+
 		__array(	char,	next_comm,	TASK_COMM_LEN	)
 		__field(	pid_t,	next_pid			)
-		__field(	int,	next_prio			)
+		__field(	int,	next_policy			)
+		__field(	s64,	next_f1				)
+		__field(	u64,	next_f2				)
+		__field(	u64,	next_f3				)
 	),
 
 	TP_fast_assign(
-		memcpy(__entry->next_comm, next->comm, TASK_COMM_LEN);
+		memcpy(__entry->prev_comm, prev->comm, TASK_COMM_LEN);
 		__entry->prev_pid	= prev->pid;
-		__entry->prev_prio	= prev->prio;
+		__trace_sched_switch_fields(prev);
 		__entry->prev_state	= __trace_sched_switch_state(preempt, prev);
-		memcpy(__entry->prev_comm, prev->comm, TASK_COMM_LEN);
+
+		memcpy(__entry->next_comm, next->comm, TASK_COMM_LEN);
 		__entry->next_pid	= next->pid;
-		__entry->next_prio	= next->prio;
+		__trace_sched_switch_fields(next);
 	),
 
-	TP_printk("prev_comm=%s prev_pid=%d prev_prio=%d prev_state=%s%s ==> next_comm=%s next_pid=%d next_prio=%d",
-		__entry->prev_comm, __entry->prev_pid, __entry->prev_prio,
+	TP_printk("prev_comm=%s prev_pid=%d prev=%s:{%Ld,%Lu,%Lu} prev_state=%s%s ==> next_comm=%s next_pid=%d next=%s:{%Ld,%Lu,%Lu}",
+		__entry->prev_comm, __entry->prev_pid,
+		__print_symbolic(__entry->prev_policy,
+				{SCHED_IDLE,	"IDLE"},
+				{SCHED_BATCH,	"BATCH"},
+				{SCHED_NORMAL,	"NORMAL"},
+				{SCHED_RR,	"RR"},
+				{SCHED_FIFO,	"FIFO"},
+				{SCHED_DEADLINE,"DEADLINE"}),
+		__entry->prev_f1, __entry->prev_f2, __entry->prev_f3,
+
 		__entry->prev_state & (TASK_STATE_MAX-1) ?
 		  __print_flags(__entry->prev_state & (TASK_STATE_MAX-1), "|",
 				{ 1, "S"} , { 2, "D" }, { 4, "T" }, { 8, "t" },
 				{ 16, "Z" }, { 32, "X" }, { 64, "x" },
 				{ 128, "K" }, { 256, "W" }, { 512, "P" },
 				{ 1024, "N" }) : "R",
-		__entry->prev_state & TASK_STATE_MAX ? "+" : "",
-		__entry->next_comm, __entry->next_pid, __entry->next_prio)
+		__print_flags(__entry->prev_state & ~(TASK_STATE_MAX-1), "|",
+				{ TASK_STATE_PREEMPT,   "+" },
+				{ TASK_STATE_THROTTLED, "t" },
+				{ TASK_STATE_YIELDED,	"y" }),
+
+		__entry->next_comm, __entry->next_pid,
+		__print_symbolic(__entry->next_policy,
+				{SCHED_IDLE,	"IDLE"},
+				{SCHED_BATCH,	"BATCH"},
+				{SCHED_NORMAL,	"NORMAL"},
+				{SCHED_RR,	"RR"},
+				{SCHED_FIFO,	"FIFO"},
+				{SCHED_DEADLINE,"DEADLINE"}),
+		__entry->next_f1, __entry->next_f2, __entry->next_f3,
+		)
 );
 
 /*

[toc] | [prev] | [next] | [standalone]

#1340598

On Tue, 23 Feb 2016 11:44:08 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> No it very much illustrates the problem and is a very clear indication
> that tracepoints are an ABI.

Yes they are. But note, they can change if nobody notices ;-)


> > 
> > Heh, it's not really changing state. The code directly after this is:
> > 
> > 	p->dl.runtime = 0;  
> 
> Yes, it more or less 'works', but its still atrocious shite. Its the
> worst kind of anti pattern possible.
> 
> Suppose someone comes and removes that line, and ignores the tracepoint
> stuff, because, hell its a tracepoint, those don't modify stuff.
> 
> Its just really, utterly bad practice.
> 
> You've done this tracing code long enough, you really should _KNOW_
> this.

You're right. I got too caught up in the cleverness of the hack to
acknowledge it is a hack.

But Daniel has a patch to clean up the yield code which would also help
in making this hack unnecessary for this tracepoint.

> 
> > > So tell me why these specific tracepoints and why the existing ones
> > > could not be extended to include this information. For example, why a
> > > trace_sched_dealine_yield, and not a generic trace_sched_yield() that
> > > works for all classes.  
> > 
> > But what about reporting actual runtime, and when the next period will
> > come. That only matters for deadline.  
> 
> How is that an answer to the question? Are you implying a generic
> trace_sched_yield() call could not do this?
> 
> > > But do not present me with a bunch of random arse, hacked together
> > > tracepoints and tell me they might be useful, maybe.  
> > 
> > 
> > They ARE useful. These are the tracepoints I'm currently using to
> > debug the deadline scheduler with. They have been indispensable for my
> > current work.  
> 
> They are, most obviously, a hacked together debug session for sure. This
> is _NOT_ what you commit.
> 
> Now ideally we'd do something like the below, but because trainwreck, we
> cannot actually do this I think :-(
> 
> It gets you about half of what your patch does, but shows how to also
> do a generic sched_yield(). The replenish might have to remain special,
> although both CFS and RT also have replenishes, albeit significantly
> different.

OK, I admit. I was very single focused on deadline scheduler. I wasn't
looking at how this could work with the rest of the scheduler. I'll
take a look at the patches you posted.

Thanks!

-- Steve

[toc] | [prev] | [next] | [standalone]

#1341661

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 23 Feb 2016 11:44:08 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > No it very much illustrates the problem and is a very clear indication
> > that tracepoints are an ABI.
> 
> Yes they are. But note, they can change if nobody notices ;-)

Which is rather seldom for something as high profile as core scheduler
tracepoints ...

You can be cavalier about tracepoints in subsystems where the tracepoints are at 
most used by the maintainers with specialized, closely maintained tooling that is 
easy to change. Not so much about scheduler tracepoints which are being widely 
utilized by various projects.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1340687

On Tue, Feb 23, 2016 at 11:44:08AM +0100, Peter Zijlstra wrote:
>  include/trace/events/sched.h | 97 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 86 insertions(+), 11 deletions(-)
> 
> diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
> index 9b90c57517a9..b902eb71830b 100644
> --- a/include/trace/events/sched.h
> +++ b/include/trace/events/sched.h
> @@ -103,9 +103,15 @@ DEFINE_EVENT(sched_wakeup_template, sched_wakeup_new,
>  	     TP_PROTO(struct task_struct *p),
>  	     TP_ARGS(p));
>  
> +#define TASK_STATE_PREEMPT	(TASK_STATE_MAX << 0)
> +#define TASK_STATE_THROTTLED	(TASK_STATE_MAX << 1)
> +#define TASK_STATE_YIELDED	(TASK_STATE_MAX << 2)
> +
>  #ifdef CREATE_TRACE_POINTS
>  static inline long __trace_sched_switch_state(bool preempt, struct task_struct *p)
>  {
> +	long state = p->state;
> +
>  #ifdef CONFIG_SCHED_DEBUG
>  	BUG_ON(p != current);
>  #endif /* CONFIG_SCHED_DEBUG */
> @@ -114,10 +120,49 @@ static inline long __trace_sched_switch_state(bool preempt, struct task_struct *
>  	 * Preemption ignores task state, therefore preempted tasks are always
>  	 * RUNNING (we will not have dequeued if state != RUNNING).
>  	 */
> -	return preempt ? TASK_RUNNING | TASK_STATE_MAX : p->state;
> +	if (preempt) {
> +		state = TASK_RUNNING | TASK_STATE_MAX;
> +	} else if (dl_task(p)) {
> +		if (p->dl.dl_throttled)
> +			state |= TASK_STATE_THROTTLED;
> +		else if (p->dl.dl_yielded)
> +			state |= TASK_STATE_YIELDED;

These might want to be inverted, that is, yielded will have throttled
set, so yield should take precedence.

> +	}
> +
> +	return state;
>  }
>  #endif /* CREATE_TRACE_POINTS */
>  
> +#define __trace_sched_switch_fields(name)	do {			\
> +	__entry->name##_policy = name->policy;				\
> +	switch (name->policy) {						\

Sadly we cannot use policy, for that isn't updated on PI. The best we
can do is dl_task(), rt_task() else ...

> +	case SCHED_IDLE:						\

This doesn't have nice, so should go with default;

> +	case SCHED_BATCH:						\
> +	case SCHED_NORMAL:						\
> +		__entry->name##_f1 = PRIO_TO_NICE(name->static_prio);	\
> +		__entry->name##_f2 = 0;					\
> +		__entry->name##_f3 = 0;					\
> +		break;							\
> +	case SCHED_RR:							\
> +	case SCHED_FIFO:						\
> +		__entry->name##_f1 = USER_PRIO(name->normal_prio);	\
> +		__entry->name##_f2 = 0;					\
> +		__entry->name##_f3 = 0;					\
> +		break;							\
> +	case SCHED_DEADLINE:						\
> +		__entry->name##_f1 = name->dl.runtime;			\
> +		__entry->name##_f2 = name->dl.deadline;			\
> +		__entry->name##_f3 = name->dl.dl_period;		\
> +		break;							\
> +	default:							\
> +		__entry->name##_f1 = 0;					\
> +		__entry->name##_f2 = 0;					\
> +		__entry->name##_f3 = 0;					\
> +		break;							\
> +	}								\
> +} while (0)

[toc] | [prev] | [next] | [standalone]

#1340831

On 02/23/2016 07:44 AM, Peter Zijlstra wrote:
>>> Worse, the proposed tracepoints are atrocious, look at crap like this:
>>> > > 
>>>> > > > +		if (trace_sched_deadline_yield_enabled()) {
>>>> > > > +			u64 delta_exec = rq_clock_task(rq) - p->se.exec_start;
>>>> > > > +			/* Subtract the last run till now */
>>>> > > > +			if (likely((s64)delta_exec > 0))
>>>> > > > +				p->dl.runtime -= delta_exec;
>>>> > > > +			trace_sched_deadline_yield(&p->dl);
>>>> > > > +		}  
>>> > > 
>>> > > tracepoints should _NEVER_ change state, ever.
>> > 
>> > Heh, it's not really changing state. The code directly after this is:
>> > 
>> > 	p->dl.runtime = 0;
> Yes, it more or less 'works', but its still atrocious shite. Its the
> worst kind of anti pattern possible.
> 
> Suppose someone comes and removes that line, and ignores the tracepoint
> stuff, because, hell its a tracepoint, those don't modify stuff.
> 
> Its just really, utterly bad practice.

It is possible to clean up this by removing the "p->dl.runtime = 0;"
from yield_task_dl(), to carry the dl_runtime until update_curr_dl().
We end up not proposing this change because we thought it could be
too much intrusive. But seems that it was the correct approach.

Btw, your patch for "Always calculate end of period on sched_yield()"
already do the necessary clean up, so in the possible next version of
these tracepoint the hack will disappear. 

>>> > > So tell me why these specific tracepoints and why the existing ones
>>> > > could not be extended to include this information. For example, why a
>>> > > trace_sched_dealine_yield, and not a generic trace_sched_yield() that
>>> > > works for all classes.
>> > 
>> > But what about reporting actual runtime, and when the next period will
>> > come. That only matters for deadline.
> How is that an answer to the question? Are you implying a generic
> trace_sched_yield() call could not do this?

I agree that a trace_sched_yield() could partially do this. But each
scheduler would have its own specific data to be printed, and it is hard
to define how many and the type of these data.

>>> > > But do not present me with a bunch of random arse, hacked together
>>> > > tracepoints and tell me they might be useful, maybe.
>> > 
>> > 
>> > They ARE useful. These are the tracepoints I'm currently using to
>> > debug the deadline scheduler with. They have been indispensable for my
>> > current work.
> They are, most obviously, a hacked together debug session for sure. This
> is _NOT_ what you commit.
> 
> Now ideally we'd do something like the below, but because trainwreck, we
> cannot actually do this I think :-(

I do liked this patch, but I agree that bad things can happen on
applications that already use sched:sched_switch :-(.
 
> I really dislike tracepoints, and I'm >.< close to proposing a patch
> removing them all from the scheduler.

Scheduler tracepoints become such a standard not only for debugging the
kernel, but also for understanding performance issues and finding
optimizations for user-space tasks, e.g. finding the best way to spread
task among processors on a large NUMA box. Many people would cry
if sched tracepoints disappears :'-(.

> @@ -132,33 +177,63 @@ TRACE_EVENT(sched_switch,
>  	TP_STRUCT__entry(
>  		__array(	char,	prev_comm,	TASK_COMM_LEN	)
>  		__field(	pid_t,	prev_pid			)
> -		__field(	int,	prev_prio			)
> +		__field(	int,	prev_policy			)
> +		__field(	s64,	prev_f1				)
> +		__field(	u64,	prev_f2				)
> +		__field(	u64,	prev_f3				)

This helps me to explain why did I chose to create deadline specific
tracepoints.

It is not possible to define in advance a common set of parameters to be
traced/printed for all future (exotic) scheduler, because each scheduler
will use its own set of parameters. Hence, the number and type of the
parameter can vary. How many fields do we need? and which are the types
of these fields? What if a scheduler needs two s64 fields and no u64?

Moreover, it is not possible to define the changes that will be needed
on classical schedulers algorithms for them to fit on Linux's
abstractions and restrictions. Therefore, it is not safe to mention
that LLF would (theoretically) use the same set of parameters of a EDF
scheduler, because the theoretical LLF would need to be modified
to fit on Linux. For example, there is no throttling concept on the
classical deadline scheduler. So, the proposed tracepoints are only valid
for Linux's sched deadline scheduler (deadline scheduler + CBS).

That is why I believe that it would be a better approach to have
scheduler specific tracepoints for the sched_*_entity particularities
and sched_* specific points interest.

Finally, I was based in the same approach used on the fair scheduler's
sched:sched_stat* tracepoints: they report sched_entity data on fair.c,
and my patches report sched_deadline_entity data on deadline.c.

[toc] | [prev] | [next] | [standalone]

#1341255

On 02/23/2016 07:44 AM, Peter Zijlstra wrote:
> Now ideally we'd do something like the below, but because trainwreck, we
> cannot actually do this I think :-(

Some other considerations:

1) The majority of tasks run on NORMAL scheduler with default nice. So,
prev=NORMAL:{0,0,0} and next=NORMAL:{0,0,0} does not help too much.
The tracepoint size increase from 64 to 112, but this difference will not
be useful for the majority of the user/use-cases.

2) The sched:switch can be wider than this 212 characters output:
"              m-1604  [004] d...  1940.393070: sched_switch: prev_comm=m prev_pid=1604 prev=DEADLINE:{-967616,1940402119572,30000000} prev_state=Rt ==> next_comm=kworker/4:2H next_pid=1507 next=NORMAL:{-20,0,0}"

3) On the other tracepoints, rostedt suggested me to also include the
now= field because the clock printed in the trace prefix is
not the same clock used by the deadline scheduler.

4) Although neither perf record/script nor trace-cmd record/report broke,
both printed only the old fields. So, both would need to be updated and
carry both the old and the new version.

Well, I bet there will be many other applications that were not well
written as perf and trace-cmd :-(

5) A new scheduler will probably require modifications on this
tracepoint, hence on all other tools that use this tracepoint too.

6) I liked all the code to compose the output string! But it is not
fair to say that the "format" is intuitive. One may need to put the
finger in the monitor and use a calculator to see that the filter
for a yielded tasks is "prev_state == 8192".

Btw, it only works for deadline task yield, and the yield call of all
other classes will need to be changed to support this. The question
is, is it worth for other classes?

7) This patch is currently showing the runtime zeroed on yield.
So, it needs the clean up on yield_task_dl() to properly
inform the unused runtime of the task's activation.

More than it, to be precise, it must push the zeroing of the
runtime of an yielded task to the replenishment code. Your
patch for the sched yield already do it :-) but once there,
the zeroing can't be moved back without break this
tracepoint :-(.

8) we still need to define a tracepoint for replenishment, but
as it takes place on the following points:

a) sched_setattr()
b) periodic replenishment
c) on the wakeup after a blocking.

There is no a "single tracepoint" already available to "attach" this
information. So seems the replenishment tracepoint is still needed.

Moreover, although other scheduler may have this concept, I am
not sure if they take place under the same conditions.

So, AFAICS, this integrate approach is neither adding more information,
nor preventing ABI problems now and possibly in the future.

On the other hand, although a per-scheduler approach would increase
the number of tracepoints, the tracepoints tends to be simpler, confined
to the scheduler's .c and more coherent to the scheduler's abstractions.

I fear that the mix of the deadline scheduler and other schedulers
information, and the adaptation on other tracepoints to make the deadline
scheduler points of interest fit on current tracepoints, would add
undesired noise/imprecision on the interpretation of the sched deadline
actions/abstractions :-(.

-- Daniel

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web