Re: [PATCH v3] sched/deadline: Change the time to replenish runtime for sleep tasks

[Juri Lelli]

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@xxxxxxx>
> > > > > ---
> 
> ...
> 
> > 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