On Fri, 26 Jun 2009, Eric Dumazet wrote:
> I wont argue with you David, just try to correct bugs.

quoted text
>

> 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?
- Davide
--

To unsubscribe from this list: send the line "unsubscribe netdev" in

the body of a message to majordomo@vger.kernel.org

More majordomo info at  http://vger.kernel.org/majordomo-info.html