Date: Fri, 8 Apr 2016 14:50:55 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: xlpang@redhat.com, linux-kernel@vger.kernel.org,
        Juri Lelli <juri.lelli@arm.com>, Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH] sched/deadline/rtmutex: Fix a PI crash for deadline
 tasks
Message-ID: <20160408145055.13c98a75@gandalf.local.home>
In-Reply-To: <20160408173835.GC1087@worktop>
References: <1459508418-25577-1-git-send-email-xlpang@redhat.com>
	<20160401113827.GQ3430@twins.programming.kicks-ass.net>
	<56FE685E.6080001@redhat.com>
	<19912883-8AB1-4DFD-A0E1-F23057785243@infradead.org>
	<56FE78E0.5060504@redhat.com>
	<20160401215143.GB2906@worktop>
	<57037974.1020002@redhat.com>
	<20160405091954.GI3448@twins.programming.kicks-ass.net>
	<20160405092954.GC24771@twins.programming.kicks-ass.net>
	<20160408122510.15978179@gandalf.local.home>
	<20160408173835.GC1087@worktop>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2095
Lines: 50

On Fri, 8 Apr 2016 19:38:35 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Apr 08, 2016 at 12:25:10PM -0400, Steven Rostedt wrote:
> 
> > So the preempt_disable() is to allow us to set current back to its
> > normal priority first before waking up the other task because we don't
> > want two tasks at the same priority?  
> 
> > What's the point of swapping deboost and the wake up again?  
> 
> In the context of this patch, it ensures the new pi_task pointer points
> to something that exists -- this is a rather useful property for a
> pointer to have.

It's not clear to what would make the new pi_task pointer object no
longer exist from this patch. I take it that waking up the wake_q, will
cause something to change in the code of rt_mutex_adjust_prio(current).
If so, it should probably be stated in a comment, because nothing is
obvious here.

> 
> It furthermore guarantees that it points to a blocked task, another
> useful property.

I would think that the slowfn() would have removed anything to do with
what's on the wake_q removed from current. What task on what pointer.
I'm only looking at this current patch, not anything to do with the
original patch of this thread. That is, just the swap of waking up
wake_q and calling rt_mutex_adjust_prio().

> 
> > Maybe I'm missing something, what exactly do you mean by "same state"?  
> 
> So the whole point of boosting is to donate the waiters eligibility to
> run to the lock owner. Semantically it is very poor to have two tasks
> run based on this one eligibility.
> 
> And for pure priority inheritance it doesn't go further than that.
> 
> But now consider we want to implement things like bandwidth inheritance,
> where the lock owner actively consumes the runtime budget of the waiter,
> then it is very important to not have any overlap on the budget usage.

If the two tasks (current and the wakee) are on two different CPUs, the
bandwidth charge does make better sense. Again, perhaps a bit more
explanation in the comment would help, as "state" is too generic of a
term to get anything from it.

-- Steve