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

[PATCH v5] 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.

This change removes the useless second parameter of setup_new_dl_entity().
While we are at it we also optimize things further calling setup_new_dl_
entity() only for already queued tasks, since (as pointed out by Xunlei)
we already do the very same update at tasks wakeup time anyway. By doing
so, we don't need to worry about a potential PI donor anymore, as rt_
mutex_setprio() takes care of that already for us.

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>
Cc: Xunlei Pang <xpang@redhat.com>
Signed-off-by: Juri Lelli <juri.lelli@arm.com>

---
 Changes from v4:
   - remove dead code checking for boosted status, as never triggered
     after rt_mutex_setprio()

 Changes from v3:
   - call setup_new_dl_entity only if task is enqueued already

 Changes from v2:
   - optimize by calling rt_mutex_get_top_task only if the task is
     boosted (as suggested by Steve)

 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 | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index fcb7f0217ff4..9491dbe039e8 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -346,12 +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);
 
+	WARN_ON(dl_se->dl_boosted);
 	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
 
 	/*
@@ -367,8 +367,8 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
 	 * future; in fact, we must consider execution overheads (time
 	 * spent on hardirq context, etc.).
 	 */
-	dl_se->deadline = rq_clock(rq) + pi_se->dl_deadline;
-	dl_se->runtime = pi_se->dl_runtime;
+	dl_se->deadline = rq_clock(rq) + dl_se->dl_deadline;
+	dl_se->runtime = dl_se->dl_runtime;
 }
 
 /*
@@ -1720,19 +1720,28 @@ 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);
 
-	if (task_on_rq_queued(p) && rq->curr != p) {
+	if (task_on_rq_queued(p)) {
+		/*
+		 * If p is not queued we will update its parameters at next
+		 * wakeup. If p is dl_boosted we already updated its params in
+		 * rt_mutex_setprio()->enqueue_task(..., ENQUEUE_REPLENISH),
+		 * p's deadline being now already after rq_clock(rq).
+		 */
+		if (dl_time_before(p->dl.deadline, rq_clock(rq)))
+			setup_new_dl_entity(&p->dl);
+
+		if (rq->curr != p) {
 #ifdef CONFIG_SMP
-		if (tsk_nr_cpus_allowed(p) > 1 && rq->dl.overloaded)
-			queue_push_tasks(rq);
+			if (tsk_nr_cpus_allowed(p) > 1 && rq->dl.overloaded)
+				queue_push_tasks(rq);
 #else
-		if (dl_task(rq->curr))
-			check_preempt_curr_dl(rq, p, 0);
-		else
-			resched_curr(rq);
+			if (dl_task(rq->curr))
+				check_preempt_curr_dl(rq, p, 0);
+			else
+				resched_curr(rq);
 #endif
+		}
 	}
 }
 
-- 
2.7.0

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

[Xunlei Pang]

On 2016/08/05 at 18:09, Juri Lelli 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.
>
> This change removes the useless second parameter of setup_new_dl_entity().
> While we are at it we also optimize things further calling setup_new_dl_
> entity() only for already queued tasks, since (as pointed out by Xunlei)
> we already do the very same update at tasks wakeup time anyway. By doing
> so, we don't need to worry about a potential PI donor anymore, as rt_
> mutex_setprio() takes care of that already for us.
>
> 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>
> Cc: Xunlei Pang <xpang@redhat.com>
> Signed-off-by: Juri Lelli <juri.lelli@arm.com>

Reviewed-by: Xunlei Pang <xlpang@redhat.com>


