Date: Fri, 18 Nov 2016 16:56:47 +0100
From: luca abeni <luca.abeni@unitn.it>
To: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@arm.com>,
        Claudio Scordino <claudio@evidence.eu.com>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [RFC v3 2/6] Improve the tracking of active utilisation
Message-ID: <20161118165647.46f8c867@sweethome>
In-Reply-To: <20161118153615.GM3142@twins.programming.kicks-ass.net>
References: <1477317998-7487-1-git-send-email-luca.abeni@unitn.it>
        <1477317998-7487-3-git-send-email-luca.abeni@unitn.it>
        <20161118153615.GM3142@twins.programming.kicks-ass.net>
Organization: university of trento
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2153
Lines: 56

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