Date: Wed, 1 Mar 2017 12:37:49 +0900
From: Byungchul Park <byungchul.park@lge.com>
To: Juri Lelli <juri.lelli@arm.com>
CC: <peterz@infradead.org>, <mingo@kernel.org>,
        <linux-kernel@vger.kernel.org>, <juri.lelli@gmail.com>,
        <rostedt@goodmis.org>, <kernel-team@lge.com>
Subject: Re: [PATCH v3] sched/deadline: Change the time to replenish runtime
 for sleep tasks
Message-ID: <20170301033749.GB11663@X58A-UD3R>
References: <1487830499-1691-1-git-send-email-byungchul.park@lge.com>
 <20170228113515.GM19665@e106622-lin>
 <20170228130903.GF3817@X58A-UD3R>
 <20170228174753.GS19665@e106622-lin>
MIME-Version: 1.0
In-Reply-To: <20170228174753.GS19665@e106622-lin>
User-Agent: Mutt/1.5.21 (2010-09-15)
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3837
Lines: 93

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