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

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

> > On Fri, 26 Jun 2009, Eric Dumazet wrote:

> >

> >> 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.

--

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