Re: [PATCH v3 1/3] rtmutex: update rt-mutex-design

From: Steven Rostedt <rostedt@goodmis.org>
To: Alex Shi <alex.shi@linaro.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, Jonathan Corbet <corbet@lwn.net>,
	"open list:LOCKING PRIMITIVES" <linux-kernel@vger.kernel.org>,
	"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
	Sebastian Siewior <bigeasy@linutronix.de>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Juri Lelli <juri.lelli@arm.com>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v3 1/3] rtmutex: update rt-mutex-design
Date: Thu, 6 Jul 2017 09:25:23 -0400	[thread overview]
Message-ID: <20170706092523.3faba0c0@gandalf.local.home> (raw)
In-Reply-To: <033bcfeb-2dec-e11c-1bd6-faf256b0b971@linaro.org>

On Thu, 6 Jul 2017 10:39:28 +0800
Alex Shi <alex.shi@linaro.org> wrote:

> Hi Steven,
> 
> Thanks a lot for detailed review. Every suggestion were token except one need
> extra review: the 'Waking up in loop'. Is this OK or need more further change?
> 
> BTW, I didn't add you on Reviewers, since you are author already. :)

Actually, I probably should be. Comment below.

> 
> 
> Best regards
> Alex
> 
> 
> On 07/04/2017 02:49 AM, Steven Rostedt wrote:
> >> +In the first case, the task will try again to acquire the lock. If it  
> > 
> > Hmm, I know you mention it below, but it is confusing. In both cases
> > the task will try again to acquire the lock. The difference between the
> > two cases is what happens if it fails to acquire the lock.
> > 
> > This part should be rewritten.
> >   
> 
> +The task can then wake up for a couple of reasons:
> +  1) The previous lock owner released the lock, and the task now is top_waiter
> +  2) we received a signal or timeout
> 
> +In both cases, the task will try again to acquire the lock. If it
> +does, then it will take itself off the waiters tree and set itself back
> +to the TASK_RUNNING state.
> 
> +In first case, if the lock was acquired by another task before this task
> +could get the lock, then it will go back to sleep and wait to be woken again.
> 
> +The second case is only applicable for tasks that are grabbing a mutex
> +that can wake up before getting the lock, either due to a signal or
> +a timeout (i.e. rt_mutex_timed_futex_lock()). When woken, it will try to
> +take the lock again, if it succeeds, then the task will return with the
> +lock held, otherwise it will return with -EINTR if the task was woken
> +by a signal, or -ETIMEDOUT if it timed out.

This looks fine.

> 
> ....
> 
> -Reviewers:  Ingo Molnar, Thomas Gleixner, Thomas Duetsch, and Randy Dunlap
> +Reviewers:  Ingo Molnar, Thomas Gleixner, Thomas Duetsch, Randy Dunlap
> +               and Sebastian Siewior

Since the updated was not reviewed by all of the above, you should
change this to:

 Original Reviewers: Ingo Molnar, Thomas Gleixner, Thomas Deutsch, and
                     Randy Dunlap

 Update (7/6/2017) Reviewers: Steven Rostedt and Sebastian Siewior 

Did Randy make comments?

-- Steve