All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] sched/deadline: Change the time to replenish runtime for sleep tasks
@ 2017-02-23  6:14 Byungchul Park
  2017-02-23 10:11 ` Byungchul Park
  2017-02-28 11:35 ` Juri Lelli
  0 siblings, 2 replies; 8+ messages in thread
From: Byungchul Park @ 2017-02-23  6:14 UTC (permalink / raw)
  To: peterz, mingo; +Cc: linux-kernel, juri.lelli, rostedt, kernel-team

Let's consider the following example.

timeline : o...................o.........o.......o..o
           ^                   ^         ^       ^  ^
           |                   |         |       |  |
       start                   |         |       |  |
                original runtime         |       |  |
                     sleep with (-)runtime       |  |
                                 original deadline  |
                                              wake up

When this task is woken up, a negative runtime should be considered,
which means that the task should get penalized when assigning runtime,
becasue it already spent more than expected. Current code handles this
by replenishing a runtime in hrtimer callback for deadline. But this
approach has room for improvement:

   It will be replenished twice unnecessarily if the task sleeps for
   long time so that the deadline, assigned in the hrtimer callback,
   also passed. In other words, one happens in the callback and the
   other happens in update_dl_entiry() when waking it up.

So force to replenish it for sleep tasks when waking it up.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/sched/deadline.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 27737f3..cb43ce9 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -498,8 +498,9 @@ static void update_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);
 
