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

Groups > linux.kernel > #1424855 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] sched/deadline: remove useless param from setup_new_dl_entity Juri Lelli <juri.lelli@arm.com> - 2016-06-17 11:50 +0200
    Re: [PATCH] sched/deadline: remove useless param from  setup_new_dl_entity luca abeni <luca.abeni@unitn.it> - 2016-06-17 12:00 +0200
      Re: [PATCH] sched/deadline: remove useless param from  setup_new_dl_entity Juri Lelli <juri.lelli@arm.com> - 2016-06-17 12:10 +0200
    Re: [PATCH] sched/deadline: remove useless param from  setup_new_dl_entity Steven Rostedt <rostedt@goodmis.org> - 2016-06-17 15:50 +0200
      Re: [PATCH] sched/deadline: remove useless param from  setup_new_dl_entity Juri Lelli <juri.lelli@arm.com> - 2016-06-17 18:30 +0200
        Re: [PATCH] sched/deadline: remove useless param from  setup_new_dl_entity luca abeni <luca.abeni@unitn.it> - 2016-06-17 22:20 +0200
          Re: [PATCH] sched/deadline: remove useless param from  setup_new_dl_entity luca abeni <luca.abeni@unitn.it> - 2016-06-17 22:40 +0200

#1424855 — [PATCH] 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 to setup a new dl_entity.

Remove the second, useless, parameter.

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>
---
 kernel/sched/deadline.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index fcb7f0217ff4..5229788a4765 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -346,8 +346,7 @@ 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);
