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

Groups > linux.kernel > #1578327 > unrolled thread

[PATCH] sched/deadline: Remove redundant code replenishing runtime

Back to article view | Back to linux.kernel

Contents

  [PATCH] sched/deadline: Remove redundant code replenishing runtime Byungchul Park <byungchul.park@lge.com> - 2017-02-10 10:30 +0100
    Re: [PATCH] sched/deadline: Remove redundant code replenishing  runtime Juri Lelli <juri.lelli@arm.com> - 2017-02-10 14:50 +0100
      Re: [PATCH] sched/deadline: Remove redundant code replenishing  runtime Byungchul Park <byungchul.park@lge.com> - 2017-02-13 03:40 +0100
        Re: [PATCH] sched/deadline: Remove redundant code replenishing  runtime Byungchul Park <byungchul.park@lge.com> - 2017-02-13 05:40 +0100
          Re: [PATCH] sched/deadline: Remove redundant code replenishing  runtime Juri Lelli <juri.lelli@arm.com> - 2017-02-13 16:30 +0100

#1578327 — [PATCH] sched/deadline: Remove redundant code replenishing runtime

For a task passing its deadline while !rq, it will be replenished
in the following path because dl_se->deadline < rq_lock.

   enqueue_dl_entity(ENQUEUE_WAKEUP)
      update_dl_entity

Therefore, code replenishing it in the timer callback in the case is
unnecessary. This is not for enhancing performance but just for removing
a redundant code.

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

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 27737f3..9c77696 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -624,10 +624,8 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
 	 * We can be both throttled and !queued. Replenish the counter
 	 * but do not enqueue -- wait for our wakeup to do that.
 	 */
-	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

[toc] | [next] | [standalone]

#1578460 — Re: [PATCH] sched/deadline: Remove redundant code replenishing runtime

Hi,