-	if (dl_time_before(dl_se->deadline, rq_clock(rq)) ||
-	    dl_entity_overflow(dl_se, pi_se, rq_clock(rq))) {
+	if (dl_time_before(dl_se->deadline, rq_clock(rq)))
+		replenish_dl_entity(dl_se, pi_se);
+	else if (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;
 	}
@@ -621,13 +622,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
 	 *         __dequeue_task_dl()
 	 *     prev->on_rq = 0;
 	 *
-	 * We can be both throttled and !queued. Replenish the counter
-	 * but do not enqueue -- wait for our wakeup to do that.
+	 * We can be both throttled and !queued. Wait for our wakeup to
+	 * replenish runtime and enqueue p.
 	 */
-	if (!task_on_rq_queued(p)) {
-		replenish_dl_entity(dl_se, dl_se);
+	if (!task_on_rq_queued(p))
 		goto unlock;
-	}
 
 #ifdef CONFIG_SMP
 	if (unlikely(!rq->online)) {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] sched/deadline: Change the time to replenish runtime for sleep tasks
  2017-02-23  6:14 [PATCH v3] sched/deadline: Change the time to replenish runtime for sleep tasks Byungchul Park
@ 2017-02-23 10:11 ` Byungchul Park
  2017-02-28 11:35 ` Juri Lelli
  1 sibling, 0 replies; 8+ messages in thread
From: Byungchul Park @ 2017-02-23 10:11 UTC (permalink / raw)
  To: peterz, mingo; +Cc: linux-kernel, juri.lelli, rostedt, kernel-team

On Thu, Feb 23, 2017 at 03:14:59PM +0900, Byungchul Park wrote:
> Let's consider the following example.
> 
> timeline : o...................o.........o.......o..o
>            ^                   ^         ^       ^  ^
>            |                   |         |       |  |
>        start                   |         |       |  |
>                 original runtime         |       |  |
>                      sleep with (-)runtime       |  |
>                                  original deadline  |
>                                               wake up
> 
> When this task is woken up, a negative runtime should be considered,
> which means that the task should get penalized when assigning runtime,
> becasue it already spent more than expected. Current code handles this
> by replenishing a runtime in hrtimer callback for deadline. But this
> approach has room for improvement:
> 
>    It will be replenished twice unnecessarily if the task sleeps for
>    long time so that the deadline, assigned in the hrtimer callback,
>    also passed. In other words, one happens in the callback and the
>    other happens in update_dl_entiry() when waking it up.
> 
> So force to replenish it for sleep tasks when waking it up.

To be honest, I think this patch is more valuable when used with
removing unnecessary hrtimer interrupt for deadline control, but
not that much without it.

I just wonder what you think.

> 
> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> ---
>  kernel/sched/deadline.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 27737f3..cb43ce9 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -498,8 +498,9 @@ static void update_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);
>  
> -	if (dl_time_before(dl_se->deadline, rq_clock(rq)) ||
> -	    dl_entity_overflow(dl_se, pi_se, rq_clock(rq))) {
> +	if (dl_time_before(dl_se->deadline, rq_clock(rq)))
> +		replenish_dl_entity(dl_se, pi_se);
> +	else if (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;
>  	}
> @@ -621,13 +622,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>  	 *         __dequeue_task_dl()
>  	 *     prev->on_rq = 0;
>  	 *
> -	 * We can be both throttled and !queued. Replenish the counter
> -	 * but do not enqueue -- wait for our wakeup to do that.
> +	 * We can be both throttled and !queued. Wait for our wakeup to
> +	 * replenish runtime and enqueue p.
>  	 */
> -	if (!task_on_rq_queued(p)) {
> -		replenish_dl_entity(dl_se, dl_se);
> +	if (!task_on_rq_queued(p))
>  		goto unlock;
> -	}
>  
>  #ifdef CONFIG_SMP
>  	if (unlikely(!rq->online)) {
> -- 
> 1.9.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] sched/deadline: Change the time to replenish runtime for sleep tasks
  2017-02-23  6:14 [PATCH v3] sched/deadline: Change the time to replenish runtime for sleep tasks Byungchul Park
  2017-02-23 10:11 ` Byungchul Park
@ 2017-02-28 11:35 ` Juri Lelli
  2017-02-28 13:09   ` Byungchul Park
  1 sibling, 1 reply; 8+ messages in thread
From: Juri Lelli @ 2017-02-28 11:35 UTC (permalink / raw)
  To: Byungchul Park
  Cc: peterz, mingo, linux-kernel, juri.lelli, rostedt, kernel-team

Hi,

On 23/02/17 15:14, Byungchul Park wrote:
> Let's consider the following example.
> 
> timeline : o...................o.........o.......o..o
>            ^                   ^         ^       ^  ^
>            |                   |         |       |  |
>        start                   |         |       |  |
>                 original runtime         |       |  |
>                      sleep with (-)runtime       |  |
>                                  original deadline  |
>                                               wake up
> 
> When this task is woken up, a negative runtime should be considered,
> which means that the task should get penalized when assigning runtime,
> becasue it already spent more than expected. Current code handles this
> by replenishing a runtime in hrtimer callback for deadline. But this
> approach has room for improvement:
> 
>    It will be replenished twice unnecessarily if the task sleeps for
>    long time so that the deadline, assigned in the hrtimer callback,
>    also passed. In other words, one happens in the callback and the
>    other happens in update_dl_entiry() when waking it up.
> 
> So force to replenish it for sleep tasks when waking it up.
> 
> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> ---
>  kernel/sched/deadline.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 27737f3..cb43ce9 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -498,8 +498,9 @@ static void update_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);
>  
> -	if (dl_time_before(dl_se->deadline, rq_clock(rq)) ||
> -	    dl_entity_overflow(dl_se, pi_se, rq_clock(rq))) {
> +	if (dl_time_before(dl_se->deadline, rq_clock(rq)))
> +		replenish_dl_entity(dl_se, pi_se);
> +	else if (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;
>  	}
> @@ -621,13 +622,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>  	 *         __dequeue_task_dl()
>  	 *     prev->on_rq = 0;
>  	 *
> -	 * We can be both throttled and !queued. Replenish the counter
> -	 * but do not enqueue -- wait for our wakeup to do that.
> +	 * We can be both throttled and !queued. Wait for our wakeup to
> +	 * replenish runtime and enqueue p.
>  	 */
> -	if (!task_on_rq_queued(p)) {
> -		replenish_dl_entity(dl_se, dl_se);

Hasn't this patch the same problem we discussed a couple of weeks ago?

https://marc.info/?l=linux-kernel&m=148699950802995

Thanks,

- Juri

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] sched/deadline: Change the time to replenish runtime for sleep tasks
  2017-02-28 11:35 ` Juri Lelli
@ 2017-02-28 13:09   ` Byungchul Park
  2017-02-28 17:47     ` Juri Lelli
  0 siblings, 1 reply; 8+ messages in thread
From: Byungchul Park @ 2017-02-28 13:09 UTC (permalink / raw)
  To: Juri Lelli; +Cc: peterz, mingo, linux-kernel, juri.lelli, rostedt, kernel-team

On Tue, Feb 28, 2017 at 11:35:15AM +0000, Juri Lelli wrote:
> Hi,
> 
> On 23/02/17 15:14, Byungchul Park wrote:
> > Let's consider the following example.
> > 
> > timeline : o...................o.........o.......o..o
> >            ^                   ^         ^       ^  ^
> >            |                   |         |       |  |
> >        start                   |         |       |  |
> >                 original runtime         |       |  |
> >                      sleep with (-)runtime       |  |
> >                                  original deadline  |
> >                                               wake up
> > 
> > When this task is woken up, a negative runtime should be considered,
> > which means that the task should get penalized when assigning runtime,
> > becasue it already spent more than expected. Current code handles this
> > by replenishing a runtime in hrtimer callback for deadline. But this
> > approach has room for improvement:
> > 
> >    It will be replenished twice unnecessarily if the task sleeps for
> >    long time so that the deadline, assigned in the hrtimer callback,
> >    also passed. In other words, one happens in the callback and the
> >    other happens in update_dl_entiry() when waking it up.
> > 
> > So force to replenish it for sleep tasks when waking it up.
> > 
> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > ---
> >  kernel/sched/deadline.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index 27737f3..cb43ce9 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -498,8 +498,9 @@ static void update_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);
> >  
> > -	if (dl_time_before(dl_se->deadline, rq_clock(rq)) ||
> > -	    dl_entity_overflow(dl_se, pi_se, rq_clock(rq))) {
> > +	if (dl_time_before(dl_se->deadline, rq_clock(rq)))
> > +		replenish_dl_entity(dl_se, pi_se);
> > +	else if (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;
> >  	}
> > @@ -621,13 +622,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
> >  	 *         __dequeue_task_dl()
> >  	 *     prev->on_rq = 0;
> >  	 *
> > -	 * We can be both throttled and !queued. Replenish the counter
> > -	 * but do not enqueue -- wait for our wakeup to do that.
> > +	 * We can be both throttled and !queued. Wait for our wakeup to
> > +	 * replenish runtime and enqueue p.
> >  	 */
> > -	if (!task_on_rq_queued(p)) {
> > -		replenish_dl_entity(dl_se, dl_se);
> 
> Hasn't this patch the same problem we discussed a couple of weeks ago?

No. This patch solves the problem by calling replenish_dl_entity() when
a dl task is woken up.

The problem was that it cannot consider negative runtime if we replenish
the task when it's woken up. So I made replenish_dl_entity() called even
on wake-up path, instead of simple assignment.

IMHO, this patch avoids double-replenishing properly, but adds additional
condition on wake-up path to acheive it. To be honest, I don't think it's
worth as I expected.

Thank you,
Byungchul

> 
> https://marc.info/?l=linux-kernel&m=148699950802995
> 
> Thanks,
> 
> - Juri

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] sched/deadline: Change the time to replenish runtime for sleep tasks
  2017-02-28 13:09   ` Byungchul Park
@ 2017-02-28 17:47     ` Juri Lelli
  2017-03-01  3:37       ` Byungchul Park
  0 siblings, 1 reply; 8+ messages in thread
From: Juri Lelli @ 2017-02-28 17:47 UTC (permalink / raw)
  To: Byungchul Park
  Cc: peterz, mingo, linux-kernel, juri.lelli, rostedt, kernel-team

On 28/02/17 22:09, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 11:35:15AM +0000, Juri Lelli wrote:
> > Hi,
> > 
> > On 23/02/17 15:14, Byungchul Park wrote:
> > > Let's consider the following example.
> > > 
> > > timeline : o...................o.........o.......o..o
> > >            ^                   ^         ^       ^  ^
> > >            |                   |         |       |  |
> > >        start                   |         |       |  |
> > >                 original runtime         |       |  |
> > >                      sleep with (-)runtime       |  |
> > >                                  original deadline  |
> > >                                               wake up
> > > 
> > > When this task is woken up, a negative runtime should be considered,
> > > which means that the task should get penalized when assigning runtime,
> > > becasue it already spent more than expected. Current code handles this
> > > by replenishing a runtime in hrtimer callback for deadline. But this
> > > approach has room for improvement:
> > > 
> > >    It will be replenished twice unnecessarily if the task sleeps for
> > >    long time so that the deadline, assigned in the hrtimer callback,
> > >    also passed. In other words, one happens in the callback and the
> > >    other happens in update_dl_entiry() when waking it up.
> > > 
> > > So force to replenish it for sleep tasks when waking it up.
> > > 
> > > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > > ---
> > >  kernel/sched/deadline.c | 13 ++++++-------
> > >  1 file changed, 6 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > > index 27737f3..cb43ce9 100644
> > > --- a/kernel/sched/deadline.c
> > > +++ b/kernel/sched/deadline.c
> > > @@ -498,8 +498,9 @@ static void update_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);
> > >  
> > > -	if (dl_time_before(dl_se->deadline, rq_clock(rq)) ||
> > > -	    dl_entity_overflow(dl_se, pi_se, rq_clock(rq))) {
> > > +	if (dl_time_before(dl_se->deadline, rq_clock(rq)))
> > > +		replenish_dl_entity(dl_se, pi_se);
> > > +	else if (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;
> > >  	}
> > > @@ -621,13 +622,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
> > >  	 *         __dequeue_task_dl()
> > >  	 *     prev->on_rq = 0;
> > >  	 *
> > > -	 * We can be both throttled and !queued. Replenish the counter
> > > -	 * but do not enqueue -- wait for our wakeup to do that.
> > > +	 * We can be both throttled and !queued. Wait for our wakeup to
> > > +	 * replenish runtime and enqueue p.
> > >  	 */
> > > -	if (!task_on_rq_queued(p)) {
> > > -		replenish_dl_entity(dl_se, dl_se);
> > 
> > Hasn't this patch the same problem we discussed a couple of weeks ago?
> 
> No. This patch solves the problem by calling replenish_dl_entity() when
> a dl task is woken up.
> 

So, if the task was throttled in the "going to sleep" path we set the
replenishment timer to fire at your "original deadline" instant of time
above. Now, 3 things can happen I guess:

 - task wakes up before the replenishment timer ("original deadline")
   -> it is throttled, so we do nothing
 
 - task wakes up after the replenishment timer
   -> we replenished it in the timer callback (which considers negative
      runtime from previous execution)
      + deadline should be in the future
      + dl_entity shouldn't overflow
      + we don't touch its parameters, but we simply enqueue it back on dl_rq

 - task wakes up even after the deadline it has got from previous
   replenishment expired
   -> we assign to him completely new parameters, but since he didn't
      use the previous runtime at all, this should be fine I guess

What am I still missing? :)

> The problem was that it cannot consider negative runtime if we replenish
> the task when it's woken up. So I made replenish_dl_entity() called even
> on wake-up path, instead of simple assignment.
> 
> IMHO, this patch avoids double-replenishing properly, but adds additional
> condition on wake-up path to acheive it. To be honest, I don't think it's
> worth as I expected.
> 
> Thank you,
> Byungchul
> 
> > 
> > https://marc.info/?l=linux-kernel&m=148699950802995
> > 
> > Thanks,
> > 
> > - Juri

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] sched/deadline: Change the time to replenish runtime for sleep tasks
  2017-02-28 17:47     ` Juri Lelli
@ 2017-03-01  3:37       ` Byungchul Park
  2017-03-01  9:59         ` Juri Lelli
  0 siblings, 1 reply; 8+ messages in thread
From: Byungchul Park @ 2017-03-01  3:37 UTC (permalink / raw)
  To: Juri Lelli; +Cc: peterz, mingo, linux-kernel, juri.lelli, rostedt, kernel-team

On Tue, Feb 28, 2017 at 05:47:53PM +0000, Juri Lelli wrote:
> > > > Let's consider the following example.
> > > > 
> > > > timeline : o...................o.........o.......o..o
> > > >            ^                   ^         ^       ^  ^
> > > >            |                   |         |       |  |
> > > >        start                   |         |       |  |
> > > >                 original runtime         |       |  |
> > > >                      sleep with (-)runtime       |  |
> > > >                                  original deadline  |
> > > >                                               wake up
> > > > 
> > > > When this task is woken up, a negative runtime should be considered,
> > > > which means that the task should get penalized when assigning runtime,
> > > > becasue it already spent more than expected. Current code handles this
> > > > by replenishing a runtime in hrtimer callback for deadline. But this
> > > > approach has room for improvement:
> > > > 
> > > >    It will be replenished twice unnecessarily if the task sleeps for
> > > >    long time so that the deadline, assigned in the hrtimer callback,
> > > >    also passed. In other words, one happens in the callback and the
> > > >    other happens in update_dl_entiry() when waking it up.
> > > > 
> > > > So force to replenish it for sleep tasks when waking it up.
> > > > 
> > > > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > > > ---

...

> So, if the task was throttled in the "going to sleep" path we set the
> replenishment timer to fire at your "original deadline" instant of time

Hi,

Precisely speaking, we set the timer if the task was throttled, no
matter whether it's in the 'going to sleep' path or not. IWO, the timer
is set even in the path. And I want to say it's unnecessary.

> above. Now, 3 things can happen I guess:
> 
>  - task wakes up before the replenishment timer ("original deadline")
>    -> it is throttled, so we do nothing
>  
>  - task wakes up after the replenishment timer
>    -> we replenished it in the timer callback (which considers negative
>       runtime from previous execution)
>       + deadline should be in the future
>       + dl_entity shouldn't overflow
>       + we don't touch its parameters, but we simply enqueue it back on dl_rq
> 
>  - task wakes up even after the deadline it has got from previous
>    replenishment expired
>    -> we assign to him completely new parameters, but since he didn't
>       use the previous runtime at all, this should be fine I guess

Exactly same as what I thought. I agree that current code works properly.
However, the code has room for improvement because tasks being throttled
in 'going to sleep' path do not need to set additional timer.

To be honest, I also tried to remove setting timer in the case, but it
makes code a bit cluttered because I should handle the your first case
above, that means I should add a proper timer on the wake-up path if
necessary.

The try would be worthy if avoiding unnecessary timer is more important
than making code cluttered. But I did not include the try since I'm not
sure. What do you think about that? Which one is more important between
avoiding unnecessary timer and avoiding making code cluttered?

Thank you,
Byungchul

> 
> What am I still missing? :)
> 
> > The problem was that it cannot consider negative runtime if we replenish
> > the task when it's woken up. So I made replenish_dl_entity() called even
> > on wake-up path, instead of simple assignment.
> > 
> > IMHO, this patch avoids double-replenishing properly, but adds additional
> > condition on wake-up path to acheive it. To be honest, I don't think it's
> > worth as I expected.
> > 
> > Thank you,
> > Byungchul
> > 
> > > 
> > > https://marc.info/?l=linux-kernel&m=148699950802995
> > > 
> > > Thanks,
> > > 
> > > - Juri

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] sched/deadline: Change the time to replenish runtime for sleep tasks
  2017-03-01  3:37       ` Byungchul Park
@ 2017-03-01  9:59         ` Juri Lelli
  2017-03-01 12:09           ` Byungchul Park
  0 siblings, 1 reply; 8+ messages in thread
From: Juri Lelli @ 2017-03-01  9:59 UTC (permalink / raw)
  To: Byungchul Park
  Cc: peterz, mingo, linux-kernel, juri.lelli, rostedt, kernel-team

On 01/03/17 12:37, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 05:47:53PM +0000, Juri Lelli wrote:
> > > > > Let's consider the following example.
> > > > > 
> > > > > timeline : o...................o.........o.......o..o
> > > > >            ^                   ^         ^       ^  ^
> > > > >            |                   |         |       |  |
> > > > >        start                   |         |       |  |
> > > > >                 original runtime         |       |  |
> > > > >                      sleep with (-)runtime       |  |
> > > > >                                  original deadline  |
> > > > >                                               wake up
> > > > > 
> > > > > When this task is woken up, a negative runtime should be considered,
> > > > > which means that the task should get penalized when assigning runtime,
> > > > > becasue it already spent more than expected. Current code handles this
> > > > > by replenishing a runtime in hrtimer callback for deadline. But this
> > > > > approach has room for improvement:
> > > > > 
> > > > >    It will be replenished twice unnecessarily if the task sleeps for
> > > > >    long time so that the deadline, assigned in the hrtimer callback,
> > > > >    also passed. In other words, one happens in the callback and the
> > > > >    other happens in update_dl_entiry() when waking it up.
> > > > > 
> > > > > So force to replenish it for sleep tasks when waking it up.
> > > > > 
> > > > > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > > > > ---
> 
> ...
> 
> > So, if the task was throttled in the "going to sleep" path we set the
> > replenishment timer to fire at your "original deadline" instant of time
> 
> Hi,
> 
> Precisely speaking, we set the timer if the task was throttled, no
> matter whether it's in the 'going to sleep' path or not. IWO, the timer
> is set even in the path. And I want to say it's unnecessary.
> 
> > above. Now, 3 things can happen I guess:
> > 
> >  - task wakes up before the replenishment timer ("original deadline")
> >    -> it is throttled, so we do nothing
> >  
> >  - task wakes up after the replenishment timer
> >    -> we replenished it in the timer callback (which considers negative
> >       runtime from previous execution)
> >       + deadline should be in the future
> >       + dl_entity shouldn't overflow
> >       + we don't touch its parameters, but we simply enqueue it back on dl_rq
> > 
> >  - task wakes up even after the deadline it has got from previous
> >    replenishment expired
> >    -> we assign to him completely new parameters, but since he didn't
> >       use the previous runtime at all, this should be fine I guess
> 
> Exactly same as what I thought. I agree that current code works properly.
> However, the code has room for improvement because tasks being throttled
> in 'going to sleep' path do not need to set additional timer.
> 
> To be honest, I also tried to remove setting timer in the case, but it
> makes code a bit cluttered because I should handle the your first case
> above, that means I should add a proper timer on the wake-up path if
> necessary.
> 
> The try would be worthy if avoiding unnecessary timer is more important
> than making code cluttered. But I did not include the try since I'm not
> sure. What do you think about that? Which one is more important between
> avoiding unnecessary timer and avoiding making code cluttered?
> 

I'd err on the side of clearness and safety (where I assume the current
code is safer only because it has been tested for a while :). However,
if you can prove in a reproducible manner that the changes you are
proposing make overhead way smaller, we might still consider them.

Anyway, IMVHO, there are still lot of known issues and missing features
that deserve time; so, if your have some time to look at DEADLINE stuff,
I'd kindly point you at

https://github.com/jlelli/sched-deadline/wiki/TODOs

You would be very welcome if you think you can pick something of what
above up, and we should synchronize in that case, as several people are
already working on some of the aforementioned bits.

Thanks,

- Juri

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] sched/deadline: Change the time to replenish runtime for sleep tasks
  2017-03-01  9:59         ` Juri Lelli
