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

Groups > linux.kernel > #1442338 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v4] sched/deadline: remove useless param from setup_new_dl_entity Juri Lelli <juri.lelli@arm.com> - 2016-07-13 13:00 +0200
    Re: [PATCH v4] sched/deadline: remove useless param from  setup_new_dl_entity Xunlei Pang <xpang@redhat.com> - 2016-07-15 12:40 +0200
      Re: [PATCH v4] sched/deadline: remove useless param from  setup_new_dl_entity Juri Lelli <juri.lelli@arm.com> - 2016-07-18 15:10 +0200
        Re: [PATCH v4] sched/deadline: remove useless param from  setup_new_dl_entity Xunlei Pang <xpang@redhat.com> - 2016-07-18 15:40 +0200
          Re: [PATCH v4] sched/deadline: remove useless param from  setup_new_dl_entity Juri Lelli <juri.lelli@arm.com> - 2016-07-21 16:30 +0200
            Re: [PATCH v4] sched/deadline: remove useless param from  setup_new_dl_entity Juri Lelli <juri.lelli@arm.com> - 2016-07-21 16:40 +0200
              Re: [PATCH v4] sched/deadline: remove useless param from  setup_new_dl_entity Juri Lelli <juri.lelli@arm.com> - 2016-07-21 16:50 +0200
                Re: [PATCH v4] sched/deadline: remove useless param from  setup_new_dl_entity Xunlei Pang <xpang@redhat.com> - 2016-07-22 05:00 +0200

#1442338 — [PATCH v4] sched/deadline: remove useless param from setup_new_dl_entity

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.

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.

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 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 | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index fcb7f0217ff4..dc56f5be0112 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;
+	struct sched_dl_entity *pi_se = dl_se;
 
 	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
 
@@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted &&
+	    (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) &&
+	    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.).
@@ -1720,19 +1730,26 @@ 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 (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.4

[toc] | [next] | [standalone]

#1444162 — Re: [PATCH v4] sched/deadline: remove useless param from setup_new_dl_entity

On 2016/07/13 at 18:58, 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.
>
> 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.
>
> 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.
>
> 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 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 | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fcb7f0217ff4..dc56f5be0112 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;
> +	struct sched_dl_entity *pi_se = dl_se;
>  
>  	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
>  
> @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted &&
> +	    (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) &&
> +	    dl_prio(pi_task->normal_prio))
> +		pi_se = &pi_task->dl;

Since this is only called for queued cases now, there is no need to
check boosted stuff here. As enqueue_task(ENQUEUE_REPLENISH)
is called before check_class_changed() in rt_mutex_setprio().

Regards,
Xunlei

