Re: [PATCH v5 16/29] sched/deadline: Record the root domain holding a task's bandwidth

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

Hi Juri,

> > +           if (p->dl.dl_bw_gen == dl_bw_gen) {
>
> So, this is not guarded by dl_departed_lock, but then this gets fixed in
> later patches?

Yes, but only as a side effect, and the patch should not rely on it.

dl_bw_gen changes in a rebuild, which holds cpuset_mutex. From patch 18
on, set_cpus_allowed_dl() reaches this test only from cpuset_attach(),
which holds cpuset_mutex too. At this patch, from reading, other
callers still reach it without cpuset_mutex: select_fallback_rq(), and
sched_setaffinity() with admission control off. A rebuild can then
change dl_bw_gen between the test and the __dl_sub().

The planned next version takes dl_departed_lock around the test and the
update of the record, from this patch on.

> > +    * enters the class. A runtime below about a millionth of the period
> > +    * gives a bandwidth of 0. It counts for nothing, and no move updates
>
> Ah right, quite cryptic corner case buried in this comment. :)
>
> This is the 1024ns/2s of the test. Which indeed gets dl_bw 0. Don't
> think it's a huge deal from a theory/practice pov as to actually make an
> impact we probably need quite a lot of these admitted (fork bomb style).
> But it's still weird :P. Not sure if we want to impose a min of 1 dl_bw
> for these. If it actually complicates the accounting (for no good
> returns) or not.

I agree that it is odd. I left it as it is upstream. The accounting
needs no special case for it beyond this comment: a bandwidth of 0 is
added and removed like any other. The case move_zero_bw of
deadline_bw_test covers it.

A minimum of 1 would change what sched_setattr() admits, by one
millionth of a CPU for each such task. I would rather not do that in a
series of fixes. I can send it as a separate patch afterwards if you
want it.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com