@ 2017-03-01 12:09           ` Byungchul Park
  0 siblings, 0 replies; 8+ messages in thread
From: Byungchul Park @ 2017-03-01 12:09 UTC (permalink / raw)
  To: Juri Lelli; +Cc: peterz, mingo, linux-kernel, juri.lelli, rostedt, kernel-team

On Wed, Mar 01, 2017 at 09:59:33AM +0000, Juri Lelli wrote:
> I'd err on the side of clearness and safety (where I assume the current
> code is safer only because it has been tested for a while :). However,
> if you can prove in a reproducible manner that the changes you are
> proposing make overhead way smaller, we might still consider them.

Then.. I will check how much it reduces the overhead.

> Anyway, IMVHO, there are still lot of known issues and missing features
> that deserve time; so, if your have some time to look at DEADLINE stuff,
> I'd kindly point you at
> 
> https://github.com/jlelli/sched-deadline/wiki/TODOs
> 
> You would be very welcome if you think you can pick something of what
> above up, and we should synchronize in that case, as several people are
> already working on some of the aforementioned bits.

Why not? I would be very honored if I can work together.

To be honest with you, I'm currently developing another patch set about
task migration in dl/rt. I will let you know after making it clear.

Unfortunately I have little time until 19th Mar, so I can do my work and
your TODOs after the day. I will continue the works after the day. :)

Thank you very much,
Byungchul

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-03-01 12:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-23  6:14 [PATCH v3] sched/deadline: Change the time to replenish runtime for sleep tasks Byungchul Park
2017-02-23 10:11 ` Byungchul Park
2017-02-28 11:35 ` Juri Lelli
2017-02-28 13:09   ` Byungchul Park
2017-02-28 17:47     ` Juri Lelli
2017-03-01  3:37       ` Byungchul Park
2017-03-01  9:59         ` Juri Lelli
2017-03-01 12:09           ` Byungchul Park

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.