> +
> +	/*
>  	 * 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.).
> @@ -1720,19 +1730,26 @@ 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 (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
> +		}
>  	}
>  }
>  

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

#1445471 — Re: [PATCH v4] sched/deadline: remove useless param from setup_new_dl_entity

On 15/07/16 18:39, Xunlei Pang wrote:
> On 2016/07/13 at 18:58, Juri Lelli wrote:

[...]

> 
> Since this is only called for queued cases now, there is no need to
> check boosted stuff here. As enqueue_task(ENQUEUE_REPLENISH)
> is called before check_class_changed() in rt_mutex_setprio().
> 

But we don't do the same in setscheduler, right?

Best,

- Juri

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

#1445485 — Re: [PATCH v4] sched/deadline: remove useless param from setup_new_dl_entity

On 2016/07/18 at 21:04, Juri Lelli wrote:
> On 15/07/16 18:39, Xunlei Pang wrote:
>> On 2016/07/13 at 18:58, Juri Lelli wrote:
> [...]
>
>> Since this is only called for queued cases now, there is no need to
>> check boosted stuff here. As enqueue_task(ENQUEUE_REPLENISH)
>> is called before check_class_changed() in rt_mutex_setprio().
>>
> But we don't do the same in setscheduler, right?

If p is deadline PI-boosted, setscheduler() won't call change its sched_class.
If p isn't deadline PI-boosted, then pi_task is NULL.

So, I think the added code won't hit. Did I miss something?

Regards,
Xunlei

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

#1447916 — Re: [PATCH v4] sched/deadline: remove useless param from setup_new_dl_entity

Hi,

On 18/07/16 21:37, Xunlei Pang wrote:
> On 2016/07/18 at 21:04, Juri Lelli wrote:
> > On 15/07/16 18:39, Xunlei Pang wrote:
> >> On 2016/07/13 at 18:58, Juri Lelli wrote:
> > [...]
> >
> >> Since this is only called for queued cases now, there is no need to
> >> check boosted stuff here. As enqueue_task(ENQUEUE_REPLENISH)
> >> is called before check_class_changed() in rt_mutex_setprio().
> >>
> > But we don't do the same in setscheduler, right?
> 
> If p is deadline PI-boosted, setscheduler() won't call change its sched_class.
> If p isn't deadline PI-boosted, then pi_task is NULL.
> 
> So, I think the added code won't hit. Did I miss something?
> 

No, I think you are right.

I'll shoot a v5 ASAP (I think I'll put a WARN_ON just in case).

Best,

- Juri

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

#1447922 — Re: [PATCH v4] sched/deadline: remove useless param from setup_new_dl_entity

On 21/07/16 15:21, Juri Lelli wrote:
> Hi,
> 
> On 18/07/16 21:37, Xunlei Pang wrote:
> > On 2016/07/18 at 21:04, Juri Lelli wrote:
> > > On 15/07/16 18:39, Xunlei Pang wrote:
> > >> On 2016/07/13 at 18:58, Juri Lelli wrote:
> > > [...]
> > >
> > >> Since this is only called for queued cases now, there is no need to
> > >> check boosted stuff here. As enqueue_task(ENQUEUE_REPLENISH)
> > >> is called before check_class_changed() in rt_mutex_setprio().
> > >>
> > > But we don't do the same in setscheduler, right?
> > 
> > If p is deadline PI-boosted, setscheduler() won't call change its sched_class.
> > If p isn't deadline PI-boosted, then pi_task is NULL.
> > 
> > So, I think the added code won't hit. Did I miss something?
> > 
> 
> No, I think you are right.
> 

Oh, and we need to filter the call after rt_mutex_setprio has
already issued a replenishment.

> I'll shoot a v5 ASAP (I think I'll put a WARN_ON just in case).
> 
> Best,
> 
> - Juri
> 

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

#1447927 — Re: [PATCH v4] sched/deadline: remove useless param from setup_new_dl_entity

On 21/07/16 15:36, Juri Lelli wrote:
> On 21/07/16 15:21, Juri Lelli wrote:
> > Hi,
> > 
> > On 18/07/16 21:37, Xunlei Pang wrote:
> > > On 2016/07/18 at 21:04, Juri Lelli wrote:
> > > > On 15/07/16 18:39, Xunlei Pang wrote:
> > > >> On 2016/07/13 at 18:58, Juri Lelli wrote:
> > > > [...]
> > > >
> > > >> Since this is only called for queued cases now, there is no need to
> > > >> check boosted stuff here. As enqueue_task(ENQUEUE_REPLENISH)
> > > >> is called before check_class_changed() in rt_mutex_setprio().
> > > >>
> > > > But we don't do the same in setscheduler, right?
> > > 
> > > If p is deadline PI-boosted, setscheduler() won't call change its sched_class.
> > > If p isn't deadline PI-boosted, then pi_task is NULL.
> > > 
> > > So, I think the added code won't hit. Did I miss something?
> > > 
> > 
> > No, I think you are right.
> > 
> 
> Oh, and we need to filter the call after rt_mutex_setprio has
> already issued a replenishment.
> 

Does something like this on top of v4 make sense?

---
 kernel/sched/deadline.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index dc56f5be0112..6f05ac78711c 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -350,9 +350,8 @@ 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;
-	struct sched_dl_entity *pi_se = dl_se;
 
+	WARN_ON(dl_se->dl_boosted);
 	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
 
 	/*
@@ -364,21 +363,12 @@ 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 (dl_se->dl_boosted &&
-	    (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) &&
-	    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.).
 	 */
