Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted bandwidth

[luca abeni]

 Hi all,

On Wed, 10 Feb 2016 11:58:12 +0000
Juri Lelli <juri.lelli@xxxxxxx> wrote:
[...]
> > >  	} else if (!dl_policy(policy) && task_has_dl_policy(p)) {
> > >  		__dl_clear(dl_b, p->dl.dl_bw);
> > > +		__dl_sub_ac(task_rq(p), p->dl.dl_bw);
> > 
> > Instead of adding __dl_add_ac() and __dl_sub_ac) calls here, maybe
> > they can be added in switched_to_dl() and switched_from_dl()?
> > 
> 
> That might work too yes. I think there is value if we are able to move
> all __dl_{add,sub}_ac calls in deadline.c. Actually, we should
> probably move __dl_{add,clear} there as well, so that changes to rq
> and root_domain happen at the same time.
> 
> > I'll test this idea locally, and I'll send an updated patch if it
> > works.
> > 
> 
> Thanks! Will wait for it :).

I know there are still open issues with select_fallback_rq() and
similar stuff, but as promised I worked on moving the __dl_*_ac() calls
from core.c to deadline.c (which is orthogonal respect to the
select_fallback_rq() thing). I post these patches so that people can
see how the code would look like after moving __dl_*_ac() calls to
deadline.c and can provide some comments; the patches are not submitted
for inclusion (yet).


So, the first attached patch (to be applied over Juri's patch) just
moves two __dl_sub_ac() and __dl_add_ac() invocations from
dl_overflow() to deadline.c, using the switched_from_dl() and
switched_to_dl() methods. This should cover the cases of tasks moving
from SCHED_{OTHER,RT} to SCHED_DEADLINE and vice-versa. The case in
which the deadline parameters of a task are changed still needs some
__dl_* calls in dl_overflow().

These calls are moved to deadline.c by the second attached patch, which
uses prio_changed_dl()... Here, prio_changed_dl() needs to know the
"old" task utilization, while it is given  the "old task
priority" (which, for a deadline task is equal to the current
priority)... So, I changed the meaning of the third parameter of
prio_changed_dl(), from "old priority" to "old utilization".
I know that changing the meaning of a parameter between different
scheduling classes might be a bad idea... But the "prio_changed" method
seems to be designed for priority-based schedulers only, and does not
provide good information to the SCHED_DEADLINE scheduling class... The
alternative is to change the prototype of the function, adding an
"oldbw" (or, better, "oldperiod" and "oldruntime") parameter.
Let me know what you think about this.

Notice that the second patch also contains two hunks that can be
extracted from it (if needed):
1) remove the call to switched_to_dl() from prio_changed_dl(): this
   call seems to be useless
2) change __sched_setscheduler() to invoke prio_changed for deadline
   tasks changing their parameters. Currently, this method is not
   called (because when changing parameters deadline tasks do not
   change their priority, so new_effective_prio == oldprio). I suspect
   calling prio_changed is the correct behaviour; of course, this can
   be done in different ways, let me know your opinion.

I tested these two patches in various ways, and I do not see
regressions respect to Juri's patch (but I expect issues with
select_fallback_rq()... BTW, if anyone can provide some testcases for
it, I can try to fix that case too).

Finally, when playing with switched_to_dl() I realized that it can be
used to remove the dl_new field. So, I wrote patch 0003 (attached),
which seems to be working correctly, but I am probably missing some
important tests. Let me know what you think about it: I think it might
be a nice simplification of the code, but as usual I might be missing
something :)



			Thanks,
				Luca