>
> ---
>  Changes from v4:
>    - remove dead code checking for boosted status, as never triggered
>      after rt_mutex_setprio()
>
>  Changes from v3:
>    - call setup_new_dl_entity only if task is enqueued already
>
>  Changes from v2:
>    - optimize by calling rt_mutex_get_top_task only if the task is
>      boosted (as suggested by Steve)
>
>  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 | 35 ++++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fcb7f0217ff4..9491dbe039e8 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -346,12 +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);
>  
> +	WARN_ON(dl_se->dl_boosted);
>  	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
>  
>  	/*
> @@ -367,8 +367,8 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
>  	 * future; in fact, we must consider execution overheads (time
>  	 * spent on hardirq context, etc.).
>  	 */
> -	dl_se->deadline = rq_clock(rq) + pi_se->dl_deadline;
> -	dl_se->runtime = pi_se->dl_runtime;
> +	dl_se->deadline = rq_clock(rq) + dl_se->dl_deadline;
> +	dl_se->runtime = dl_se->dl_runtime;
>  }
>  
>  /*
> @@ -1720,19 +1720,28 @@ 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);
>  
> -	if (task_on_rq_queued(p) && rq->curr != p) {
> +	if (task_on_rq_queued(p)) {
> +		/*
> +		 * If p is not queued we will update its parameters at next
> +		 * wakeup. If p is dl_boosted we already updated its params in
> +		 * rt_mutex_setprio()->enqueue_task(..., ENQUEUE_REPLENISH),
> +		 * p's deadline being now already after rq_clock(rq).
> +		 */
> +		if (dl_time_before(p->dl.deadline, rq_clock(rq)))
> +			setup_new_dl_entity(&p->dl);
> +
> +		if (rq->curr != p) {
>  #ifdef CONFIG_SMP
> -		if (tsk_nr_cpus_allowed(p) > 1 && rq->dl.overloaded)
> -			queue_push_tasks(rq);
> +			if (tsk_nr_cpus_allowed(p) > 1 && rq->dl.overloaded)
> +				queue_push_tasks(rq);
>  #else
> -		if (dl_task(rq->curr))
> -			check_preempt_curr_dl(rq, p, 0);
> -		else
> -			resched_curr(rq);
> +			if (dl_task(rq->curr))
> +				check_preempt_curr_dl(rq, p, 0);
> +			else
> +				resched_curr(rq);
>  #endif
> +		}
>  	}
>  }
>  

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

[Steven Rostedt]

On Fri,  5 Aug 2016 11:09:59 +0100
Juri Lelli <juri.lelli@arm.com> wrote:

> @@ -1720,19 +1720,28 @@ 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);
>  
> -	if (task_on_rq_queued(p) && rq->curr != p) {
> +	if (task_on_rq_queued(p)) {

I always hated functions totally encapsulated by an if statement. This
can be a bit simpler (and less indented) if you have:

	/* If p is not queued, its parameters will be updated at wakeup */
	if (!task_on_rq_queued(p))
		return;

	[...]

Other than that, it looks good.

Reviewed-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> +		/*
> +		 * If p is not queued we will update its parameters at next
> +		 * wakeup. If p is dl_boosted we already updated its params in
> +		 * rt_mutex_setprio()->enqueue_task(..., ENQUEUE_REPLENISH),
> +		 * p's deadline being now already after rq_clock(rq).
> +		 */
> +		if (dl_time_before(p->dl.deadline, rq_clock(rq)))
> +			setup_new_dl_entity(&p->dl);
> +
> +		if (rq->curr != p) {
>  #ifdef CONFIG_SMP
> -		if (tsk_nr_cpus_allowed(p) > 1 && rq->dl.overloaded)
> -			queue_push_tasks(rq);
> +			if (tsk_nr_cpus_allowed(p) > 1 && rq->dl.overloaded)
> +				queue_push_tasks(rq);
>  #else
> -		if (dl_task(rq->curr))
> -			check_preempt_curr_dl(rq, p, 0);
> -		else
> -			resched_curr(rq);
> +			if (dl_task(rq->curr))
> +				check_preempt_curr_dl(rq, p, 0);
> +			else
> +				resched_curr(rq);
>  #endif
> +		}
>  	}
>  }
>  

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

[Juri Lelli]