-	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;
 }
 
 /*
@@ -1734,9 +1724,11 @@ 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.
+		 * wakeup. If p is dl_boosted we already updated its params in
+		 * rt_mutex_setprio()->enqueue_task(..., ENQUEUE_REPLENISH).
 		 */
-		if (dl_time_before(p->dl.deadline, rq_clock(rq)))
+		if (dl_time_before(p->dl.deadline, rq_clock(rq)) &&
+				!p->dl.dl_boosted)
 			setup_new_dl_entity(&p->dl);
 
 		if (rq->curr != p) {
-- 

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

#1448320 — Re: [PATCH v4] sched/deadline: remove useless param from setup_new_dl_entity

On 2016/07/21 at 22:46, Juri Lelli wrote:
> On 21/07/16 15:36, Juri Lelli wrote:
>> On 21/07/16 15:21, Juri Lelli wrote:
>>> Hi,
>>>
>>> On 18/07/16 21:37, Xunlei Pang wrote:
>>>> On 2016/07/18 at 21:04, Juri Lelli wrote:
>>>>> On 15/07/16 18:39, Xunlei Pang wrote:
>>>>>> On 2016/07/13 at 18:58, Juri Lelli wrote:
>>>>> [...]
>>>>>
>>>>>> Since this is only called for queued cases now, there is no need to
>>>>>> check boosted stuff here. As enqueue_task(ENQUEUE_REPLENISH)
>>>>>> is called before check_class_changed() in rt_mutex_setprio().
>>>>>>
>>>>> But we don't do the same in setscheduler, right?
>>>> If p is deadline PI-boosted, setscheduler() won't call change its sched_class.
>>>> If p isn't deadline PI-boosted, then pi_task is NULL.
>>>>
>>>> So, I think the added code won't hit. Did I miss something?
>>>>
>>> No, I think you are right.
>>>
>> Oh, and we need to filter the call after rt_mutex_setprio has
>> already issued a replenishment.
>>
> Does something like this on top of v4 make sense?
>
> ---
>  kernel/sched/deadline.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index dc56f5be0112..6f05ac78711c 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -350,9 +350,8 @@ 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;
> -	struct sched_dl_entity *pi_se = dl_se;
>  
> +	WARN_ON(dl_se->dl_boosted);
>  	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
>  
>  	/*
> @@ -364,21 +363,12 @@ 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 (dl_se->dl_boosted &&
> -	    (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) &&
> -	    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.).
>  	 */
> -	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;
>  }
>  
>  /*
> @@ -1734,9 +1724,11 @@ 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.
> +		 * wakeup. If p is dl_boosted we already updated its params in
> +		 * rt_mutex_setprio()->enqueue_task(..., ENQUEUE_REPLENISH).
>  		 */
> -		if (dl_time_before(p->dl.deadline, rq_clock(rq)))
> +		if (dl_time_before(p->dl.deadline, rq_clock(rq)) &&
> +				!p->dl.dl_boosted)

Hi Juri,

It looks good to me, only one question:
For on_rq boosted to deadline, p->dl.deadline has been updated after rq_lock(rq) by
rt_mutex_setprio()->enqueue_task(..., ENQUEUE_REPLENISH) and no rq clock update
afterwards, so dl_time_before() will be false, seems p->dl.dl_boosted check is needless.

Regards,
Xunlei

>  			setup_new_dl_entity(&p->dl);
>  
>  		if (rq->curr != p) {

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web