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

[Darren Hart]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/