On 05/08/16 09:56, Steven Rostedt wrote:
> On Fri,  5 Aug 2016 11:09:59 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
> > @@ -1720,19 +1720,28 @@ 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);
> >  
> > -	if (task_on_rq_queued(p) && rq->curr != p) {
> > +	if (task_on_rq_queued(p)) {
> 
> I always hated functions totally encapsulated by an if statement. This
> can be a bit simpler (and less indented) if you have:
> 
> 	/* If p is not queued, its parameters will be updated at wakeup */
> 	if (!task_on_rq_queued(p))
> 		return;
> 
> 	[...]
> 

You mean like what follows?

I'll post a v6 if OK.

Thanks,

- Juri

--->8---
 kernel/sched/deadline.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 9491dbe039e8..03f35f66cf1f 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1721,27 +1721,28 @@ 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 (task_on_rq_queued(p)) {
-		/*
-		 * If p is not queued we will update its parameters at next
-		 * wakeup. If p is dl_boosted we already updated its params in
-		 * rt_mutex_setprio()->enqueue_task(..., ENQUEUE_REPLENISH),
-		 * p's deadline being now already after rq_clock(rq).
-		 */
-		if (dl_time_before(p->dl.deadline, rq_clock(rq)))
-			setup_new_dl_entity(&p->dl);
+	/* If p is not queued we will update its parameters at next wakeup. */
+	if (!task_on_rq_queued(p))
+		return;
+
+	/*
+	 * If p is boosted we already updated its params in
+	 * rt_mutex_setprio()->enqueue_task(..., ENQUEUE_REPLENISH),
+	 * p's deadline being now already after rq_clock(rq).
+	 */
+	if (dl_time_before(p->dl.deadline, rq_clock(rq)))
+		setup_new_dl_entity(&p->dl);
 
-		if (rq->curr != p) {
+	if (rq->curr != p) {
 #ifdef CONFIG_SMP
-			if (tsk_nr_cpus_allowed(p) > 1 && rq->dl.overloaded)
-				queue_push_tasks(rq);
+		if (tsk_nr_cpus_allowed(p) > 1 && rq->dl.overloaded)
+			queue_push_tasks(rq);
 #else
-			if (dl_task(rq->curr))
-				check_preempt_curr_dl(rq, p, 0);
-			else
-				resched_curr(rq);
+		if (dl_task(rq->curr))
+			check_preempt_curr_dl(rq, p, 0);
+		else
+			resched_curr(rq);
 #endif
-		}
 	}
 }

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

[Steven Rostedt]

On Fri, 5 Aug 2016 15:34:44 +0100
Juri Lelli <juri.lelli@arm.com> wrote:

> On 05/08/16 09:56, Steven Rostedt wrote:
> > On Fri,  5 Aug 2016 11:09:59 +0100
> > Juri Lelli <juri.lelli@arm.com> wrote:
> >   
> > > @@ -1720,19 +1720,28 @@ 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);
> > >  
> > > -	if (task_on_rq_queued(p) && rq->curr != p) {
> > > +	if (task_on_rq_queued(p)) {  
> > 
> > I always hated functions totally encapsulated by an if statement. This
> > can be a bit simpler (and less indented) if you have:
> > 
> > 	/* If p is not queued, its parameters will be updated at wakeup */
> > 	if (!task_on_rq_queued(p))
> > 		return;
> > 
> > 	[...]
> >   
> 
> You mean like what follows?
> 
> I'll post a v6 if OK.
> 

Yes! I think that looks much nicer, and easier to read.

You can add my Reviewed-by tag too.

Thanks!

-- Steve

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

[Juri Lelli]

On 05/08/16 10:54, Steven Rostedt wrote:
> On Fri, 5 Aug 2016 15:34:44 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
> > On 05/08/16 09:56, Steven Rostedt wrote:
> > > On Fri,  5 Aug 2016 11:09:59 +0100
> > > Juri Lelli <juri.lelli@arm.com> wrote:
> > >   
> > > > @@ -1720,19 +1720,28 @@ 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);
> > > >  
> > > > -	if (task_on_rq_queued(p) && rq->curr != p) {
> > > > +	if (task_on_rq_queued(p)) {  
> > > 
> > > I always hated functions totally encapsulated by an if statement. This
> > > can be a bit simpler (and less indented) if you have:
> > > 
> > > 	/* If p is not queued, its parameters will be updated at wakeup */
> > > 	if (!task_on_rq_queued(p))
> > > 		return;
> > > 
> > > 	[...]
> > >   
> > 
> > You mean like what follows?
> > 
> > I'll post a v6 if OK.
> > 
> 
> Yes! I think that looks much nicer, and easier to read.
> 

Yep. Way better. :)

> You can add my Reviewed-by tag too.
> 

Thanks!

Best,

- Juri