@@ -367,8 +366,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;
 }
 
 /*
@@ -1721,7 +1720,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

[toc] | [next] | [standalone]

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

Hi,

On Fri, 17 Jun 2016 10:48:41 +0100
Juri Lelli <juri.lelli@arm.com> wrote:

> setup_new_dl_entity() takes two parameters, but it only actually uses
> one of them to setup a new dl_entity.
> 
> Remove the second, useless, parameter.

Funnily enough, I was adding something similar in my local queue :)
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>
> ---
>  kernel/sched/deadline.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fcb7f0217ff4..5229788a4765 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -346,8 +346,7 @@ 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);
> @@ -367,8 +366,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;
>  }
>  
>  /*
> @@ -1721,7 +1720,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

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

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

On 17/06/16 11:58, Luca Abeni wrote:
> Hi,
> 
> On Fri, 17 Jun 2016 10:48:41 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
> > setup_new_dl_entity() takes two parameters, but it only actually uses
> > one of them to setup a new dl_entity.
> > 
> > Remove the second, useless, parameter.
> 
> Funnily enough, I was adding something similar in my local queue :)
> Looks good to me
> 

Yeah, noticed this while looking at your reclaiming patches. :)

Thanks for reviewing,

- Juri

> 
> > 
> > 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>
> > ---
> >  kernel/sched/deadline.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index fcb7f0217ff4..5229788a4765 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -346,8 +346,7 @@ 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);
> > @@ -367,8 +366,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;
> >  }
> >  
> >  /*
> > @@ -1721,7 +1720,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
> 

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

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

On Fri, 17 Jun 2016 10:48:41 +0100
Juri Lelli <juri.lelli@arm.com> wrote:

> setup_new_dl_entity() takes two parameters, but it only actually uses
> one of them to setup a new dl_entity.
> 

Actually this patch is making it so that setup_new_dl_entity() only
uses one of the parameters. Can you note why that change happened.
Because this change log implies that the second parameter wasn't used
before this patch, and that is incorrect.

This patch reverts part of the change done in
commit 2d3d891d334 "sched/deadline: Add SCHED_DEADLINE inheritance
logic"

It would be nice to have the reason in the change log.

Thanks,

-- Steve


> Remove the second, useless, parameter.
> 
> 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>
> ---
>  kernel/sched/deadline.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fcb7f0217ff4..5229788a4765 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -346,8 +346,7 @@ 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);
> @@ -367,8 +366,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;
>  }
>  
>  /*
> @@ -1721,7 +1720,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

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

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

On 17/06/16 09:49, Steven Rostedt wrote:
> On Fri, 17 Jun 2016 10:48:41 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
> > setup_new_dl_entity() takes two parameters, but it only actually uses
> > one of them to setup a new dl_entity.
> > 
> 
> Actually this patch is making it so that setup_new_dl_entity() only
> uses one of the parameters. Can you note why that change happened.
> Because this change log implies that the second parameter wasn't used
> before this patch, and that is incorrect.
> 

True, but we were practically already using the same parameter, under a
different name though, after

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

as we currently do:

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

> This patch reverts part of the change done in
> commit 2d3d891d334 "sched/deadline: Add SCHED_DEADLINE inheritance
> logic"
> 

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. So, I guess the question
is actually why we wanted to use pi_se's parameters (the potential PI
donor) for setting up a new entity? Maybe we broke the situation where a
task is currently boosted by a DEADLINE waiter and we swich the holder
to DEADLINE?

> It would be nice to have the reason in the change log.
> 

Thanks a lot for pointing out what might be more than inaccuracy in the
changelog.

Best,

- Juri

> 
> > Remove the second, useless, parameter.
> > 
> > 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>
> > ---
> >  kernel/sched/deadline.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index fcb7f0217ff4..5229788a4765 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -346,8 +346,7 @@ 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);
> > @@ -367,8 +366,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;
> >  }
> >  
> >  /*
> > @@ -1721,7 +1720,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
> 

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

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

On Fri, 17 Jun 2016 17:28:37 +0100
Juri Lelli <juri.lelli@arm.com> wrote:
[...]
> True, but we were practically already using the same parameter, under a
> different name though, after
> 
> 2f9f3fdc928 "sched/deadline: Remove dl_new from struct sched_dl_entity"
> 
> as we currently do:
> 
>   setup_new_dl_entity(&p->dl, &p->dl)
> 
> > This patch reverts part of the change done in
> > commit 2d3d891d334 "sched/deadline: Add SCHED_DEADLINE inheritance
> > logic"
> >   
> 
> 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. So, I guess the question
> is actually why we wanted to use pi_se's parameters (the potential PI
> donor) for setting up a new entity?
That's a good question :)

> Maybe we broke the situation where a
> task is currently boosted by a DEADLINE waiter and we swich the holder
> to DEADLINE?
I remember I tested this setup (using linaro's version of rt-app), and
it seemed to work correctly...

Re-reading the code now, I actually wonder why my patch did not break
inheritance in this situation...


			Luca
> 
> > It would be nice to have the reason in the change log.
> >   
> 
> Thanks a lot for pointing out what might be more than inaccuracy in the
> changelog.
> 
> Best,
> 
> - Juri
> 
> >   
> > > Remove the second, useless, parameter.
> > > 
> > > 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>
> > > ---
> > >  kernel/sched/deadline.c | 9 ++++-----
> > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > > index fcb7f0217ff4..5229788a4765 100644
> > > --- a/kernel/sched/deadline.c
> > > +++ b/kernel/sched/deadline.c
> > > @@ -346,8 +346,7 @@ 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);
> > > @@ -367,8 +366,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;
> > >  }
> > >  
> > >  /*
> > > @@ -1721,7 +1720,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  
> >   

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

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

On Fri, 17 Jun 2016 22:15:18 +0200
luca abeni <luca.abeni@unitn.it> wrote:

> On Fri, 17 Jun 2016 17:28:37 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> [...]
> > True, but we were practically already using the same parameter, under a
> > different name though, after
> > 
> > 2f9f3fdc928 "sched/deadline: Remove dl_new from struct sched_dl_entity"
> > 
> > as we currently do:
> > 
> >   setup_new_dl_entity(&p->dl, &p->dl)
> >   
> > > This patch reverts part of the change done in
> > > commit 2d3d891d334 "sched/deadline: Add SCHED_DEADLINE inheritance
> > > logic"
> > >     
> > 
> > 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. So, I guess the question
> > is actually why we wanted to use pi_se's parameters (the potential PI
> > donor) for setting up a new entity?  
> That's a good question :)
> 
> > Maybe we broke the situation where a
> > task is currently boosted by a DEADLINE waiter and we swich the holder
> > to DEADLINE?  
> I remember I tested this setup (using linaro's version of rt-app), and
> it seemed to work correctly...
> 
> Re-reading the code now, I actually wonder why my patch did not break
> inheritance in this situation...
Ok; I think I know why inheritance is not broken (or, at least, it does
not appear to be broken when testing it with rt-app):
- When a -deadline task blocks on a mutex that is held by a SCHED_OTHER
  or SCHED_FIFO task, such a task is promoted to -deadline
- setup_new_dl_entity() is invoked, and it sets the tasks' deadline to
  rq_clock(rq) (+ 0), so the task holding the lock is immediately
  scheduled
- as soon as update_curr_dl() is invoked (in the worst case at the next
  tick), the task's deadline and runtime are set to the "desired values"
  (using pi_se)

So, the behaviour is not changed too much respect to the previous one.



				Luca

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web