Re: [RFC] myri10ge: small rx_done refactoring

On Thu, Mar 24, 2011 at 08:15:39AM -0700, Stephen Hemminger wrote:

quoted

On Wed, Mar 23, 2011 at 08:33:57AM -0700, Stephen Hemminger wrote:

quoted

On Wed, 23 Mar 2011 13:52:04 +0100
Stanislaw Gruszka [off-list ref] wrote:

quoted

Add lro_enable variable to read NETIF_F_LRO flag only once per napi poll
call. This should fix theoretical race condition with
myri10ge_set_rx_csum() and myri10ge_set_flags() where flag NETIF_F_LRO
can be changed.

You may need a barrier or the race may still be there.

I don't understand why barrier in that case is need.

What I tried to avoid is.

myri10ge_clean_rx_done():

if (dev->features & NETIF_F_LRO) 
	setup lro 
					myri10ge_set_flags()

if (dev->features & NETIF_F_LRO)
	flush lro

Now we read dev->features & NETIF_F_LRO only once to local
lro_enabled variable. So we can not flush without setup
or setup without flush. No idea why memory barries is still
needed.

quoted

The driver seems to use mb() where wmb() is intended, and never use rmb()?

Yes, I think we can have some optimalization here.

Without barrier there is no guarantee that compiler read the flags
into a local variable. It is free to do the same thing as the original
code.

Ok, so C code like:

code1
if (dev->features & NETIF_F_LRO) 
	branch1
code2;
if (dev->features & NETIF_F_LRO)
	branch2

and

bool lro_enabled = dev->features & NETIF_F_LRO;
code1
if (lro_enabled)
	branch1
code2
if (lro_enabled)
	branch2

can give the same assembly output.

It's really hard for me to understand that. I could
understand, if we would get global variable directly
like:
bool lro_enabled = dev->lro_enabled;
instead of:
bool lro_enabled = dev->features & NETIF_F_LRO;

David, can you confirm that Staphen is correct?

Also where this barrier() should go. Before  
"bool lro_enabled = dev->features & NETIF_F_LRO;"
or after?

Stanislaw