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

Thomas Gleixner
2009-03-31T06:46:05Z

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.

> + /* 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

> + 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 ?

Thanks,

tglx