Date: Fri, 18 Nov 2016 17:06:50 +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: <20161118170650.50932614@sweethome>
In-Reply-To: <20161118154748.GN3142@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>
        <20161118154748.GN3142@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: 1677
Lines: 48

On Fri, 18 Nov 2016 16:47:48 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Oct 24, 2016 at 04:06:34PM +0200, Luca Abeni wrote:
> > @@ -1074,6 +1161,14 @@ select_task_rq_dl(struct task_struct *p, int
> > cpu, int sd_flag, int flags) }
> >  	rcu_read_unlock();
> >  
> > +	rq = task_rq(p);
> > +	raw_spin_lock(&rq->lock);
> > +	if (hrtimer_active(&p->dl.inactive_timer)) {
> > +		sub_running_bw(&p->dl, &rq->dl);
> > +		hrtimer_try_to_cancel(&p->dl.inactive_timer);
> > +	}
> > +	raw_spin_unlock(&rq->lock);  
> 
> Its a bit sad having to take rq->lock here...

I think I can move the locking inside the if() (so that rq->lock is not
taken if the inactive timer is not active); apart from this, the only
solution I can think about is to modify select_task_rq_dl() not to
change the cpu if the timer is active... (I think the task will be
migrated by a following push() if needed). What do you think? Any other
solution I am not seeing?


> Also, what happens when hrtimer_try_to_cancel() fails?

This is something I am working on... My original idea was that nothing
bad happens, because the timer handler will see the task as RUNNING and
will not decrease the running bw... But this is wrong.
My new idea is to add a "dl_contending" flag in the scheduling entity,
that indicates if the running bw has already been subtracted or not.
With this, the issue should be solved (if anyone sees additional
issues, or a better solution that does not require an additional flag,
let me know).

BTW, this code also missed a put_task_struct() for the case in which
hrtimer_try_to_cancel() does not fail :(


			Thanks,
				Luca
> 
> > +
> >  out:
> >  	return cpu;
> >  }