On Fri, Jun 26, 2009 at 05:42:30AM +0000, Jarek Poplawski wrote:

On 26-06-2009 05:14, Davide Libenzi wrote:

quoted

On Fri, 26 Jun 2009, Eric Dumazet wrote:

quoted

I wont argue with you David, just try to correct bugs.

fs/ext4/ioctl.c line 182

	set_current_state(TASK_INTERRUPTIBLE);
	add_wait_queue(&EXT4_SB(sb)->ro_wait_queue, &wait);
	if (timer_pending(&EXT4_SB(sb)->turn_ro_timer)) {
		schedule();

Another example of missing barrier after add_wait_queue()

Because add_wait_queue() misses a barrier, we have to add one after each call.

Maybe it would be safer to add barrier in add_wait_queue() itself, not in _pollwait().

Not all the code that uses add_wait_queue() does need to have the MB,
like code that does the most common pattern:

xxx_poll(...) {
	poll_wait(...);
	lock();
	flags = calc_flags(->status);
	unlock();
	return flags;
}

xxx_update(...) {
	lock();
	->status = ...;
	unlock();
	if (waitqueue_active())
		wake_up();
}

It's the code that does the lockless flags calculation in ->poll that 
might need it.
I dunno what the amount of changes are, but cross-matching MB across 
subsystems does not look nice.
IMHO that's a detail of the subsystem locking, and should be confined 
inside the subsystem itself.
No?

How about poll_wait_mb() and waitqueue_active_mb() (with mb and
additional check for NULL of wait_queue_head)?

Hmm... But considering Eric's arguments I see it would be hard
/impossible to do it with the current api, so let's forget.

Jarek P.