Date: Thu, 10 Nov 2016 13:45:19 +0100
From: luca abeni <luca.abeni@unitn.it>
To: Juri Lelli <juri.lelli@arm.com>
Cc: linux-kernel@vger.kernel.org, Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.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: <20161110134519.76d245c6@sweethome>
In-Reply-To: <20161110123415.GJ16920@e106622-lin>
References: <1477317998-7487-1-git-send-email-luca.abeni@unitn.it>
        <1477317998-7487-3-git-send-email-luca.abeni@unitn.it>
        <20161101164604.GB2769@ARMvm>
        <20161101224633.4e5ee0ca@utopia>
        <20161102033552.4d41c9ca@utopia>
        <20161110100428.GH16920@e106622-lin>
        <20161110115610.GI16920@e106622-lin>
        <20161110131558.7a9480b2@sweethome>
        <20161110123415.GJ16920@e106622-lin>
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: 2271
Lines: 53

On Thu, 10 Nov 2016 12:34:15 +0000
Juri Lelli <juri.lelli@arm.com> wrote:
[...]
> > Ok; I'll think about some possible solution for this race... If I do
> > not find any simple way to solve it, I'll add a "contending" flag,
> > which allows to know if the inactive timer handler already executed
> > or not.
> >   
> 
> Right, this might help.
> 
> Another thing that I was thinking of is whether we can use the return
> value of hrtimer_try_to_cancel() to decide what to do:
> 
>  - if it returns 0 it means that the callback exectuted or the timer
> was never set, so nothing to do (as in nothing to sub_running_bw from)
>  - if it returns 1 we succedeed, so we need to actively sub_running_bw
>  - if -1 we can assume that it will eventually do sub_running_bw() so
> we don't need to care explicitly
This is about what I did in the initial version of the patch, but I
found some problems... I'll try to have another look at this approach.


> Now I guess the problem is that the task can be migrated while his
> inactive_timer is set (by select_task_rq_dl or by other classes load
> balacing if setscheduled to a different class). Can't we store a back
> reference to the rq from which the inactive_timer was queued and use
> that to sub_running_bw() from? It seems that we might end up with some
> "shadow" bandwidth, say when we do a wakeup migration, but maybe this
> is something we can tolerate? Just thinking aloud. :)
I'll think about this...


> > BTW, talking about sched_dl_entity flags: I see there are three
> > different int fields "dl_throttled, "dl_boosted" and "dl_yielded";
> > any reason for doing this instead of having a "dl_flags" field and
> > setting its different bits when the entity is throttled, boosted or
> > yielded? In other words: if I need this "contending" flag, should I
> > add a new "dl_contending" field?
> >   
> 
> I think you might want to add a clean-up patch to your series (or a
> separate one) fixing the current situation, and the build on to adding
> the new flag if needed.
Ok, can do this. I just wanted to know if there is a reason for having
different "dl_*" fields instead of a "flags" field (I do not know,
maybe performance?) or if it is just an historical accident and this
can be changed.


			Thanks,
				Luca