Re: [tip PATCH v6 1/8] RFC: futex: futex_wait_queue_me()

Darren Hart
2009-03-31T14:59:22Z

Thomas Gleixner wrote:
> On Mon, 30 Mar 2009, Darren Hart wrote:
>> +
>> + /* add_wait_queue is the barrier after __set_current_state. */
>> + __set_current_state(TASK_INTERRUPTIBLE);
>> + add_wait_queue(&q->waiter, &wait);
>> + /*
>> + * NOTE: we don't remove ourselves from the waitqueue because
>> + * we are the only user of it.
>> + */
>
> This comment, while correct is at an odd place.

How about something like this:

/* add_wait_queue is the barrier after __set_current_state. */
__set_current_state(TASK_INTERRUPTIBLE);

/*
* Add current as the futex_q waiter. We don't remove ourselves from
* the wait_queue because we are the only user of it.
*/
add_wait_queue(&q->waiter, &wait);

>
>> + /* Arm the timer */
>> + if (timeout) {
>> + hrtimer_start_expires(&timeout->timer, HRTIMER_MODE_ABS);
>> + if (!hrtimer_active(&timeout->timer))
>> + timeout->task = NULL;
>> + }
>> +
>> + /*
>> + * !plist_node_empty() is safe here without any lock.
>> + * q.lock_ptr != 0 is not safe, because of ordering against wakeup.
>> + */
>> + if (likely(!plist_node_empty(&q->list))) {
>> + /*
>> + * If the timer has already expired, current will already be
>> + * flagged for rescheduling. Only call schedule if there
>> + * is no timeout, or if it has yet to expire.
>> + */
>> + if (!timeout || likely(timeout->task))
>
> Remove the likely(). It does not make sense

Done.

>
>> + schedule();
>> + }
>> + __set_current_state(TASK_RUNNING);
>> +}
>> +
>> static int futex_wait(u32 __user *uaddr, int fshared,
>> u32 val, ktime_t *abs_time, u32 bitset, int clockrt)
>> {
>> - struct task_struct *curr = current;
>> + struct hrtimer_sleeper timeout, *to = NULL;
>> struct restart_block *restart;
>> - DECLARE_WAITQUEUE(wait, curr);
>> struct futex_hash_bucket *hb;
>> struct futex_q q;
>> u32 uval;
>> int ret;
>> - struct hrtimer_sleeper t;
>> - int rem = 0;
>>
>> if (!bitset)
>> return -EINVAL;
>>
>> q.pi_state = NULL;
>> q.bitset = bitset;
>> +
>> + if (abs_time) {
>> + unsigned long slack;
>
> missing new line
>
>> + to = &timeout;
>> + slack = current->timer_slack_ns;
>> + if (rt_task(current))
>> + slack = 0;
>
> Hmm. I thought we would use current->timer_slack_ns ?

Hrm, right, so long as I'm changing futex_wait I might as well correct this. Done (and voids the missing new line comment above).

Thanks,

--
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team