Date:	Mon, 09 Mar 2009 11:31:27 -0700
From:	Darren Hart <dvhltc@...ibm.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	"lkml, " <linux-kernel@...r.kernel.org>,
	Steven Rostedt <srostedt@...hat.com>,
	Sripathi Kodi <sripathik@...ibm.com>,
	John Stultz <johnstul@...ux.vnet.ibm.com>
Subject: Re: [TIP][RFC 5/7] rt_mutex: add proxy lock routines

Thomas Gleixner wrote:
> On Mon, 2 Mar 2009, Darren Hart wrote:
>> /**
>> + * rt_mutex_start_proxy_lock - prepare another task to take the lock
> 
> Hmm. _start_ sounds weird. 

I thought on this for a while... but these names still seem the most 
appropriate to me, here's why:

rt_mutex - because it is
start - because this is the first half of a two part action
proxy - because it is initiated by one thread on behalf of another
lock - because we are trying to take the lock

This seems the most consistent with the naming scheme used throughout 
rtmutex.c as well.  If you have a pair of names for these two functions 
that you think would make more sense, please let me know.

> Also we do not prepare another task to take
> the lock. We either take the lock on behalf on another task or block
> that task on the lock.

Agreed:

" * rt_mutex_start_proxy_lock - Start lock acquisition for another task"

> 
>> + * @lock:		the rt_mutex to take
>> + * @waiter:		the rt_mutex_waiter initialized by the waiter
> 
>   initialized by the caller perhaps ?

Actually the rt_mutex_waiter is created on the stack of the waiter in 
futex_wait_requeue_pi() and added to the futex_q structure for the waker 
to access.  So it should be the waiter... if the comment is confusing I 
can either elaborate on multiple lines or just say something like:

"* @waiter:		the pre-initialized rt_mutex_waiter"

Since this call shouldn't care who initialized it, nor where, so long as 
it IS initialized.  I'll take this approach unless I hear otherwise.


> 
>> + * @task:		the task to prepare
>> + * @detext_deadlock:	passed to task_blocks_on_rt_mutex

"* @detect_deadlock:	perform deadlock detection (1) or not (0)"

> 
> That's not interesting where it is passed to. The argument tells us,
> whether deadlock detection needs to be done or not.
> 
>> + * The lock should have an owner, and it should not be task.
> 
> Why ? The lock can have no owner, if the previous owner released it
> before we took lock->wait_lock.

Hrm... I was considering moving the spin_lock(wait_lock) out of this 
routine, but we would still need to ensure the lock was still held. 
I'll look at making this safe without that condition.

> 
>> + * Special API call for FUTEX_REQUEUE_PI support.
>> + */
>> +int rt_mutex_start_proxy_lock(struct rt_mutex *lock,
>> +			      struct rt_mutex_waiter *waiter,
>> +			      struct task_struct *task, int detect_deadlock)
>> +{
>> +	int ret;
>> +
>> +	spin_lock(&lock->wait_lock);
> 
> You need to try to take the lock on behalf of task here under
> lock->wait_lock to avoid an enqueue on an ownerless rtmutex.
> 

Will do.

>> +
>> +/**
>> + * rt_mutex_finish_proxy_lock - Complete the taking of the lock initialized
>> on
>> + *                              our behalf by another thread.
> 
> IIRC this needs to be a single line. Or does kerneldoc support this now ?

You are correct.  V6 will correct all the kernel-doc screw-ups.

> 
>> + * @lock: the rt_mutex we were woken on
>> + * @to: the timeout, null if none. hrtimer should already have been started.
>> + * @waiter: the pre-initialized rt_mutex_waiter
>> + * @detect_deadlock: for use by __rt_mutex_slowlock
> 
> See above.

Check.

Thanks for the review,

-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists