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

[Darren Hart <dvhltc@us.ibm.com>]

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