[PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[Juri Lelli]

setup_new_dl_entity() takes two parameters, but it only actually uses
one of them, under a different name, to setup a new dl_entity, after:

 2f9f3fdc928 "sched/deadline: Remove dl_new from struct sched_dl_entity"

as we currently do

 setup_new_dl_entity(&p->dl, &p->dl)

However, before Luca's change we were doing

 setup_new_dl_entity(dl_se, pi_se)

in update_dl_entity() for a dl_se->new entity: we were using pi_se's
parameters (the potential PI donor) for setting up a new entity.

Restore this behaviour (as we want to correctly initialize parameters of
a boosted task that enters DEADLINE) by removing the useless second
parameter of setup_new_dl_entity() and retrieving the top waiter
directly from inside that function.

Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Luca Abeni <luca.abeni@unitn.it>
Signed-off-by: Juri Lelli <juri.lelli@arm.com>

---
 Changes from v1:
   - Steve pointed out that we were actually using the second parameter
     to permorm initialization
   - Luca confirmed that behavior is slightly changed w.r.t. before his
     change
   - changelog updated and original behavior restored
---
 kernel/sched/deadline.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index fcb7f0217ff4..2000ad2294d5 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -346,11 +346,12 @@ static void check_preempt_curr_dl(struct rq *rq, struct task_struct *p,
  * one, and to (try to!) reconcile itself with its own scheduling
  * parameters.
  */
-static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
-				       struct sched_dl_entity *pi_se)
+static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
 {
 	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
 	struct rq *rq = rq_of_dl_rq(dl_rq);
+	struct task_struct *pi_task = rt_mutex_get_top_task(dl_task_of(dl_se));
+	struct sched_dl_entity *pi_se = dl_se;
 
 	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
 
@@ -363,6 +364,13 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
 		return;
 
 	/*
+	 * Use the scheduling parameters of the top pi-waiter task,
+	 * if we have one from which we can inherit a deadline.
+	 */
+	if (pi_task && dl_se->dl_boosted && dl_prio(pi_task->normal_prio))
+		pi_se = &pi_task->dl;
+
+	/*
 	 * We use the regular wall clock time to set deadlines in the
 	 * future; in fact, we must consider execution overheads (time
 	 * spent on hardirq context, etc.).
@@ -1721,7 +1729,7 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
 static void switched_to_dl(struct rq *rq, struct task_struct *p)
 {
 	if (dl_time_before(p->dl.deadline, rq_clock(rq)))
-		setup_new_dl_entity(&p->dl, &p->dl);
+		setup_new_dl_entity(&p->dl);
 
 	if (task_on_rq_queued(p) && rq->curr != p) {
 #ifdef CONFIG_SMP
-- 
2.7.0

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[luca abeni]

Hi Juri,

On Wed, 29 Jun 2016 20:07:43 +0100
Juri Lelli <juri.lelli@arm.com> wrote:

> setup_new_dl_entity() takes two parameters, but it only actually uses
> one of them, under a different name, to setup a new dl_entity, after:
> 
>  2f9f3fdc928 "sched/deadline: Remove dl_new from struct
> sched_dl_entity"
> 
> as we currently do
> 
>  setup_new_dl_entity(&p->dl, &p->dl)
> 
> However, before Luca's change we were doing
> 
>  setup_new_dl_entity(dl_se, pi_se)
> 
> in update_dl_entity() for a dl_se->new entity: we were using pi_se's
> parameters (the potential PI donor) for setting up a new entity.
> 
> Restore this behaviour (as we want to correctly initialize parameters
> of a boosted task that enters DEADLINE) by removing the useless second
> parameter of setup_new_dl_entity() and retrieving the top waiter
> directly from inside that function.
I did not have time to test this patch yet, but it still looks good to
me.



			Thanks,
				Luca


> 
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Luca Abeni <luca.abeni@unitn.it>
> Signed-off-by: Juri Lelli <juri.lelli@arm.com>
> 
> ---
>  Changes from v1:
>    - Steve pointed out that we were actually using the second
> parameter to permorm initialization
>    - Luca confirmed that behavior is slightly changed w.r.t. before
> his change
>    - changelog updated and original behavior restored
> ---
>  kernel/sched/deadline.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fcb7f0217ff4..2000ad2294d5 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -346,11 +346,12 @@ static void check_preempt_curr_dl(struct rq
> *rq, struct task_struct *p,
>   * one, and to (try to!) reconcile itself with its own scheduling
>   * parameters.
>   */
> -static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
> -				       struct sched_dl_entity *pi_se)
> +static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
>  {
>  	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
>  	struct rq *rq = rq_of_dl_rq(dl_rq);
> +	struct task_struct *pi_task =
> rt_mutex_get_top_task(dl_task_of(dl_se));
> +	struct sched_dl_entity *pi_se = dl_se;
>  
>  	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
>  
> @@ -363,6 +364,13 @@ static inline void setup_new_dl_entity(struct
> sched_dl_entity *dl_se, return;
>  
>  	/*
> +	 * Use the scheduling parameters of the top pi-waiter task,
> +	 * if we have one from which we can inherit a deadline.
> +	 */
> +	if (pi_task && dl_se->dl_boosted &&
> dl_prio(pi_task->normal_prio))
> +		pi_se = &pi_task->dl;
> +
> +	/*
>  	 * We use the regular wall clock time to set deadlines in the
>  	 * future; in fact, we must consider execution overheads
> (time
>  	 * spent on hardirq context, etc.).
> @@ -1721,7 +1729,7 @@ static void switched_from_dl(struct rq *rq,
> struct task_struct *p) static void switched_to_dl(struct rq *rq,
> struct task_struct *p) {
>  	if (dl_time_before(p->dl.deadline, rq_clock(rq)))
> -		setup_new_dl_entity(&p->dl, &p->dl);
> +		setup_new_dl_entity(&p->dl);
>  
>  	if (task_on_rq_queued(p) && rq->curr != p) {
>  #ifdef CONFIG_SMP

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[Juri Lelli]

On 04/07/16 11:03, Luca Abeni wrote:
> Hi Juri,
> 
> On Wed, 29 Jun 2016 20:07:43 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
> > setup_new_dl_entity() takes two parameters, but it only actually uses
> > one of them, under a different name, to setup a new dl_entity, after:
> > 
> >  2f9f3fdc928 "sched/deadline: Remove dl_new from struct
> > sched_dl_entity"
> > 
> > as we currently do
> > 
> >  setup_new_dl_entity(&p->dl, &p->dl)
> > 
> > However, before Luca's change we were doing
> > 
> >  setup_new_dl_entity(dl_se, pi_se)
> > 
> > in update_dl_entity() for a dl_se->new entity: we were using pi_se's
> > parameters (the potential PI donor) for setting up a new entity.
> > 
> > Restore this behaviour (as we want to correctly initialize parameters
> > of a boosted task that enters DEADLINE) by removing the useless second
> > parameter of setup_new_dl_entity() and retrieving the top waiter
> > directly from inside that function.
> I did not have time to test this patch yet, but it still looks good to
> me.
> 

Thanks for reviewing it.

Best,

- Juri

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[Wanpeng Li]

2016-06-30 3:07 GMT+08:00 Juri Lelli <juri.lelli@arm.com>:
> setup_new_dl_entity() takes two parameters, but it only actually uses
> one of them, under a different name, to setup a new dl_entity, after:
>
>  2f9f3fdc928 "sched/deadline: Remove dl_new from struct sched_dl_entity"
>
> as we currently do
>
>  setup_new_dl_entity(&p->dl, &p->dl)
>
> However, before Luca's change we were doing
>
>  setup_new_dl_entity(dl_se, pi_se)
>
> in update_dl_entity() for a dl_se->new entity: we were using pi_se's
> parameters (the potential PI donor) for setting up a new entity.
>
> Restore this behaviour (as we want to correctly initialize parameters of
> a boosted task that enters DEADLINE) by removing the useless second
> parameter of setup_new_dl_entity() and retrieving the top waiter
> directly from inside that function.
>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Luca Abeni <luca.abeni@unitn.it>
> Signed-off-by: Juri Lelli <juri.lelli@arm.com>
>

Reviewed-by: Wanpeng Li <wanpeng.li@hotmail.com>

> ---
>  Changes from v1:
>    - Steve pointed out that we were actually using the second parameter
>      to permorm initialization
>    - Luca confirmed that behavior is slightly changed w.r.t. before his
>      change
>    - changelog updated and original behavior restored
> ---
>  kernel/sched/deadline.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fcb7f0217ff4..2000ad2294d5 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -346,11 +346,12 @@ static void check_preempt_curr_dl(struct rq *rq, struct task_struct *p,
>   * one, and to (try to!) reconcile itself with its own scheduling
>   * parameters.
>   */
> -static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
> -                                      struct sched_dl_entity *pi_se)
> +static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
>  {
>         struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
>         struct rq *rq = rq_of_dl_rq(dl_rq);
> +       struct task_struct *pi_task = rt_mutex_get_top_task(dl_task_of(dl_se));
> +       struct sched_dl_entity *pi_se = dl_se;
>
>         WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
>
> @@ -363,6 +364,13 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
>                 return;
>
>         /*
> +        * Use the scheduling parameters of the top pi-waiter task,
> +        * if we have one from which we can inherit a deadline.
> +        */
> +       if (pi_task && dl_se->dl_boosted && dl_prio(pi_task->normal_prio))
> +               pi_se = &pi_task->dl;
> +
> +       /*
>          * We use the regular wall clock time to set deadlines in the
>          * future; in fact, we must consider execution overheads (time
>          * spent on hardirq context, etc.).
> @@ -1721,7 +1729,7 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
>  static void switched_to_dl(struct rq *rq, struct task_struct *p)
>  {
>         if (dl_time_before(p->dl.deadline, rq_clock(rq)))
> -               setup_new_dl_entity(&p->dl, &p->dl);
> +               setup_new_dl_entity(&p->dl);
>
>         if (task_on_rq_queued(p) && rq->curr != p) {
>  #ifdef CONFIG_SMP
> --
> 2.7.0
>



-- 
Regards,
Wanpeng Li

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[Juri Lelli]

On 05/07/16 15:37, Wanpeng Li wrote:
> 2016-06-30 3:07 GMT+08:00 Juri Lelli <juri.lelli@arm.com>:
> > setup_new_dl_entity() takes two parameters, but it only actually uses
> > one of them, under a different name, to setup a new dl_entity, after:
> >
> >  2f9f3fdc928 "sched/deadline: Remove dl_new from struct sched_dl_entity"
> >
> > as we currently do
> >
> >  setup_new_dl_entity(&p->dl, &p->dl)
> >
> > However, before Luca's change we were doing
> >
> >  setup_new_dl_entity(dl_se, pi_se)
> >
> > in update_dl_entity() for a dl_se->new entity: we were using pi_se's
> > parameters (the potential PI donor) for setting up a new entity.
> >
> > Restore this behaviour (as we want to correctly initialize parameters of
> > a boosted task that enters DEADLINE) by removing the useless second
> > parameter of setup_new_dl_entity() and retrieving the top waiter
> > directly from inside that function.
> >
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Luca Abeni <luca.abeni@unitn.it>
> > Signed-off-by: Juri Lelli <juri.lelli@arm.com>
> >
> 
> Reviewed-by: Wanpeng Li <wanpeng.li@hotmail.com>
> 

Thanks!

- Juri

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[Steven Rostedt]

On Wed, 29 Jun 2016 20:07:43 +0100
Juri Lelli <juri.lelli@arm.com> wrote:


> ---
>  kernel/sched/deadline.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fcb7f0217ff4..2000ad2294d5 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -346,11 +346,12 @@ static void check_preempt_curr_dl(struct rq *rq, struct task_struct *p,
>   * one, and to (try to!) reconcile itself with its own scheduling
>   * parameters.
>   */
> -static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
> -				       struct sched_dl_entity *pi_se)
> +static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
>  {
>  	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
>  	struct rq *rq = rq_of_dl_rq(dl_rq);
> +	struct task_struct *pi_task = rt_mutex_get_top_task(dl_task_of(dl_se));
> +	struct sched_dl_entity *pi_se = dl_se;
>  
>  	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
>  
> @@ -363,6 +364,13 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
>  		return;
>  
>  	/*
> +	 * Use the scheduling parameters of the top pi-waiter task,
> +	 * if we have one from which we can inherit a deadline.
> +	 */
> +	if (pi_task && dl_se->dl_boosted && dl_prio(pi_task->normal_prio))
> +		pi_se = &pi_task->dl;
> +

OK, I'm micro-optimizing now, but hey, isn't this a fast path?

What about changing the above to:

	struct task_struct *pi_task;
	[...]

	if (dl_se->dl_boosted && dl_prio(pi_task->normal_prio &&
	    (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se)))
		pe_se = &pi_task->dl;

This way we don't need to do any work of looking at
rt_mutex_get_top_task() for the normal case.

-- Steve


> +	/*
>  	 * We use the regular wall clock time to set deadlines in the
>  	 * future; in fact, we must consider execution overheads (time
>  	 * spent on hardirq context, etc.).
> @@ -1721,7 +1729,7 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
>  static void switched_to_dl(struct rq *rq, struct task_struct *p)
>  {
>  	if (dl_time_before(p->dl.deadline, rq_clock(rq)))
> -		setup_new_dl_entity(&p->dl, &p->dl);
> +		setup_new_dl_entity(&p->dl);
>  
>  	if (task_on_rq_queued(p) && rq->curr != p) {
>  #ifdef CONFIG_SMP

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[Juri Lelli]

On 05/07/16 10:20, Steven Rostedt wrote:
> On Wed, 29 Jun 2016 20:07:43 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
> 
> > ---
> >  kernel/sched/deadline.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index fcb7f0217ff4..2000ad2294d5 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -346,11 +346,12 @@ static void check_preempt_curr_dl(struct rq *rq, struct task_struct *p,
> >   * one, and to (try to!) reconcile itself with its own scheduling
> >   * parameters.
> >   */
> > -static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
> > -				       struct sched_dl_entity *pi_se)
> > +static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
> >  {
> >  	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
> >  	struct rq *rq = rq_of_dl_rq(dl_rq);
> > +	struct task_struct *pi_task = rt_mutex_get_top_task(dl_task_of(dl_se));
> > +	struct sched_dl_entity *pi_se = dl_se;
> >  
> >  	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
> >  
> > @@ -363,6 +364,13 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
> >  		return;
> >  
> >  	/*
> > +	 * Use the scheduling parameters of the top pi-waiter task,
> > +	 * if we have one from which we can inherit a deadline.
> > +	 */
> > +	if (pi_task && dl_se->dl_boosted && dl_prio(pi_task->normal_prio))
> > +		pi_se = &pi_task->dl;
> > +
> 
> OK, I'm micro-optimizing now, but hey, isn't this a fast path?
> 
> What about changing the above to:
> 
> 	struct task_struct *pi_task;
> 	[...]
> 
> 	if (dl_se->dl_boosted && dl_prio(pi_task->normal_prio &&
                                    ^
OK, we need to reorder these two
                                    V
> 	    (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se)))
> 		pe_se = &pi_task->dl;
> 
> This way we don't need to do any work of looking at
> rt_mutex_get_top_task() for the normal case.
> 

But, yes. Looks good to me. I'll shoot a v3 ASAP.

Thanks,

- Juri

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[Steven Rostedt]

On Tue, 5 Jul 2016 15:39:33 +0100
Juri Lelli <juri.lelli@arm.com> wrote:

		return;
> > >  
> > >  	/*
> > > +	 * Use the scheduling parameters of the top pi-waiter task,
> > > +	 * if we have one from which we can inherit a deadline.
> > > +	 */
> > > +	if (pi_task && dl_se->dl_boosted && dl_prio(pi_task->normal_prio))
> > > +		pi_se = &pi_task->dl;
> > > +  
> > 
> > OK, I'm micro-optimizing now, but hey, isn't this a fast path?
> > 
> > What about changing the above to:
> > 
> > 	struct task_struct *pi_task;
> > 	[...]
> > 
> > 	if (dl_se->dl_boosted && dl_prio(pi_task->normal_prio &&  
>                                     ^
> OK, we need to reorder these two
>                                     V
> > 	    (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se)))
> > 		pe_se = &pi_task->dl;

Opps, you're right.

> > 
> > This way we don't need to do any work of looking at
> > rt_mutex_get_top_task() for the normal case.
> >   
> 
> But, yes. Looks good to me. I'll shoot a v3 ASAP.

I have to ask, should there be any check if the dl_se has a shorter
deadline than the pi one?

-- Steve

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[Juri Lelli]

On 05/07/16 12:47, Steven Rostedt wrote:
> On Tue, 5 Jul 2016 15:39:33 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
> 		return;
> > > >  
> > > >  	/*
> > > > +	 * Use the scheduling parameters of the top pi-waiter task,
> > > > +	 * if we have one from which we can inherit a deadline.
> > > > +	 */
> > > > +	if (pi_task && dl_se->dl_boosted && dl_prio(pi_task->normal_prio))
> > > > +		pi_se = &pi_task->dl;
> > > > +  
> > > 
> > > OK, I'm micro-optimizing now, but hey, isn't this a fast path?
> > > 
> > > What about changing the above to:
> > > 
> > > 	struct task_struct *pi_task;
> > > 	[...]
> > > 
> > > 	if (dl_se->dl_boosted && dl_prio(pi_task->normal_prio &&  
> >                                     ^
> > OK, we need to reorder these two
> >                                     V
> > > 	    (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se)))
> > > 		pe_se = &pi_task->dl;
> 
> Opps, you're right.
> 
> > > 
> > > This way we don't need to do any work of looking at
> > > rt_mutex_get_top_task() for the normal case.
> > >   
> > 
> > But, yes. Looks good to me. I'll shoot a v3 ASAP.
> 
> I have to ask, should there be any check if the dl_se has a shorter
> deadline than the pi one?
> 

Yeah. I wondered the same actually. I convinced myself that, since the
task is boosted, we assume that the donor will have a shorter deadline.
We seem to be doing the same elsewhere, but Luca was saying some time
ago that the DI thing my have some problems and needs to be revised.
Is is fair enough fixing this bit in accordance with the current (maybe
broken) behaviour and then spend time reviewing the whole thing, or do
we want to do both at the same time (which will of course require more
time)?

Best,

- Juri

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[luca abeni]

On Tue, 5 Jul 2016 17:58:30 +0100
Juri Lelli <juri.lelli@arm.com> wrote:

> On 05/07/16 12:47, Steven Rostedt wrote:
> > On Tue, 5 Jul 2016 15:39:33 +0100
> > Juri Lelli <juri.lelli@arm.com> wrote:
> > 
> > 		return;  
> > > > >  
> > > > >  	/*
> > > > > +	 * Use the scheduling parameters of the top
> > > > > pi-waiter task,
> > > > > +	 * if we have one from which we can inherit a
> > > > > deadline.
> > > > > +	 */
> > > > > +	if (pi_task && dl_se->dl_boosted &&
> > > > > dl_prio(pi_task->normal_prio))
> > > > > +		pi_se = &pi_task->dl;
> > > > > +    
> > > > 
> > > > OK, I'm micro-optimizing now, but hey, isn't this a fast path?
> > > > 
> > > > What about changing the above to:
> > > > 
> > > > 	struct task_struct *pi_task;
> > > > 	[...]
> > > > 
> > > > 	if (dl_se->dl_boosted && dl_prio(pi_task->normal_prio
> > > > &&    
> > >                                     ^
> > > OK, we need to reorder these two
> > >                                     V  
> > > > 	    (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se)))
> > > > 		pe_se = &pi_task->dl;  
> > 
> > Opps, you're right.
> >   
> > > > 
> > > > This way we don't need to do any work of looking at
> > > > rt_mutex_get_top_task() for the normal case.
> > > >     
> > > 
> > > But, yes. Looks good to me. I'll shoot a v3 ASAP.  
> > 
> > I have to ask, should there be any check if the dl_se has a shorter
> > deadline than the pi one?
> >   
> 
> Yeah. I wondered the same actually. I convinced myself that, since the
> task is boosted, we assume that the donor will have a shorter
> deadline.

Do you mean relative deadline (dl_se->dl_deadline) or absolute
(scheduling) dealine (dl_se->deadline)?

If I understand well, here we are in setup_new_dl_entity(), right?
This should be called only from switched_to_dl(); so, dl_se is from a
task that is switching to -deadline. If it is dl_boosted, it means that
it is switching from SCHED_OTHER (or RT) to -deadline because of
inheritance... So, it is very likely that dl_se->dl_deadline is not
meaningful.

Moreover, setup_new_dl_entity() is only called if the current
scheduling deadline of the task is not usable (that is, if
"dl_time_before(p->dl.deadline, rq_clock(rq)"). So, dl_se->deadline
will be surely smaller than pi_se->deadline... But the inheritance has
to happen anyway.


> We seem to be doing the same elsewhere, but Luca was saying
> some time ago that the DI thing my have some problems and needs to be
> revised.

My doubts regarding the inheritance code currently used for -deadline
tasks are due to the fact that it is not clear which kind of
inheritance algorithm is used...
I think it should use deadline inheritance, that, AFAIK, says that when
task T1 block waiting for task T2, T2 can inherit T1's _absolute_
deadline - if it is earlier than T2's one.
But the current code seems to be using relative deadlines (dl_deadline)
to decide the inheritance...

Having a better look at this is in my TODO list... But I still need to
find some time :)



				Luca

> Is is fair enough fixing this bit in accordance with the
> current (maybe broken) behaviour and then spend time reviewing the
> whole thing, or do we want to do both at the same time (which will of
> course require more time)?
> 
> Best,
> 
> - Juri

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[luca abeni]

On Tue, 5 Jul 2016 10:20:13 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 29 Jun 2016 20:07:43 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
> 
> > ---
> >  kernel/sched/deadline.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index fcb7f0217ff4..2000ad2294d5 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -346,11 +346,12 @@ static void check_preempt_curr_dl(struct rq
> > *rq, struct task_struct *p,
> >   * one, and to (try to!) reconcile itself with its own scheduling
> >   * parameters.
> >   */
> > -static inline void setup_new_dl_entity(struct sched_dl_entity
> > *dl_se,
> > -				       struct sched_dl_entity
> > *pi_se) +static inline void setup_new_dl_entity(struct
> > sched_dl_entity *dl_se) {
> >  	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
> >  	struct rq *rq = rq_of_dl_rq(dl_rq);
> > +	struct task_struct *pi_task =
> > rt_mutex_get_top_task(dl_task_of(dl_se));
> > +	struct sched_dl_entity *pi_se = dl_se;
> >  
> >  	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
> >  
> > @@ -363,6 +364,13 @@ static inline void setup_new_dl_entity(struct
> > sched_dl_entity *dl_se, return;
> >  
> >  	/*
> > +	 * Use the scheduling parameters of the top pi-waiter task,
> > +	 * if we have one from which we can inherit a deadline.
> > +	 */
> > +	if (pi_task && dl_se->dl_boosted &&
> > dl_prio(pi_task->normal_prio))
> > +		pi_se = &pi_task->dl;
> > +  
> 
> OK, I'm micro-optimizing now, but hey, isn't this a fast path?
> 
> What about changing the above to:
> 
> 	struct task_struct *pi_task;
> 	[...]
> 
> 	if (dl_se->dl_boosted && dl_prio(pi_task->normal_prio &&
> 	    (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se)))
> 		pe_se = &pi_task->dl;
> 
> This way we don't need to do any work of looking at
> rt_mutex_get_top_task() for the normal case.

If something like this is done, I think that enqueue_task_dl() (that
contains similar code) should be updated too.


			Thanks,
				Luca

> 
> -- Steve
> 
> 
> > +	/*
> >  	 * We use the regular wall clock time to set deadlines in
> > the
> >  	 * future; in fact, we must consider execution overheads
> > (time
> >  	 * spent on hardirq context, etc.).
> > @@ -1721,7 +1729,7 @@ static void switched_from_dl(struct rq *rq,
> > struct task_struct *p) static void switched_to_dl(struct rq *rq,
> > struct task_struct *p) {
> >  	if (dl_time_before(p->dl.deadline, rq_clock(rq)))
> > -		setup_new_dl_entity(&p->dl, &p->dl);
> > +		setup_new_dl_entity(&p->dl);
> >  
> >  	if (task_on_rq_queued(p) && rq->curr != p) {
> >  #ifdef CONFIG_SMP  
> 

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[Juri Lelli]

On 06/07/16 10:44, Luca Abeni wrote:
> On Tue, 5 Jul 2016 17:58:30 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
> > On 05/07/16 12:47, Steven Rostedt wrote:
> > > On Tue, 5 Jul 2016 15:39:33 +0100
> > > Juri Lelli <juri.lelli@arm.com> wrote:
> > > 
> > > 		return;  
> > > > > >  
> > > > > >  	/*
> > > > > > +	 * Use the scheduling parameters of the top
> > > > > > pi-waiter task,
> > > > > > +	 * if we have one from which we can inherit a
> > > > > > deadline.
> > > > > > +	 */
> > > > > > +	if (pi_task && dl_se->dl_boosted &&
> > > > > > dl_prio(pi_task->normal_prio))
> > > > > > +		pi_se = &pi_task->dl;
> > > > > > +    
> > > > > 
> > > > > OK, I'm micro-optimizing now, but hey, isn't this a fast path?
> > > > > 
> > > > > What about changing the above to:
> > > > > 
> > > > > 	struct task_struct *pi_task;
> > > > > 	[...]
> > > > > 
> > > > > 	if (dl_se->dl_boosted && dl_prio(pi_task->normal_prio
> > > > > &&    
> > > >                                     ^
> > > > OK, we need to reorder these two
> > > >                                     V  
> > > > > 	    (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se)))
> > > > > 		pe_se = &pi_task->dl;  
> > > 
> > > Opps, you're right.
> > >   
> > > > > 
> > > > > This way we don't need to do any work of looking at
> > > > > rt_mutex_get_top_task() for the normal case.
> > > > >     
> > > > 
> > > > But, yes. Looks good to me. I'll shoot a v3 ASAP.  
> > > 
> > > I have to ask, should there be any check if the dl_se has a shorter
> > > deadline than the pi one?
> > >   
> > 
> > Yeah. I wondered the same actually. I convinced myself that, since the
> > task is boosted, we assume that the donor will have a shorter
> > deadline.
> 
> Do you mean relative deadline (dl_se->dl_deadline) or absolute
> (scheduling) dealine (dl_se->deadline)?
> 
> If I understand well, here we are in setup_new_dl_entity(), right?
> This should be called only from switched_to_dl(); so, dl_se is from a
> task that is switching to -deadline. If it is dl_boosted, it means that
> it is switching from SCHED_OTHER (or RT) to -deadline because of
> inheritance... So, it is very likely that dl_se->dl_deadline is not
> meaningful.
> 

Right, very same thought I also had (and forgot to mention). So, we
cannot really do here the check Steve was wondering about.

> Moreover, setup_new_dl_entity() is only called if the current
> scheduling deadline of the task is not usable (that is, if
> "dl_time_before(p->dl.deadline, rq_clock(rq)"). So, dl_se->deadline
> will be surely smaller than pi_se->deadline... But the inheritance has
> to happen anyway.
> 
> 
> > We seem to be doing the same elsewhere, but Luca was saying
> > some time ago that the DI thing my have some problems and needs to be
> > revised.
> 
> My doubts regarding the inheritance code currently used for -deadline
> tasks are due to the fact that it is not clear which kind of
> inheritance algorithm is used...
> I think it should use deadline inheritance, that, AFAIK, says that when
> task T1 block waiting for task T2, T2 can inherit T1's _absolute_
> deadline - if it is earlier than T2's one.
> But the current code seems to be using relative deadlines (dl_deadline)
> to decide the inheritance...
> 

True. Problem is however that, even if enforcing is disabled for a
boosted task, we keep postponing the task's deadline when it depletes
its runtime (soft-CBS). So, which one should we use to do so?

At the instant of time the task gets a new potential donor this donor
might have a shorter absolute deadline. But the task's relative deadline
might be better (shorter w.r.t. donor's relative) to postpone the
absolute one when needed.

> Having a better look at this is in my TODO list... But I still need to
> find some time :)
> 

Same here. No spare cycles right now to have a thorough look at this. :(

Thanks,

- Juri

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[Steven Rostedt]

On Thu, 7 Jul 2016 09:39:09 +0100
Juri Lelli <juri.lelli@arm.com> wrote:

 
> > Having a better look at this is in my TODO list... But I still need to
> > find some time :)
> >   
> 
> Same here. No spare cycles right now to have a thorough look at this. :(

All in all, this should not hold up the current patch set. Maybe mark
it with a "TODO" and try to remember to look into it at another time.
The chances that we are switching from a SCHED_OTHER/RT to DEADLINE
that is already boosted is extremely rare. It can happen, but it's not
critical enough to hold this up.

-- Steve

Re: [PATCH v2] sched/deadline: remove useless param from setup_new_dl_entity

[Juri Lelli]

On 07/07/16 09:47, Steven Rostedt wrote:
> On Thu, 7 Jul 2016 09:39:09 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
>  
> > > Having a better look at this is in my TODO list... But I still need to
> > > find some time :)
> > >   
> > 
> > Same here. No spare cycles right now to have a thorough look at this. :(
> 
> All in all, this should not hold up the current patch set. Maybe mark
> it with a "TODO" and try to remember to look into it at another time.

Added to my TODO list (right after reviewing Luca's reclaiming bits next
version and fix the cpuset issue :/) and posted v3.

> The chances that we are switching from a SCHED_OTHER/RT to DEADLINE
> that is already boosted is extremely rare. It can happen, but it's not
> critical enough to hold this up.
> 

Thanks,

- Juri