Date: Fri, 8 Apr 2016 15:15:42 -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: <20160408151542.24328fd6@gandalf.local.home>
In-Reply-To: <20160408185916.GQ3448@twins.programming.kicks-ass.net>
References: <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>
	<20160408145055.13c98a75@gandalf.local.home>
	<20160408185916.GQ3448@twins.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2880
Lines: 76

On Fri, 8 Apr 2016 20:59:16 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Apr 08, 2016 at 02:50:55PM -0400, Steven Rostedt wrote:
> > 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.  
> 
> Its pretty obvious that a running task can exit :-)
> 
> But also, wake_q holds a task ref.
> 
> > > 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.  
> 
> It cannot.
> 

It cannot?

Maybe I didn't make myself clear in the question.

>From what I understand, the slowfn() modifies the task pi_list (or
rbtree, as it is today). As this is an unlock, the task being woken
(the next one to grab the lock) is removed from the previous task's pi
list.

In rt_mutex_adjust_prio(current) I see it simply grabs current's
pi_lock and calls __rt_mutex_adjust_prio(current). This calls
rt_mutex_getprio(current) which returns current's normal prio if it
doesn't have any pi waiters, or it looks at the top pi waiter on the
tasks list and returns that. Which wouldn't be the task on wake_q,
otherwise we wouldn't be deboosting in the first place.

Again, I don't see how the swap has anything to do with pointers
disappearing.

> > 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().  
> 
> This whole patch was in the context of the previous patch, as should be
> clear from the thread.

OK, so this patch is to be added to the previous patch, and everything
should be in context to that. My comment was just about this change,
not the change of the original patch.

> 
> In any case, I just realized we do not in fact provide this guarantee
> (of pointing to a blocked task) that needs a bit more work.

Looking forward to it ;-)

-- Steve