Re: [patch V3 3/7] rtmutex: Document pi chain walk

[Lai Jiangshan <laijs@cn.fujitsu.com>]

On 06/10/2014 08:45 AM, Steven Rostedt wrote:
> On Mon, 09 Jun 2014 20:28:08 -0000
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
>> Add commentry to document the chain walk and the protection mechanisms
>> and their scope.
>>
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> ---
>>  kernel/locking/rtmutex.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 52 insertions(+)
>>
>> Index: tip/kernel/locking/rtmutex.c
>> ===================================================================
>> --- tip.orig/kernel/locking/rtmutex.c
>> +++ tip/kernel/locking/rtmutex.c
>> @@ -285,6 +285,47 @@ static inline struct rt_mutex *task_bloc
>>   * @top_task:	the current top waiter
>>   *
>>   * Returns 0 or -EDEADLK.
>> + *
>> + * Chain walk basics and protection scope
>> + *
>> + * [A] refcount on task
>> + * [B] task->pi_lock held
>> + * [C] rtmutex->lock held
> 
> A,B, C is rather meaningless, and requires constant looking back up at
> the key. Perhaps [R],[P] and [L]
> 
>  [R] refcount on task (get_task_struct)
>  [P] task->pi_lock held
>  [L] rtmutex->lock held
> 
> 
> That way we can associate R being refcount, P being pi_lock and L being
> lock. Easier to remember.
> 
> 
>> + *
>> + * call()					Protected by
> 
> "call()"?
> 
>> + *	@task					[A]
>> + *	@orig_lock if != NULL			@top_task is blocked on it
>> + *	@next_lock				Unprotected. Cannot be
>> + *						dereferenced. Only used for
>> + *						comparison.
>> + *	@orig_waiter if != NULL			@top_task is blocked on it
>> + *	@top_task				current, or in case of proxy
>> + *						locking protected by calling
>> + *						code
>> + * again:
>> + *	loop_sanity_check();
>> + * retry:
>> + *	lock(task->pi_lock);			[A] acquire [B]
>> + *	waiter = task->pi_blocked_on;		[B]
>> + *	check_exit_conditions();		[B]
>> + *	lock = waiter->lock;			[B]
>> + *	if (!try_lock(lock->wait_lock)) {	[B] try to acquire [C]
>> + *		unlock(task->pi_lock);		drop [B]
>> + *		goto retry;
>> + *	}
>> + *	check_exit_conditions();		[B] + [C]
>> + *	requeue_lock_waiter(lock, waiter);	[B] + [C]
>> + *	unlock(task->pi_lock);			drop [B]
>> + *	drop_task_ref(task);			drop [A]
> 
> Maybe just state "put_task_struct()", less abstractions.
> 
>> + *	check_exit_conditions();		[C]
>> + *	task = owner(lock);			[C]
>> + *	get_task_ref(task);			[C] acquire [A]
> 
> get_task_struct()
> 
> -- Steve
> 
>> + *	lock(task->pi_lock);			[C] acquire [B]
>> + *	requeue_pi_waiter(task, waiters(lock));	[B] + [C]
>> + *	check_exit_conditions();		[B] + [C]
>> + *	unlock(task->pi_lock);			drop [B]
>> + *	unlock(lock->wait_lock);		drop [C]
>> + *	goto again;
>>   */

There are four check_exit_conditions()s with the same name but with different locking.

I don't think it is a good a idea to copy the code to the comment of
the function description, we will need to always keep them coincident forever.

I prefer to comment them in the function body or comment them
in higher level abstraction.

>>  static int rt_mutex_adjust_prio_chain(struct task_struct *task,
>>  				      int deadlock_detect,
>> @@ -326,6 +367,12 @@ static int rt_mutex_adjust_prio_chain(st
>>  
>>  		return -EDEADLK;
>>  	}
>> +
>> +	/*
>> +	 * We are fully preemptible here and only hold the refcount on
>> +	 * @task. So everything can have changed under us since the
>> +	 * caller or our own code below (goto retry) dropped all locks.
>> +	 */
>>   retry:
>>  	/*
>>  	 * Task can not go away as we did a get_task() before !
>> @@ -383,6 +430,11 @@ static int rt_mutex_adjust_prio_chain(st
>>  	if (!detect_deadlock && waiter->prio == task->prio)
>>  		goto out_unlock_pi;
>>  
>> +	/*
>> +	 * We need to trylock here as we are holding task->pi_lock,
>> +	 * which is the reverse lock order versus the other rtmutex
>> +	 * operations.
>> +	 */
>>  	lock = waiter->lock;
>>  	if (!raw_spin_trylock(&lock->wait_lock)) {
>>  		raw_spin_unlock_irqrestore(&task->pi_lock, flags);
>>
> 
> .
>