Re: [PATCH] kernel/futex.c: Uneeded memory barrier

[Rusty Russell <rusty@rustcorp.com.au>]

In message <20030915102306.A22451@flint.arm.linux.org.uk> you write:
> On Mon, Sep 15, 2003 at 01:41:30PM +1000, Rusty Russell wrote:
> > ....hiding the subtlety in wrapper functions is the wrong approach.  We
> > have excellent wait_event, wait_event_interruptible and
> > wait_event_interruptible_timeout macros in wait.h which these drivers
> > should be using, which would make them simpler, less buggy and
> > smaller.
> 
> "smaller and simpler" hmm.  And _more_ buggy.  Let's take this case:
> 
> 	add_wait_queue(&wq, &wait);
> 	for (;;) {
> 		set_current_state(TASK_INTERRUPTIBLE);
> 		if (condition)
> 			break;
> 		if (file->f_flags & O_NONBLOCK) {
> 			ret = -EAGAIN;
> 			break;
> 		}
> 		if (signal_pending(current)) {
> 			ret = -ERESTARTSYS;
> 			break;
> 		}
> 		schedule();
> 	}
> 	__set_current_state(TASK_RUNNING);
> 	remove_wait_queue(&wq, &wait);
> 
> There are cases like the above which make the wait_event*() macros
> inappropriate:

There are, sure, but I'm not sure this is really one:

	ret = 0;
	if (file->f_flags & O_NONBLOCK) {
		if (!condition)
			ret = -EAGAIN;
	} else
		wait_event_interruptible(&wq, condition, &ret);

AFAICT this is equivalent, and clearer?

> - needing to atomically dequeue some data

Definitely: the futex code is a good example of this.  But the
presence of spinlocks actually makes the barrier issue vanish anyway.

> I've yet to see anyone using wait_event*() in these circumstances -
> they're great for your simple "did something happen" case which the
> majority of drivers use, but there are use cases where wait_event*()
> is not appropriate.

I don't mind someone doing something weird and needing to use lower
primitives, but I still feel the wait_event* family are badly
underused.  Perhaps it's just lack of publicity...

Cheers!
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.