On 10/02/17 18:11, Byungchul Park wrote:
> For a task passing its deadline while !rq, it will be replenished
> in the following path because dl_se->deadline < rq_lock.
> 
>    enqueue_dl_entity(ENQUEUE_WAKEUP)
>       update_dl_entity
> 
> Therefore, code replenishing it in the timer callback in the case is
> unnecessary. This is not for enhancing performance but just for removing
> a redundant code.
> 
> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> ---
>  kernel/sched/deadline.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 27737f3..9c77696 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -624,10 +624,8 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>  	 * We can be both throttled and !queued. Replenish the counter
>  	 * but do not enqueue -- wait for our wakeup to do that.
>  	 */
> -	if (!task_on_rq_queued(p)) {
> -		replenish_dl_entity(dl_se, dl_se);

I think we actually want to replenish and set the next deadline at this
point of time, not the one that we get when the task will eventually wake up.

Best,

- Juri

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

#1579359 — Re: [PATCH] sched/deadline: Remove redundant code replenishing runtime

On Fri, Feb 10, 2017 at 01:39:33PM +0000, Juri Lelli wrote:
> Hi,
> 
> On 10/02/17 18:11, Byungchul Park wrote:
> > For a task passing its deadline while !rq, it will be replenished
> > in the following path because dl_se->deadline < rq_lock.
> > 
> >    enqueue_dl_entity(ENQUEUE_WAKEUP)
> >       update_dl_entity
> > 
> > Therefore, code replenishing it in the timer callback in the case is
> > unnecessary. This is not for enhancing performance but just for removing
> > a redundant code.
> > 
> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > ---
> >  kernel/sched/deadline.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index 27737f3..9c77696 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -624,10 +624,8 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
> >  	 * We can be both throttled and !queued. Replenish the counter
> >  	 * but do not enqueue -- wait for our wakeup to do that.
> >  	 */
> > -	if (!task_on_rq_queued(p)) {
> > -		replenish_dl_entity(dl_se, dl_se);
> 
> I think we actually want to replenish and set the next deadline at this
> point of time, not the one that we get when the task will eventually wake up.

Hello juri,

But I wonder if it's meaningful to set a next deadline for a 'sleeping
task', which, rather, could be worse because its bandwidth might be
distorted at the time it's woken up.

IMHO, it's neat to set its deadline and runtime when being woken up, in
the case already passed its deadline. Am I wrong?

Thank you,
Byungchul

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

#1579400 — Re: [PATCH] sched/deadline: Remove redundant code replenishing runtime

On Mon, Feb 13, 2017 at 11:30:09AM +0900, Byungchul Park wrote:
> On Fri, Feb 10, 2017 at 01:39:33PM +0000, Juri Lelli wrote:
> > Hi,
> > 
> > On 10/02/17 18:11, Byungchul Park wrote:
> > > For a task passing its deadline while !rq, it will be replenished
> > > in the following path because dl_se->deadline < rq_lock.
> > > 
> > >    enqueue_dl_entity(ENQUEUE_WAKEUP)
> > >       update_dl_entity
> > > 
> > > Therefore, code replenishing it in the timer callback in the case is
> > > unnecessary. This is not for enhancing performance but just for removing
> > > a redundant code.
> > > 
> > > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > > ---
> > >  kernel/sched/deadline.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > > index 27737f3..9c77696 100644
> > > --- a/kernel/sched/deadline.c
> > > +++ b/kernel/sched/deadline.c
> > > @@ -624,10 +624,8 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
> > >  	 * We can be both throttled and !queued. Replenish the counter
> > >  	 * but do not enqueue -- wait for our wakeup to do that.
> > >  	 */
> > > -	if (!task_on_rq_queued(p)) {
> > > -		replenish_dl_entity(dl_se, dl_se);
> > 
> > I think we actually want to replenish and set the next deadline at this
> > point of time, not the one that we get when the task will eventually wake up.
> 
> Hello juri,
> 
> But I wonder if it's meaningful to set a next deadline for a 'sleeping
> task', which, rather, could be worse because its bandwidth might be
> distorted at the time it's woken up.
> 
> IMHO, it's neat to set its deadline and runtime when being woken up, in
> the case already passed its deadline. Am I wrong?

And I found that dl_entity_overflow() returns true and replenishes the
task unconditionally in update_dl_entity() again when the task is woken
up, because 'runtime / (deadline - t) > dl_runtime / dl_period' is true.

In other words, replenishing the sleeping task in timer callback is
totally unnecessary and redundant work.

> 
> Thank you,
> Byungchul

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

#1579853 — Re: [PATCH] sched/deadline: Remove redundant code replenishing runtime

[+Luca]

On 13/02/17 13:29, Byungchul Park wrote:
> On Mon, Feb 13, 2017 at 11:30:09AM +0900, Byungchul Park wrote:
> > On Fri, Feb 10, 2017 at 01:39:33PM +0000, Juri Lelli wrote:
> > > Hi,
> > > 
> > > On 10/02/17 18:11, Byungchul Park wrote:
> > > > For a task passing its deadline while !rq, it will be replenished
> > > > in the following path because dl_se->deadline < rq_lock.
> > > > 
> > > >    enqueue_dl_entity(ENQUEUE_WAKEUP)
> > > >       update_dl_entity
> > > > 
> > > > Therefore, code replenishing it in the timer callback in the case is
> > > > unnecessary. This is not for enhancing performance but just for removing
> > > > a redundant code.
> > > > 
> > > > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > > > ---
> > > >  kernel/sched/deadline.c | 4 +---
> > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > 
> > > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > > > index 27737f3..9c77696 100644
> > > > --- a/kernel/sched/deadline.c
> > > > +++ b/kernel/sched/deadline.c
> > > > @@ -624,10 +624,8 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
> > > >  	 * We can be both throttled and !queued. Replenish the counter
> > > >  	 * but do not enqueue -- wait for our wakeup to do that.
> > > >  	 */
> > > > -	if (!task_on_rq_queued(p)) {
> > > > -		replenish_dl_entity(dl_se, dl_se);
> > > 
> > > I think we actually want to replenish and set the next deadline at this
> > > point of time, not the one that we get when the task will eventually wake up.
> > 
> > Hello juri,
> > 
> > But I wonder if it's meaningful to set a next deadline for a 'sleeping
> > task', which, rather, could be worse because its bandwidth might be
> > distorted at the time it's woken up.
> > 

What you mean by 'distorted'. AFAIU, we just want to replenish when
needed. The instant of time when the task will eventually wake up it is
something we cannot rely upon, and could introduce errors.

IIUC, your situation looks like the below

   oooo|-------------------vxxx^ooo
       |                   |   |
       |                   |   |
    sleep/throttle         |   |
                     r. timer  |
   		           wakeup

The task gets throttled while going to sleep, when the replenishment
timer fires you are proposing we do nothing and we actually replenishing
using the wakeup rq_clock() as reference. My worry is that, by doing so,
we make the task potentially loose some of its bandwidth, as we will
have lost some time (the 3 x-es in the diagram above) when calculating
its next dynamic deadline. 

> > IMHO, it's neat to set its deadline and runtime when being woken up, in
> > the case already passed its deadline. Am I wrong?
> 
> And I found that dl_entity_overflow() returns true and replenishes the
> task unconditionally in update_dl_entity() again when the task is woken
> up, because 'runtime / (deadline - t) > dl_runtime / dl_period' is true.
> 

Why 'unconditionally'? It will postpone and replenish if the task is
going to overflow, if not, it will keep its runtime and deadline we set
when the replenishment timer fired.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web