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

[Thomas Gleixner <tglx@linutronix.de>]

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