Messages in this thread | | | Date | Fri, 18 Nov 2016 16:56:47 +0100 | From | luca abeni <> | Subject | Re: [RFC v3 2/6] Improve the tracking of active utilisation |
| |
On Fri, 18 Nov 2016 16:36:15 +0100 Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Oct 24, 2016 at 04:06:34PM +0200, Luca Abeni wrote: > > @@ -514,7 +556,20 @@ static void update_dl_entity(struct > > sched_dl_entity *dl_se, struct dl_rq *dl_rq = dl_rq_of_se(dl_se); > > struct rq *rq = rq_of_dl_rq(dl_rq); > > > > + if (hrtimer_is_queued(&dl_se->inactive_timer)) { > > + hrtimer_try_to_cancel(&dl_se->inactive_timer); > > + WARN_ON(dl_task_of(dl_se)->nr_cpus_allowed > 1); > > Isn't that always so? That is, DL tasks cannot be but 'global', right? Well, if I understand well in general (that is, if admission control is enabled) nr_cpus_allowed is equal to the number of CPUs in the cpuset... This is generally > 1 (and in this case select_task_rq_dl() is invoked first, and tries to cancel the timer - so I think the timer cannot be queued), or can be = 1 if we do partitioned scheduling (cpusets containing only 1 CPU, or disabled admission control). If nr_cpus_allowed is 1, then select_task_rq_dl() is not invoked, so the timer can be queued. In some of my tests I used partitioned scheduling; in some other tests I disabled admission control to mix tasks with different affinities, so I made the warning conditional to the number of CPUs being > 1.
> Also, you could use the return value of hrtimer_try_to_cancel() to > determine hrtimer_is_queued() I suppose.
Ah, ok... I was under the impression that "if (hrtimer_is_queued()) hrtimer_try_to_cancel()" is less overhead than a simple "hrtimer_try_to_cancel()", but this was just an uneducated guess... I'll change the code to avoid the check on hrtimer_is_queued().
> > > + } else { > > + /* > > + * The "inactive timer" has been cancelled in > > + * select_task_rq_dl() (and the acvive utilisation > > has > > + * been decreased). So, increase the active > > utilisation. > > + * If select_task_rq_dl() could not cancel the > > timer, > > + * inactive_task_timer() will * find the task > > state as > ^^^ > superfluous '*'?
Yes, sorry... Something went wrong when I re-indented the comment :(
Thanks, Luca
| |