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 <[email protected]>
> > > > ---
...
> 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
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 <[email protected]>
> > > > > ---
>
> ...
>
> > 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
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