Date: Mon, 27 Mar 2017 09:36:28 +0200
From: Luca Abeni <luca.abeni@santannapisa.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>,
        Tommaso Cucinotta <tommaso.cucinotta@sssup.it>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Joel Fernandes <joelaf@google.com>,
        Mathieu Poirier <mathieu.poirier@linaro.org>
Subject: Re: [RFC v5 2/9] sched/deadline: improve the tracking of active
 utilization
Message-ID: <20170327093628.29725038@luca>
In-Reply-To: <20170324224715.4098dbfb@nowhere>
References: <1490327582-4376-1-git-send-email-luca.abeni@santannapisa.it>
        <1490327582-4376-3-git-send-email-luca.abeni@santannapisa.it>
        <20170324132041.6fxayfe3id4af3n5@hirez.programming.kicks-ass.net>
        <20170324224715.4098dbfb@nowhere>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2759
Lines: 61

On Fri, 24 Mar 2017 22:47:15 +0100
luca abeni <luca.abeni@santannapisa.it> wrote:
[...]
> > > +	} else {
> > > +		/*
> > > +		 * Since "dl_non_contending" is not set, the
> > > +		 * task's utilization has already been removed
> > > from
> > > +		 * active utilization (either when the task
> > > blocked,
> > > +		 * when the "inactive timer" fired).
> > > +		 * So, add it back.
> > > +		 */
> > > +		add_running_bw(dl_se->dl_bw, dl_rq);
> > > +	}
> > > +}    
> > 
> > In general I feel it would be nice to have a state diagram included
> > somewhere near these two functions. It would be nice to not have to
> > dig out the PDF every time.  
> 
> Ok... Since I am not good at ascii art, would it be ok to add a
> textual description? If yes, I'll add a comment like:
> "
> The utilization of a task is added to the runqueue's active
> utilization when the task becomes active (is enqueued in the
> runqueue), and is removed when the task becomes inactive. A task does
> not become immediately inactive when it blocks, but becomes inactive
> at the so called "0 lag time"; so, we setup the "inactive timer" to
> fire at the "0 lag time". When the "inactive timer" fires, the task
> utilization is removed from the runqueue's active utilization. If the
> task wakes up again on the same runqueue before the "0 lag time", the
> active utilization must not be changed and the "inactive timer" must
> be cancelled. If the task wakes up again on a different runqueue
> before the "0 lag time", then the task's utilization must be removed
> from the previous runqueue's active utilization and must be added to
> the new runqueue's active utilization.
> In order to avoid races between a task waking up on a runqueue while
> the "inactive timer" is running on a different CPU, the
> "dl_non_contending" flag is used to indicate that a task is not on a
> runqueue but is active (so, the flag is set when the task blocks and
> is cleared when the "inactive timer" fires or when the task  wakes
> up). "

After re-reading all of this, I realized two things:
1) The comment I added before the definition of the dl_non_contending
   flag in sched.h is confusing. I'll try to rephrase it
2) The "dl_non_contending" name might be part of the issue, here.
   It tries to map concepts from the GRUB paper to the implementation,
   so it is understandable and makes things clear (I hope) for people
   who know the paper... But it is not the best name for people not
   knowing the GRUB paper (and pretending that someone studies the
   paper to understand the code is not good :).
   So, what about "inactive_timer_armed" (or similar)? This would
   immediately clarify what the flag is about... What do you think?
   Would this renaming be useful?


			Thanks,
				Luca