lkml.org 
[lkml]   [2009]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2009-03-09 19:35    [W:0.070 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site