Re: [TIP][RFC 5/7] rt_mutex: add proxy lock routines

[Darren Hart]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/