netif_poll_enable() & barrier

netif_poll_enable() & barrier

[Benjamin Herrenschmidt]

Hi !

I stumbled accross what might be a bug on out of order architecture:

netif_poll_enable() only does a clear_bit(). However,
netif_poll_disable/enable pairs are often used as simili-spinlocks.

(netif_poll_enable() has pretty much spin_lock semantics except that it
schedules instead of looping).

Thus, shouldn't netif_poll_disable() do an smp_wmb(); before clearing
the bit to make sure that any stores done within the poll-disabled
section are properly visible to the rest of the system before clearing
the bit ?

Cheers,
Ben.

Re: netif_poll_enable() & barrier

[David Miller]

From: Benjamin Herrenschmidt <benh@au1.ibm.com>
Date: Wed, 20 Dec 2006 14:44:12 +1100

> I stumbled accross what might be a bug on out of order architecture:
> 
> netif_poll_enable() only does a clear_bit(). However,
> netif_poll_disable/enable pairs are often used as simili-spinlocks.
> 
> (netif_poll_enable() has pretty much spin_lock semantics except that it
> schedules instead of looping).
> 
> Thus, shouldn't netif_poll_disable() do an smp_wmb(); before clearing
> the bit to make sure that any stores done within the poll-disabled
> section are properly visible to the rest of the system before clearing
> the bit ?

Although I couldn't find a problematic case with any current
in-tree drivers, it's better to be safe than sorry :-)

So I'll add a smp_mb__before_clear_bit() to netif_poll_enable() :)

Re: netif_poll_enable() & barrier

[Benjamin Herrenschmidt]

On Thu, 2006-12-28 at 21:09 -0800, David Miller wrote:
> From: Benjamin Herrenschmidt <benh@au1.ibm.com>
> Date: Wed, 20 Dec 2006 14:44:12 +1100
> 
> > I stumbled accross what might be a bug on out of order architecture:
> > 
> > netif_poll_enable() only does a clear_bit(). However,
> > netif_poll_disable/enable pairs are often used as simili-spinlocks.
> > 
> > (netif_poll_enable() has pretty much spin_lock semantics except that it
> > schedules instead of looping).
> > 
> > Thus, shouldn't netif_poll_disable() do an smp_wmb(); before clearing
> > the bit to make sure that any stores done within the poll-disabled
> > section are properly visible to the rest of the system before clearing
> > the bit ?
> 
> Although I couldn't find a problematic case with any current
> in-tree drivers, it's better to be safe than sorry :-)
> 
> So I'll add a smp_mb__before_clear_bit() to netif_poll_enable() :)

Heh, thanks ! :-)

I haven't seen any problematic case neither, though if there was one, it
would result in weird problems very hard to track down, so as you said,
better safe than sorry (unless you see a flaw in my reasoning).

Cheers,

Oh, and happy new year too ! :-)

Ben.