Re: netif_tx_disable and lockless TX

Re: netif_tx_disable and lockless TX

[Michael Chan]

Herbert Xu wrote:

> On Tue, May 30, 2006 at 09:13:22PM -0700, David Miller wrote:
> > As per the bug, I always keep thinking about changing how we
> > do the lockless stuff by simply making xmit_lock a pointer.
> > Drivers like tg3 can make it point to their local driver lock.
> 
> This is equivalent to just using the xmit_lock in the driver, right?
> IIRC the problem was with IRQ disabling vs. BH disabling.

For drivers that always take a private tx lock between tx completion
and hard_start_xmit on top of the xmit_lock, getting rid of the
xmit_lock using LLTX is a net gain.

For drivers that don't need a private tx lock or rarely need it between
tx completion and hard_start_xmit, it makes little difference whether
you use xmit_lock or private lock with LLTX.

> 
> That's why I suggest that every NIC that uses this feature be forced
> to do what TG3 does so only BH disabling is needed.  Once that's done
> they can just use xmit_lock and everyone will be happy again.
> 

As long as the tx completion is all done in NAPI, it can use BH
disabling
without irq disabling.

TG3 uses LLTX with a private tx_lock. BNX2 uses xmit_lock with a private
tx_lock that is rarely used. Both drivers use BH disabling. The profiles
of the 2 drivers are very similar.

Re: netif_tx_disable and lockless TX

[Herbert Xu]

Hi Michael:

On Tue, May 30, 2006 at 09:51:03PM -0700, Michael Chan wrote:
>
> > That's why I suggest that every NIC that uses this feature be forced
> > to do what TG3 does so only BH disabling is needed.  Once that's done
> > they can just use xmit_lock and everyone will be happy again.
> 
> As long as the tx completion is all done in NAPI, it can use BH
> disabling
> without irq disabling.

Yes, TG3 does not disable IRQs when taking its TX lock.  So do you see
any problems with replacing the TG3 TX lock using xmit_lock?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netif_tx_disable and lockless TX

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 31 May 2006 14:58:11 +1000

> Hi Michael:
> 
> On Tue, May 30, 2006 at 09:51:03PM -0700, Michael Chan wrote:
> >
> > > That's why I suggest that every NIC that uses this feature be forced
> > > to do what TG3 does so only BH disabling is needed.  Once that's done
> > > they can just use xmit_lock and everyone will be happy again.
> > 
> > As long as the tx completion is all done in NAPI, it can use BH
> > disabling
> > without irq disabling.
> 
> Yes, TG3 does not disable IRQs when taking its TX lock.  So do you see
> any problems with replacing the TG3 TX lock using xmit_lock?

I don't see any.

Thanks for reminding me about the IRQ vs. BH disabling issue.

It will come back when net channels arrive :-) This is because net
channel drivers will do all the work in hard IRQs and not in a NAPI or
NAPI-like context.

The only regret I have about that is we will go back to not being able
to profile ->hard_start_xmit() very well in such drivers.

Re: netif_tx_disable and lockless TX

[Herbert Xu]

On Tue, May 30, 2006 at 10:11:17PM -0700, David Miller wrote:
> 
> It will come back when net channels arrive :-) This is because net
> channel drivers will do all the work in hard IRQs and not in a NAPI or
> NAPI-like context.

I thought the current channel stuff is RX only.  Is TX completion moving
to IRQ context as well?

> The only regret I have about that is we will go back to not being able
> to profile ->hard_start_xmit() very well in such drivers.

Can you elborate on that? I think I've already removed all references
to this in my memory :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netif_tx_disable and lockless TX

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 31 May 2006 15:14:51 +1000

> On Tue, May 30, 2006 at 10:11:17PM -0700, David Miller wrote:
> > 
> > It will come back when net channels arrive :-) This is because net
> > channel drivers will do all the work in hard IRQs and not in a NAPI or
> > NAPI-like context.
> 
> I thought the current channel stuff is RX only.  Is TX completion moving
> to IRQ context as well?

I don't think it will be worthwhile to keep NAPI around just for
TX completion.  Sure the dev_kfree_skb() will schedule software
interrupt work to do the actual free, but the TX ring walking
and dev_kfree_skb() calls will be in hard IRQ context.

> > The only regret I have about that is we will go back to not being able
> > to profile ->hard_start_xmit() very well in such drivers.
> 
> Can you elborate on that? I think I've already removed all references
> to this in my memory :)

If you disable IRQs in the ->hard_start_xmit() handler, you don't
get timer based profiling ticks.   Currently we do.

Re: netif_tx_disable and lockless TX

[Herbert Xu]

On Tue, May 30, 2006 at 11:26:26PM -0700, David Miller wrote:
> 
> I don't think it will be worthwhile to keep NAPI around just for
> TX completion.  Sure the dev_kfree_skb() will schedule software
> interrupt work to do the actual free, but the TX ring walking
> and dev_kfree_skb() calls will be in hard IRQ context.

Sure we won't need all of NAPI.  But would it be that bad to schedule
a tasklet to do the TX completion?

> > > The only regret I have about that is we will go back to not being able
> > > to profile ->hard_start_xmit() very well in such drivers.
> > 
> > Can you elborate on that? I think I've already removed all references
> > to this in my memory :)
> 
> If you disable IRQs in the ->hard_start_xmit() handler, you don't
> get timer based profiling ticks.   Currently we do.

Oh, I'm not proposing that we disable IRQs on xmit_lock at all.  What
I'm saying is that for tg3, since both xmit_lock and tx_lock are BH-
disabling locks, they are equivalent and therefore we can replace
tx_lock with xmit_lock.

For other lockless NICs that have IRQ disabling tx_locks, they can
be converted to BH-disabling ones and then be able to use xmit_lock.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netif_tx_disable and lockless TX

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 31 May 2006 16:31:52 +1000

> On Tue, May 30, 2006 at 11:26:26PM -0700, David Miller wrote:
> > 
> > I don't think it will be worthwhile to keep NAPI around just for
> > TX completion.  Sure the dev_kfree_skb() will schedule software
> > interrupt work to do the actual free, but the TX ring walking
> > and dev_kfree_skb() calls will be in hard IRQ context.
> 
> Sure we won't need all of NAPI.  But would it be that bad to schedule
> a tasklet to do the TX completion?

It seems just an extra level of indirection. :)

> Oh, I'm not proposing that we disable IRQs on xmit_lock at all.
 ...

I understand what you're saying.  What I'm talking about is that in
net channel drivers we might go back to IRQ disabling locks again.

Re: netif_tx_disable and lockless TX

[Herbert Xu]

On Wed, May 31, 2006 at 12:08:18AM -0700, David Miller wrote:
> 
> I understand what you're saying.  What I'm talking about is that in
> net channel drivers we might go back to IRQ disabling locks again.

OK, let's assume that the TX completion will go back into the IRQ
handler.  I contend that we can still get by with a BH-disabling
xmit_lock.  This is how it would work:

The IRQ handler would look like

	if (!spin_trylock(&dev->xmit_lock)) {
		tasklet_schedule(&priv->tx_completion_tasklet);
		return;
	}

	handle_tx_completion();
	spin_unlock(&dev->xmit_lock);

Where the TX completion tasklet would simply do

	spin_lock(&dev->xmit_lock);
	handle_tx_completion();
	spin_unlock(&dev->xmit_lock);

What do you think?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netif_tx_disable and lockless TX

[jamal]

On Wed, 2006-31-05 at 22:06 +1000, Herbert Xu wrote:
> On Wed, May 31, 2006 at 12:08:18AM -0700, David Miller wrote:
> > 
> > I understand what you're saying.  What I'm talking about is that in
> > net channel drivers we might go back to IRQ disabling locks again.
> 
> OK, let's assume that the TX completion will go back into the IRQ
> handler.  I contend that we can still get by with a BH-disabling
> xmit_lock.  This is how it would work:
> 
> The IRQ handler would look like
> 
> 	if (!spin_trylock(&dev->xmit_lock)) {
> 		tasklet_schedule(&priv->tx_completion_tasklet);
> 		return;
> 	}
> 
> 	handle_tx_completion();
> 	spin_unlock(&dev->xmit_lock);
> 
> Where the TX completion tasklet would simply do
> 
> 	spin_lock(&dev->xmit_lock);
> 	handle_tx_completion();
> 	spin_unlock(&dev->xmit_lock);
> 
> What do you think?

Been done in the past, bad numbers especially in SMP for reasons of
latency and likelihood that a tasklet will run in a totally different
CPU.

Latency-wise: TX completion interrupt provides the best latency.
Processing in the poll() -aka softirq- was almost close to the hardirq
variant. So if you can make things run in a softirq such as transmit
one, then the numbers will likely stay the same.

Sorry, I havent been following discussions on netchannels[1] so i am not
qualified to comment on the "replacement" part Dave mentioned earlier.
What I can say is the tx processing doesnt have to be part of the NAPI
poll() and still use hardirq.

If you look at the earlier NAPI drivers such as the tulip or the
broadcom 1250, the TX EOL was always made an exception and was never
touched by the ->poll() code rather by the main interrupt routine. I do
prefer the scheme of leaving the TX EOL out of the NAPI poll because i
believe that it provides better performance. The caveat is there's a lot
of fscked hardware out there that does unconditional clear-on-read of
the int status register;-> I had a lot of fun with bcm1250 and ended
discovering that there were infact two status regs ;-> (it pays to know
people for undocumented things;->) one was for debug version which
doesnt clear on writing. So in one spot i would read the real register
and in the other the debug version. I had numbers which showed doing it
this way provided better performance than doing it in the poll() routine
(scouring the code just now it seems my version never made it in, so you
will see something along the lines of e1000/tg3).

BTW, Andi Kleen on CC is the person who posted some numbers on the LLTX
(as well as the patch) at some point.
I have also CCed Robert who may have comments on the use of tasklets. 

cheers,
jamal

[1] Such a shame, such an exciting topic, such few cycles. I will have
time RealSoonNow and you will probably see a_patch_from_me_too.

Re: netif_tx_disable and lockless TX

[Herbert Xu]

On Wed, May 31, 2006 at 08:36:12AM -0400, jamal wrote:
>
> > The IRQ handler would look like
> > 
> > 	if (!spin_trylock(&dev->xmit_lock)) {
> > 		tasklet_schedule(&priv->tx_completion_tasklet);
> > 		return;
> > 	}
> > 
> > 	handle_tx_completion();
> > 	spin_unlock(&dev->xmit_lock);
> > 
> > Where the TX completion tasklet would simply do
> > 
> > 	spin_lock(&dev->xmit_lock);
> > 	handle_tx_completion();
> > 	spin_unlock(&dev->xmit_lock);
> > 
> > What do you think?
> 
> Been done in the past, bad numbers especially in SMP for reasons of
> latency and likelihood that a tasklet will run in a totally different
> CPU.

Was the bad latency measured with code like the above or was it with
an unconditional tasklet schedule?

Please note that my code will do the completion in the IRQ handler
most of the time.  Only if there is contention for the spin lock will
it defer work to the softirq.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netif_tx_disable and lockless TX

[jamal]

On Wed, 2006-31-05 at 22:40 +1000, Herbert Xu wrote:
> On Wed, May 31, 2006 at 08:36:12AM -0400, jamal wrote:
> >
[..]
> > 
> > Been done in the past, bad numbers especially in SMP for reasons of
> > latency and likelihood that a tasklet will run in a totally different
> > CPU.
> 
> Was the bad latency measured with code like the above or was it with
> an unconditional tasklet schedule?
> 

The idea was to prune those skbs for reuse within reason so as to
not sacrifice perfomance. Perfomance is sacrificed if you have too many
TX EOLs, or you dont prune the descriptors fast enough. The dynamics of
proved to be hard to nail in a generic way because behavior varies based
on traffic patterns.
So the "within reason" part will unconditionally schedule a tasklet on
the tx if certain DMA ring thresholds are crossed, way after the tx lock
has been grabbed which is different from your suggestion to do it when
you cant grab a lock.
Otherwise you let the hardirq take care of things. Tasklets proved to be
a bad idea and instead we ended reclaiming the skb/descriptors right at
the point where  we initially had scheduled tasklets. I dont know what
happened to using that technique - Robert can testify better than i.

> Please note that my code will do the completion in the IRQ handler
> most of the time.  Only if there is contention for the spin lock will
> it defer work to the softirq.

that part is different and I think there may be potential - it will
require experimenting with a variety of traffic patterns. I wish i had
time to help.

cheers,
jamal

Re: netif_tx_disable and lockless TX

[Robert Olsson]

jamal writes:

 > Latency-wise: TX completion interrupt provides the best latency.
 > Processing in the poll() -aka softirq- was almost close to the hardirq
 > variant. So if you can make things run in a softirq such as transmit
 > one, then the numbers will likely stay the same.
 
 I don't remember we tried tasklet for TX a la Herbert's suggestion but we 
 used use tasklets for controlling RX processing to avoid hardirq livelock
 in pre-NAPI versions.

 Had variants of tulip driver with both TX cleaning at ->poll and TX
 cleaning at hardirq and didn't see any performance difference. The 
 ->poll was much cleaner but we kept Alexey's original work for tulip.

 > Sorry, I havent been following discussions on netchannels[1] so i am not
 > qualified to comment on the "replacement" part Dave mentioned earlier.
 > What I can say is the tx processing doesnt have to be part of the NAPI
 > poll() and still use hardirq.

 Yes true but I see TX numbers with newer boards (wire rate small pakets)
 with cleaing in ->poll. Also now linux is very safe in network "overload" 
 situations. Moving work to hardirq may change that.

 Cheers.

					--ro

Re: netif_tx_disable and lockless TX

[Stephen Hemminger]

Robert Olsson wrote:
> jamal writes:
>
>  > Latency-wise: TX completion interrupt provides the best latency.
>  > Processing in the poll() -aka softirq- was almost close to the hardirq
>  > variant. So if you can make things run in a softirq such as transmit
>  > one, then the numbers will likely stay the same.
>  
>  I don't remember we tried tasklet for TX a la Herbert's suggestion but we 
>  used use tasklets for controlling RX processing to avoid hardirq livelock
>  in pre-NAPI versions.
>
>  Had variants of tulip driver with both TX cleaning at ->poll and TX
>  cleaning at hardirq and didn't see any performance difference. The 
>  ->poll was much cleaner but we kept Alexey's original work for tulip.
>
>  > Sorry, I havent been following discussions on netchannels[1] so i am not
>  > qualified to comment on the "replacement" part Dave mentioned earlier.
>  > What I can say is the tx processing doesnt have to be part of the NAPI
>  > poll() and still use hardirq.
>
>  Yes true but I see TX numbers with newer boards (wire rate small pakets)
>  with cleaing in ->poll. Also now linux is very safe in network "overload" 
>  situations. Moving work to hardirq may change that.
>
>
>   
I also noticed that you really don't save much by doing TX cleaning at 
hardirq, because in hardirq you need to do dev_kfree_irq and that causes 
a softirq (for the routing case where users=1). So when routing it 
doesn't make much difference, both methods cause the softirq delayed 
processing to be invoked. For locally generated packets which are 
cloned, the hardirq will drop the ref count, and that is faster than 
doing the whole softirq round trip.

Re: netif_tx_disable and lockless TX

[Robert Olsson]

Stephen Hemminger writes:

 > I also noticed that you really don't save much by doing TX cleaning at 
 > hardirq, because in hardirq you need to do dev_kfree_irq and that causes 
 > a softirq (for the routing case where users=1). So when routing it 
 > doesn't make much difference, both methods cause the softirq delayed 
 > processing to be invoked. For locally generated packets which are 
 > cloned, the hardirq will drop the ref count, and that is faster than 
 > doing the whole softirq round trip.

 Right. Also the other way around, repeated ->poll can avoid TX hardirq's.

 Cheers.
						--ro

Re: netif_tx_disable and lockless TX

[jamal]

On Wed, 2006-31-05 at 19:52 +0200, Robert Olsson wrote:
> jamal writes:
> 
>  > Latency-wise: TX completion interrupt provides the best latency.
>  > Processing in the poll() -aka softirq- was almost close to the hardirq
>  > variant. So if you can make things run in a softirq such as transmit
>  > one, then the numbers will likely stay the same.
>  
>  I don't remember we tried tasklet for TX a la Herbert's suggestion but we 
>  used use tasklets for controlling RX processing to avoid hardirq livelock
>  in pre-NAPI versions.
> 

Hrm - it may have been a private thing i did then. I could swear we did
that experiment together ...
Perhaps Herbert's motivation was not really to optimize but rather to
get something unstuck in the transmit path state machine maybe in a
context of netconsole? The conditions for which that tasklet would even
run require a CPU collision to the transmit. Sorry, I didnt quiet follow
the motivation/discussion that ended in that patch.

>  Had variants of tulip driver with both TX cleaning at ->poll and TX
>  cleaning at hardirq and didn't see any performance difference. The 
>  ->poll was much cleaner but we kept Alexey's original work for tulip.
> 

It certainly is cleaner - but i do recall the hardirq variant had better
latency much observable under high packet rates aka small packets. 

>  > Sorry, I havent been following discussions on netchannels[1] so i am not
>  > qualified to comment on the "replacement" part Dave mentioned earlier.
>  > What I can say is the tx processing doesnt have to be part of the NAPI
>  > poll() and still use hardirq.
> 
>  Yes true but I see TX numbers with newer boards (wire rate small pakets)
>  with cleaing in ->poll. Also now linux is very safe in network "overload" 
>  situations. Moving work to hardirq may change that.
> 

Oh, I am not suggesting a change - i am a lot more conservative than
that ;-> these areas are delicate (not code-delicate Acme ;->) but
rather what seems obvious requires a lot of experimental results first.

Robert, your transmit results Intel or AMD based?

cheers,
jamal

Re: netif_tx_disable and lockless TX

[Michael Chan]

On Tue, 2006-05-30 at 22:11 -0700, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Wed, 31 May 2006 14:58:11 +1000
> 
> > Yes, TG3 does not disable IRQs when taking its TX lock.  So do you see
> > any problems with replacing the TG3 TX lock using xmit_lock?
> 
> I don't see any.
> 
David, So do we want to fix this issue as proposed by Herbert to replace
tx_lock with xmit_lock? It seems quite straightforward to do. For this
change to work, netpoll also needs to be fixed up a bit to check for
LLTX before getting the xmit_lock.

Re: netif_tx_disable and lockless TX

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Wed, 31 May 2006 14:20:29 -0700

> David, So do we want to fix this issue as proposed by Herbert to replace
> tx_lock with xmit_lock? It seems quite straightforward to do. For this
> change to work, netpoll also needs to be fixed up a bit to check for
> LLTX before getting the xmit_lock.

Oh yes, netpoll needs changes, thanks for noticing that.

That's why I had the idea to make xmit_lock a pointer, so
that things like that netpoll case could be transparent.

I think this netpoll wrinkle means we also have to make
sure to set the xmit_lock_owner across the board.

Re: netif_tx_disable and lockless TX

[Herbert Xu]

On Wed, May 31, 2006 at 05:09:08PM -0700, David Miller wrote:
> From: "Michael Chan" <mchan@broadcom.com>
> Date: Wed, 31 May 2006 14:20:29 -0700
> 
> > David, So do we want to fix this issue as proposed by Herbert to replace
> > tx_lock with xmit_lock? It seems quite straightforward to do. For this
> > change to work, netpoll also needs to be fixed up a bit to check for
> > LLTX before getting the xmit_lock.
> 
> Oh yes, netpoll needs changes, thanks for noticing that.

Why does it need to change? The idea is to take the LLTX flag off tg3
after the conversion to xmit_lock.

> I think this netpoll wrinkle means we also have to make
> sure to set the xmit_lock_owner across the board.

You're right.  In fact this can deadlock today for those drivers that
already make use of xmit_lock without setting the owner.  So I suppose
something like net_xmit_lock to obtain xmit_lock is called for.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netif_tx_disable and lockless TX

[Michael Chan]

On Thu, 2006-06-01 at 10:25 +1000, Herbert Xu wrote:
> On Wed, May 31, 2006 at 05:09:08PM -0700, David Miller wrote:
> > From: "Michael Chan" <mchan@broadcom.com>
> > Date: Wed, 31 May 2006 14:20:29 -0700
> > 
> > > David, So do we want to fix this issue as proposed by Herbert to replace
> > > tx_lock with xmit_lock? It seems quite straightforward to do. For this
> > > change to work, netpoll also needs to be fixed up a bit to check for
> > > LLTX before getting the xmit_lock.
> > 
> > Oh yes, netpoll needs changes, thanks for noticing that.
> 
> Why does it need to change? The idea is to take the LLTX flag off tg3
> after the conversion to xmit_lock.

Oh, I thought your idea was to keep the LLTX and just replace tx_lock
with xmit_lock in tg3. But I suppose we can also clear LLTX, remove the
tx_lock in hard_start_xmit and convert the rest to avoid changing
netpoll.

Re: netif_tx_disable and lockless TX

[Herbert Xu]

On Wed, May 31, 2006 at 04:01:06PM -0700, Michael Chan wrote:
> 
> Oh, I thought your idea was to keep the LLTX and just replace tx_lock
> with xmit_lock in tg3. But I suppose we can also clear LLTX, remove the
> tx_lock in hard_start_xmit and convert the rest to avoid changing
> netpoll.

That's what I meant.  This has the advantage that things like AF_PACKET
won't see duplicate packets.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netif_tx_disable and lockless TX

[Michael Chan]

On Thu, 2006-06-01 at 10:42 +1000, Herbert Xu wrote:
> On Wed, May 31, 2006 at 04:01:06PM -0700, Michael Chan wrote:
> > 
> > Oh, I thought your idea was to keep the LLTX and just replace tx_lock
> > with xmit_lock in tg3. But I suppose we can also clear LLTX, remove the
> > tx_lock in hard_start_xmit and convert the rest to avoid changing
> > netpoll.
> 
> That's what I meant.  This has the advantage that things like AF_PACKET
> won't see duplicate packets.

Now that I think about it some more, we can eliminate most of the
tx_lock in tg3 without converting to xmit_lock. In most places, we get
the tx_lock after we call netif_tx_disable(), etc, and quiesce the chip.
So the tx_lock or xmit_lock is really not necessary in most places. The
tp->lock is sufficient after we stop everything.

The only few remaining places where we need it is around
netif_stop_queue/netif_wake_queue in hard_start_xmit and tx completion.
We can either keep the tx_lock or convert to xmit_lock and either way
makes little difference because these are not in the normal fast path.
If we keep the tx_lock, it will be just like BNX2.

Re: netif_tx_disable and lockless TX

[Herbert Xu]

On Wed, May 31, 2006 at 05:27:53PM -0700, Michael Chan wrote:
> 
> Now that I think about it some more, we can eliminate most of the
> tx_lock in tg3 without converting to xmit_lock. In most places, we get
> the tx_lock after we call netif_tx_disable(), etc, and quiesce the chip.
> So the tx_lock or xmit_lock is really not necessary in most places. The
> tp->lock is sufficient after we stop everything.

Sounds good.

> The only few remaining places where we need it is around
> netif_stop_queue/netif_wake_queue in hard_start_xmit and tx completion.
> We can either keep the tx_lock or convert to xmit_lock and either way
> makes little difference because these are not in the normal fast path.
> If we keep the tx_lock, it will be just like BNX2.

Now that I look at it the BNX2 method looks perfect.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[NET]: Add netif_tx_lock

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 2180 bytes --]

On Thu, Jun 01, 2006 at 10:25:25AM +1000, herbert wrote:
> 
> > I think this netpoll wrinkle means we also have to make
> > sure to set the xmit_lock_owner across the board.
> 
> You're right.  In fact this can deadlock today for those drivers that
> already make use of xmit_lock without setting the owner.  So I suppose
> something like net_xmit_lock to obtain xmit_lock is called for.

OK, here is a patch which does this.

[NET]: Add netif_tx_lock

Various drivers use xmit_lock internally to synchronise with their
transmission routines.  They do so without setting xmit_lock_owner.
This is fine as long as netpoll is not in use.

With netpoll it is possible for deadlocks to occur if xmit_lock_owner
isn't set.  This is because if a printk occurs while xmit_lock is held
and xmit_lock_owner is not set can cause netpoll to attempt to take
xmit_lock recursively.

While it is possible to resolve this by getting netpoll to use trylock,
it is suboptimal because netpoll's sole objective is to maximise the
chance of getting the printk out on the wire.  So delaying or dropping
the message is to be avoided as much as possible.

So the only alternative is to always set xmit_lock_owner.  The following
patch does this by introducing the netif_tx_lock family of functions that 
take care of setting/unsetting xmit_lock_owner.

I renamed xmit_lock to _xmit_lock to indicate that it should not be used
directly.  I didn't provide irq versions of the netif_tx_lock functions
since xmit_lock is meant to be a BH-disabling lock.

This is pretty much a straight text substitution except for a small bug
fix in winbond.  It currently uses netif_stop_queue/spin_unlock_wait to
stop transmission.  This is unsafe as an IRQ can potentially wake up the
queue.  So it is safer to use netif_tx_disable.

The hamradio bits used spin_lock_irq but it is unnecessary as xmit_lock
must never be taken in an IRQ handler.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: netif-tx-lock.patch --]
[-- Type: text/plain, Size: 23979 bytes --]

diff --git a/Documentation/networking/netdevices.txt b/Documentation/networking/netdevices.txt
index 3c0a5ba..847cedb 100644
--- a/Documentation/networking/netdevices.txt
+++ b/Documentation/networking/netdevices.txt
@@ -42,9 +42,9 @@ dev->get_stats:
 	Context: nominally process, but don't sleep inside an rwlock
 
 dev->hard_start_xmit:
-	Synchronization: dev->xmit_lock spinlock.
+	Synchronization: netif_tx_lock spinlock.
 	When the driver sets NETIF_F_LLTX in dev->features this will be
-	called without holding xmit_lock. In this case the driver 
+	called without holding netif_tx_lock. In this case the driver
 	has to lock by itself when needed. It is recommended to use a try lock
 	for this and return -1 when the spin lock fails. 
 	The locking there should also properly protect against 
@@ -62,12 +62,12 @@ dev->hard_start_xmit:
 	  Only valid when NETIF_F_LLTX is set.
 
 dev->tx_timeout:
-	Synchronization: dev->xmit_lock spinlock.
+	Synchronization: netif_tx_lock spinlock.
 	Context: BHs disabled
 	Notes: netif_queue_stopped() is guaranteed true
 
 dev->set_multicast_list:
-	Synchronization: dev->xmit_lock spinlock.
+	Synchronization: netif_tx_lock spinlock.
 	Context: BHs disabled
 
 dev->poll:
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 1dae4b2..1d917ed 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -821,7 +821,8 @@ void ipoib_mcast_restart_task(void *dev_
 
 	ipoib_mcast_stop_thread(dev, 0);
 
-	spin_lock_irqsave(&dev->xmit_lock, flags);
+	local_irq_save(flags);
+	netif_tx_lock(dev);
 	spin_lock(&priv->lock);
 
 	/*
@@ -896,7 +897,8 @@ void ipoib_mcast_restart_task(void *dev_
 	}
 
 	spin_unlock(&priv->lock);
-	spin_unlock_irqrestore(&dev->xmit_lock, flags);
+	netif_tx_unlock(dev);
+	local_irq_restore(flags);
 
 	/* We have to cancel outside of the spinlock */
 	list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {
diff --git a/drivers/media/dvb/dvb-core/dvb_net.c b/drivers/media/dvb/dvb-core/dvb_net.c
index 2f0f358..9fd8752 100644
--- a/drivers/media/dvb/dvb-core/dvb_net.c
+++ b/drivers/media/dvb/dvb-core/dvb_net.c
@@ -1052,7 +1052,7 @@ static void wq_set_multicast_list (void 
 
 	dvb_net_feed_stop(dev);
 	priv->rx_mode = RX_MODE_UNI;
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 
 	if (dev->flags & IFF_PROMISC) {
 		dprintk("%s: promiscuous mode\n", dev->name);
@@ -1077,7 +1077,7 @@ static void wq_set_multicast_list (void 
 		}
 	}
 
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 	dvb_net_feed_start(dev);
 }
 
diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 54161ae..9c5a884 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -2009,7 +2009,7 @@ bnx2_poll(struct net_device *dev, int *b
 	return 1;
 }
 
-/* Called with rtnl_lock from vlan functions and also dev->xmit_lock
+/* Called with rtnl_lock from vlan functions and also netif_tx_lock
  * from set_multicast.
  */
 static void
@@ -4252,7 +4252,7 @@ bnx2_vlan_rx_kill_vid(struct net_device 
 }
 #endif
 
-/* Called with dev->xmit_lock.
+/* Called with netif_tx_lock.
  * hard_start_xmit is pseudo-lockless - a lock is only required when
  * the tx queue is full. This way, we get the benefit of lockless
  * operations most of the time without the complexities to handle
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 55d2367..46326cd 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4191,7 +4191,7 @@ static int bond_init(struct net_device *
 	 */
 	bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
 
-	/* don't acquire bond device's xmit_lock when 
+	/* don't acquire bond device's netif_tx_lock when
 	 * transmitting */
 	bond_dev->features |= NETIF_F_LLTX;
 
diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 705e122..cb1b4d0 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -533,9 +533,9 @@ #define NV_MSI_X_VECTOR_OTHER 0x2
  * critical parts:
  * - rx is (pseudo-) lockless: it relies on the single-threading provided
  *	by the arch code for interrupts.
- * - tx setup is lockless: it relies on dev->xmit_lock. Actual submission
+ * - tx setup is lockless: it relies on netif_tx_lock. Actual submission
  *	needs dev->priv->lock :-(
- * - set_multicast_list: preparation lockless, relies on dev->xmit_lock.
+ * - set_multicast_list: preparation lockless, relies on netif_tx_lock.
  */
 
 /* in dev: base, irq */
@@ -1213,7 +1213,7 @@ static void drain_ring(struct net_device
 
 /*
  * nv_start_xmit: dev->hard_start_xmit function
- * Called with dev->xmit_lock held.
+ * Called with netif_tx_lock held.
  */
 static int nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
@@ -1407,7 +1407,7 @@ static void nv_tx_done(struct net_device
 
 /*
  * nv_tx_timeout: dev->tx_timeout function
- * Called with dev->xmit_lock held.
+ * Called with netif_tx_lock held.
  */
 static void nv_tx_timeout(struct net_device *dev)
 {
@@ -1737,7 +1737,7 @@ static int nv_change_mtu(struct net_devi
 		 * Changing the MTU is a rare event, it shouldn't matter.
 		 */
 		nv_disable_irq(dev);
-		spin_lock_bh(&dev->xmit_lock);
+		netif_tx_lock_bh(dev);
 		spin_lock(&np->lock);
 		/* stop engines */
 		nv_stop_rx(dev);
@@ -1768,7 +1768,7 @@ static int nv_change_mtu(struct net_devi
 		nv_start_rx(dev);
 		nv_start_tx(dev);
 		spin_unlock(&np->lock);
-		spin_unlock_bh(&dev->xmit_lock);
+		netif_tx_unlock_bh(dev);
 		nv_enable_irq(dev);
 	}
 	return 0;
@@ -1803,7 +1803,7 @@ static int nv_set_mac_address(struct net
 	memcpy(dev->dev_addr, macaddr->sa_data, ETH_ALEN);
 
 	if (netif_running(dev)) {
-		spin_lock_bh(&dev->xmit_lock);
+		netif_tx_lock_bh(dev);
 		spin_lock_irq(&np->lock);
 
 		/* stop rx engine */
@@ -1815,7 +1815,7 @@ static int nv_set_mac_address(struct net
 		/* restart rx engine */
 		nv_start_rx(dev);
 		spin_unlock_irq(&np->lock);
-		spin_unlock_bh(&dev->xmit_lock);
+		netif_tx_unlock_bh(dev);
 	} else {
 		nv_copy_mac_to_hw(dev);
 	}
@@ -1824,7 +1824,7 @@ static int nv_set_mac_address(struct net
 
 /*
  * nv_set_multicast: dev->set_multicast function
- * Called with dev->xmit_lock held.
+ * Called with netif_tx_lock held.
  */
 static void nv_set_multicast(struct net_device *dev)
 {
diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index 102c1f0..d12605f 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c
@@ -308,9 +308,9 @@ static int sp_set_mac_address(struct net
 {
 	struct sockaddr_ax25 *sa = addr;
 
-	spin_lock_irq(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	memcpy(dev->dev_addr, &sa->sax25_call, AX25_ADDR_LEN);
-	spin_unlock_irq(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 
 	return 0;
 }
@@ -767,9 +767,9 @@ static int sixpack_ioctl(struct tty_stru
 			break;
 		}
 
-		spin_lock_irq(&dev->xmit_lock);
+		netif_tx_lock_bh(dev);
 		memcpy(dev->dev_addr, &addr, AX25_ADDR_LEN);
-		spin_unlock_irq(&dev->xmit_lock);
+		netif_tx_unlock_bh(dev);
 
 		err = 0;
 		break;
diff --git a/drivers/net/hamradio/mkiss.c b/drivers/net/hamradio/mkiss.c
index d81a8e1..3ebbbe5 100644
--- a/drivers/net/hamradio/mkiss.c
+++ b/drivers/net/hamradio/mkiss.c
@@ -357,9 +357,9 @@ static int ax_set_mac_address(struct net
 {
 	struct sockaddr_ax25 *sa = addr;
 
-	spin_lock_irq(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	memcpy(dev->dev_addr, &sa->sax25_call, AX25_ADDR_LEN);
-	spin_unlock_irq(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 
 	return 0;
 }
@@ -886,9 +886,9 @@ static int mkiss_ioctl(struct tty_struct
 			break;
 		}
 
-		spin_lock_irq(&dev->xmit_lock);
+		netif_tx_lock_bh(dev);
 		memcpy(dev->dev_addr, addr, AX25_ADDR_LEN);
-		spin_unlock_irq(&dev->xmit_lock);
+		netif_tx_unlock_bh(dev);
 
 		err = 0;
 		break;
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 31fb2d7..2e222ef 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -76,13 +76,13 @@ static void ri_tasklet(unsigned long dev
 	dp->st_task_enter++;
 	if ((skb = skb_peek(&dp->tq)) == NULL) {
 		dp->st_txq_refl_try++;
-		if (spin_trylock(&_dev->xmit_lock)) {
+		if (netif_tx_trylock(_dev)) {
 			dp->st_rxq_enter++;
 			while ((skb = skb_dequeue(&dp->rq)) != NULL) {
 				skb_queue_tail(&dp->tq, skb);
 				dp->st_rx2tx_tran++;
 			}
-			spin_unlock(&_dev->xmit_lock);
+			netif_tx_unlock(_dev);
 		} else {
 			/* reschedule */
 			dp->st_rxq_notenter++;
@@ -110,7 +110,7 @@ static void ri_tasklet(unsigned long dev
 		}
 	}
 
-	if (spin_trylock(&_dev->xmit_lock)) {
+	if (netif_tx_trylock(_dev)) {
 		dp->st_rxq_check++;
 		if ((skb = skb_peek(&dp->rq)) == NULL) {
 			dp->tasklet_pending = 0;
@@ -118,10 +118,10 @@ static void ri_tasklet(unsigned long dev
 				netif_wake_queue(_dev);
 		} else {
 			dp->st_rxq_rsch++;
-			spin_unlock(&_dev->xmit_lock);
+			netif_tx_unlock(_dev);
 			goto resched;
 		}
-		spin_unlock(&_dev->xmit_lock);
+		netif_tx_unlock(_dev);
 	} else {
 resched:
 		dp->tasklet_pending = 1;
diff --git a/drivers/net/irda/vlsi_ir.c b/drivers/net/irda/vlsi_ir.c
index 97a49e0..d70b9e8 100644
--- a/drivers/net/irda/vlsi_ir.c
+++ b/drivers/net/irda/vlsi_ir.c
@@ -959,7 +959,7 @@ static int vlsi_hard_start_xmit(struct s
 			    ||  (now.tv_sec==ready.tv_sec && now.tv_usec>=ready.tv_usec))
 			    	break;
 			udelay(100);
-			/* must not sleep here - we are called under xmit_lock! */
+			/* must not sleep here - called under netif_tx_lock! */
 		}
 	}
 
diff --git a/drivers/net/natsemi.c b/drivers/net/natsemi.c
index 9062775..2e4eced 100644
--- a/drivers/net/natsemi.c
+++ b/drivers/net/natsemi.c
@@ -318,12 +318,12 @@ performance critical codepaths:
 The rx process only runs in the interrupt handler. Access from outside
 the interrupt handler is only permitted after disable_irq().
 
-The rx process usually runs under the dev->xmit_lock. If np->intr_tx_reap
+The rx process usually runs under the netif_tx_lock. If np->intr_tx_reap
 is set, then access is permitted under spin_lock_irq(&np->lock).
 
 Thus configuration functions that want to access everything must call
 	disable_irq(dev->irq);
-	spin_lock_bh(dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	spin_lock_irq(&np->lock);
 
 IV. Notes
diff --git a/drivers/net/tulip/winbond-840.c b/drivers/net/tulip/winbond-840.c
index 136a70c..6ff016d 100644
--- a/drivers/net/tulip/winbond-840.c
+++ b/drivers/net/tulip/winbond-840.c
@@ -1605,11 +1605,11 @@ #ifdef CONFIG_PM
  * - get_stats:
  * 	spin_lock_irq(np->lock), doesn't touch hw if not present
  * - hard_start_xmit:
- * 	netif_stop_queue + spin_unlock_wait(&dev->xmit_lock);
+ * 	synchronize_irq + netif_tx_disable;
  * - tx_timeout:
- * 	netif_device_detach + spin_unlock_wait(&dev->xmit_lock);
+ * 	netif_device_detach + netif_tx_disable;
  * - set_multicast_list
- * 	netif_device_detach + spin_unlock_wait(&dev->xmit_lock);
+ * 	netif_device_detach + netif_tx_disable;
  * - interrupt handler
  * 	doesn't touch hw if not present, synchronize_irq waits for
  * 	running instances of the interrupt handler.
@@ -1635,11 +1635,10 @@ static int w840_suspend (struct pci_dev 
 		netif_device_detach(dev);
 		update_csr6(dev, 0);
 		iowrite32(0, ioaddr + IntrEnable);
-		netif_stop_queue(dev);
 		spin_unlock_irq(&np->lock);
 
-		spin_unlock_wait(&dev->xmit_lock);
 		synchronize_irq(dev->irq);
+		netif_disable_tx(dev);
 	
 		np->stats.rx_missed_errors += ioread32(ioaddr + RxMissed) & 0xffff;
 
diff --git a/drivers/net/wireless/orinoco.c b/drivers/net/wireless/orinoco.c
index c2d0b09..a5fcfcd 100644
--- a/drivers/net/wireless/orinoco.c
+++ b/drivers/net/wireless/orinoco.c
@@ -1833,7 +1833,9 @@ static int __orinoco_program_rids(struct
 	/* Set promiscuity / multicast*/
 	priv->promiscuous = 0;
 	priv->mc_count = 0;
-	__orinoco_set_multicast_list(dev); /* FIXME: what about the xmit_lock */
+
+	/* FIXME: what about netif_tx_lock */
+	__orinoco_set_multicast_list(dev);
 
 	return 0;
 }
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f4169bb..01699eb 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -406,7 +406,7 @@ #define NETIF_F_UFO             8192    
  * One part is mostly used on xmit path (device)
  */
 	/* hard_start_xmit synchronizer */
-	spinlock_t		xmit_lock ____cacheline_aligned_in_smp;
+	spinlock_t		_xmit_lock ____cacheline_aligned_in_smp;
 	/* cpu id of processor entered to hard_start_xmit or -1,
 	   if nobody entered there.
 	 */
@@ -889,11 +889,43 @@ static inline void __netif_rx_complete(s
 	clear_bit(__LINK_STATE_RX_SCHED, &dev->state);
 }
 
+static inline void netif_tx_lock(struct net_device *dev)
+{
+	spin_lock(&dev->_xmit_lock);
+	dev->xmit_lock_owner = smp_processor_id();
+}
+
+static inline void netif_tx_lock_bh(struct net_device *dev)
+{
+	spin_lock_bh(&dev->_xmit_lock);
+	dev->xmit_lock_owner = smp_processor_id();
+}
+
+static inline int netif_tx_trylock(struct net_device *dev)
+{
+	int err = spin_trylock(&dev->_xmit_lock);
+	if (!err)
+		dev->xmit_lock_owner = smp_processor_id();
+	return err;
+}
+
+static inline void netif_tx_unlock(struct net_device *dev)
+{
+	dev->xmit_lock_owner = -1;
+	spin_unlock(&dev->_xmit_lock);
+}
+
+static inline void netif_tx_unlock_bh(struct net_device *dev)
+{
+	dev->xmit_lock_owner = -1;
+	spin_unlock_bh(&dev->_xmit_lock);
+}
+
 static inline void netif_tx_disable(struct net_device *dev)
 {
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	netif_stop_queue(dev);
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 }
 
 /* These functions live elsewhere (drivers/net/net_init.c, but related) */
diff --git a/net/atm/clip.c b/net/atm/clip.c
index 72d8529..f92f9c9 100644
--- a/net/atm/clip.c
+++ b/net/atm/clip.c
@@ -98,7 +98,7 @@ static void unlink_clip_vcc(struct clip_
 		printk(KERN_CRIT "!clip_vcc->entry (clip_vcc %p)\n", clip_vcc);
 		return;
 	}
-	spin_lock_bh(&entry->neigh->dev->xmit_lock);	/* block clip_start_xmit() */
+	netif_tx_lock_bh(entry->neigh->dev);	/* block clip_start_xmit() */
 	entry->neigh->used = jiffies;
 	for (walk = &entry->vccs; *walk; walk = &(*walk)->next)
 		if (*walk == clip_vcc) {
@@ -122,7 +122,7 @@ static void unlink_clip_vcc(struct clip_
 	printk(KERN_CRIT "ATMARP: unlink_clip_vcc failed (entry %p, vcc "
 	       "0x%p)\n", entry, clip_vcc);
       out:
-	spin_unlock_bh(&entry->neigh->dev->xmit_lock);
+	netif_tx_unlock_bh(entry->neigh->dev);
 }
 
 /* The neighbour entry n->lock is held. */
diff --git a/net/core/dev.c b/net/core/dev.c
index 4fba549..8906f59 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1275,15 +1275,13 @@ int __skb_linearize(struct sk_buff *skb,
 
 #define HARD_TX_LOCK(dev, cpu) {			\
 	if ((dev->features & NETIF_F_LLTX) == 0) {	\
-		spin_lock(&dev->xmit_lock);		\
-		dev->xmit_lock_owner = cpu;		\
+		netif_tx_lock(dev);			\
 	}						\
 }
 
 #define HARD_TX_UNLOCK(dev) {				\
 	if ((dev->features & NETIF_F_LLTX) == 0) {	\
-		dev->xmit_lock_owner = -1;		\
-		spin_unlock(&dev->xmit_lock);		\
+		netif_tx_unlock(dev);			\
 	}						\
 }
 
@@ -1382,8 +1380,8 @@ #endif
 	/* The device has no queue. Common case for software devices:
 	   loopback, all the sorts of tunnels...
 
-	   Really, it is unlikely that xmit_lock protection is necessary here.
-	   (f.e. loopback and IP tunnels are clean ignoring statistics
+	   Really, it is unlikely that netif_tx_lock protection is necessary
+	   here.  (f.e. loopback and IP tunnels are clean ignoring statistics
 	   counters.)
 	   However, it is possible, that they rely on protection
 	   made by us here.
@@ -2785,7 +2783,7 @@ int register_netdevice(struct net_device
 	BUG_ON(dev->reg_state != NETREG_UNINITIALIZED);
 
 	spin_lock_init(&dev->queue_lock);
-	spin_lock_init(&dev->xmit_lock);
+	spin_lock_init(&dev->_xmit_lock);
 	dev->xmit_lock_owner = -1;
 #ifdef CONFIG_NET_CLS_ACT
 	spin_lock_init(&dev->ingress_lock);
diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
index 05d6085..c57d887 100644
--- a/net/core/dev_mcast.c
+++ b/net/core/dev_mcast.c
@@ -62,7 +62,7 @@ #include <net/arp.h>
  *	Device mc lists are changed by bh at least if IPv6 is enabled,
  *	so that it must be bh protected.
  *
- *	We block accesses to device mc filters with dev->xmit_lock.
+ *	We block accesses to device mc filters with netif_tx_lock.
  */
 
 /*
@@ -93,9 +93,9 @@ static void __dev_mc_upload(struct net_d
 
 void dev_mc_upload(struct net_device *dev)
 {
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	__dev_mc_upload(dev);
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 }
 
 /*
@@ -107,7 +107,7 @@ int dev_mc_delete(struct net_device *dev
 	int err = 0;
 	struct dev_mc_list *dmi, **dmip;
 
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 
 	for (dmip = &dev->mc_list; (dmi = *dmip) != NULL; dmip = &dmi->next) {
 		/*
@@ -139,13 +139,13 @@ int dev_mc_delete(struct net_device *dev
 			 */
 			__dev_mc_upload(dev);
 			
-			spin_unlock_bh(&dev->xmit_lock);
+			netif_tx_unlock_bh(dev);
 			return 0;
 		}
 	}
 	err = -ENOENT;
 done:
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 	return err;
 }
 
@@ -160,7 +160,7 @@ int dev_mc_add(struct net_device *dev, v
 
 	dmi1 = kmalloc(sizeof(*dmi), GFP_ATOMIC);
 
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	for (dmi = dev->mc_list; dmi != NULL; dmi = dmi->next) {
 		if (memcmp(dmi->dmi_addr, addr, dmi->dmi_addrlen) == 0 &&
 		    dmi->dmi_addrlen == alen) {
@@ -176,7 +176,7 @@ int dev_mc_add(struct net_device *dev, v
 	}
 
 	if ((dmi = dmi1) == NULL) {
-		spin_unlock_bh(&dev->xmit_lock);
+		netif_tx_unlock_bh(dev);
 		return -ENOMEM;
 	}
 	memcpy(dmi->dmi_addr, addr, alen);
@@ -189,11 +189,11 @@ int dev_mc_add(struct net_device *dev, v
 
 	__dev_mc_upload(dev);
 	
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 	return 0;
 
 done:
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 	kfree(dmi1);
 	return err;
 }
@@ -204,7 +204,7 @@ done:
 
 void dev_mc_discard(struct net_device *dev)
 {
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	
 	while (dev->mc_list != NULL) {
 		struct dev_mc_list *tmp = dev->mc_list;
@@ -215,7 +215,7 @@ void dev_mc_discard(struct net_device *d
 	}
 	dev->mc_count = 0;
 
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 }
 
 #ifdef CONFIG_PROC_FS
@@ -250,7 +250,7 @@ static int dev_mc_seq_show(struct seq_fi
 	struct dev_mc_list *m;
 	struct net_device *dev = v;
 
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	for (m = dev->mc_list; m; m = m->next) {
 		int i;
 
@@ -262,7 +262,7 @@ static int dev_mc_seq_show(struct seq_fi
 
 		seq_putc(seq, '\n');
 	}
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 	return 0;
 }
 
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index e8e05ce..9cb7818 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -273,24 +273,21 @@ static void netpoll_send_skb(struct netp
 
 	do {
 		npinfo->tries--;
-		spin_lock(&np->dev->xmit_lock);
-		np->dev->xmit_lock_owner = smp_processor_id();
+		netif_tx_lock(np->dev);
 
 		/*
 		 * network drivers do not expect to be called if the queue is
 		 * stopped.
 		 */
 		if (netif_queue_stopped(np->dev)) {
-			np->dev->xmit_lock_owner = -1;
-			spin_unlock(&np->dev->xmit_lock);
+			netif_tx_unlock(np->dev);
 			netpoll_poll(np);
 			udelay(50);
 			continue;
 		}
 
 		status = np->dev->hard_start_xmit(skb, np->dev);
-		np->dev->xmit_lock_owner = -1;
-		spin_unlock(&np->dev->xmit_lock);
+		netif_tx_unlock(np->dev);
 
 		/* success */
 		if(!status) {
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index c23e9c0..67ed14d 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -2897,7 +2897,7 @@ static __inline__ void pktgen_xmit(struc
 		}
 	}
 
-	spin_lock_bh(&odev->xmit_lock);
+	netif_tx_lock_bh(odev);
 	if (!netif_queue_stopped(odev)) {
 
 		atomic_inc(&(pkt_dev->skb->users));
@@ -2942,7 +2942,7 @@ static __inline__ void pktgen_xmit(struc
 		pkt_dev->next_tx_ns = 0;
 	}
 
-	spin_unlock_bh(&odev->xmit_lock);
+	netif_tx_unlock_bh(odev);
 
 	/* If pkt_dev->count is zero, then run forever */
 	if ((pkt_dev->count != 0) && (pkt_dev->sofar >= pkt_dev->count)) {
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 138ea92..b1e4c5e 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -72,9 +72,9 @@ void qdisc_unlock_tree(struct net_device
    dev->queue_lock serializes queue accesses for this device
    AND dev->qdisc pointer itself.
 
-   dev->xmit_lock serializes accesses to device driver.
+   netif_tx_lock serializes accesses to device driver.
 
-   dev->queue_lock and dev->xmit_lock are mutually exclusive,
+   dev->queue_lock and netif_tx_lock are mutually exclusive,
    if one is grabbed, another must be free.
  */
 
@@ -108,7 +108,7 @@ int qdisc_restart(struct net_device *dev
 		 * will be requeued.
 		 */
 		if (!nolock) {
-			if (!spin_trylock(&dev->xmit_lock)) {
+			if (!netif_tx_trylock(dev)) {
 			collision:
 				/* So, someone grabbed the driver. */
 				
@@ -126,8 +126,6 @@ int qdisc_restart(struct net_device *dev
 				__get_cpu_var(netdev_rx_stat).cpu_collision++;
 				goto requeue;
 			}
-			/* Remember that the driver is grabbed by us. */
-			dev->xmit_lock_owner = smp_processor_id();
 		}
 		
 		{
@@ -142,8 +140,7 @@ int qdisc_restart(struct net_device *dev
 				ret = dev->hard_start_xmit(skb, dev);
 				if (ret == NETDEV_TX_OK) { 
 					if (!nolock) {
-						dev->xmit_lock_owner = -1;
-						spin_unlock(&dev->xmit_lock);
+						netif_tx_unlock(dev);
 					}
 					spin_lock(&dev->queue_lock);
 					return -1;
@@ -157,8 +154,7 @@ int qdisc_restart(struct net_device *dev
 			/* NETDEV_TX_BUSY - we need to requeue */
 			/* Release the driver */
 			if (!nolock) { 
-				dev->xmit_lock_owner = -1;
-				spin_unlock(&dev->xmit_lock);
+				netif_tx_unlock(dev);
 			} 
 			spin_lock(&dev->queue_lock);
 			q = dev->qdisc;
@@ -187,7 +183,7 @@ static void dev_watchdog(unsigned long a
 {
 	struct net_device *dev = (struct net_device *)arg;
 
-	spin_lock(&dev->xmit_lock);
+	netif_tx_lock(dev);
 	if (dev->qdisc != &noop_qdisc) {
 		if (netif_device_present(dev) &&
 		    netif_running(dev) &&
@@ -203,7 +199,7 @@ static void dev_watchdog(unsigned long a
 				dev_hold(dev);
 		}
 	}
-	spin_unlock(&dev->xmit_lock);
+	netif_tx_unlock(dev);
 
 	dev_put(dev);
 }
@@ -227,17 +223,17 @@ void __netdev_watchdog_up(struct net_dev
 
 static void dev_watchdog_up(struct net_device *dev)
 {
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	__netdev_watchdog_up(dev);
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 }
 
 static void dev_watchdog_down(struct net_device *dev)
 {
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	if (del_timer(&dev->watchdog_timer))
 		dev_put(dev);
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 }
 
 void netif_carrier_on(struct net_device *dev)
@@ -582,7 +578,7 @@ void dev_deactivate(struct net_device *d
 	while (test_bit(__LINK_STATE_SCHED, &dev->state))
 		yield();
 
-	spin_unlock_wait(&dev->xmit_lock);
+	spin_unlock_wait(&dev->_xmit_lock);
 }
 
 void dev_init_scheduler(struct net_device *dev)
diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index 79b8ef3..4c16ad5 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -302,20 +302,17 @@ restart:
 
 		switch (teql_resolve(skb, skb_res, slave)) {
 		case 0:
-			if (spin_trylock(&slave->xmit_lock)) {
-				slave->xmit_lock_owner = smp_processor_id();
+			if (netif_tx_trylock(slave)) {
 				if (!netif_queue_stopped(slave) &&
 				    slave->hard_start_xmit(skb, slave) == 0) {
-					slave->xmit_lock_owner = -1;
-					spin_unlock(&slave->xmit_lock);
+					netif_tx_unlock(slave);
 					master->slaves = NEXT_SLAVE(q);
 					netif_wake_queue(dev);
 					master->stats.tx_packets++;
 					master->stats.tx_bytes += len;
 					return 0;
 				}
-				slave->xmit_lock_owner = -1;
-				spin_unlock(&slave->xmit_lock);
+				netif_tx_unlock(slave);
 			}
 			if (netif_queue_stopped(dev))
 				busy = 1;

Re: [NET]: Add netif_tx_lock

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 1 Jun 2006 21:15:04 +1000

> On Thu, Jun 01, 2006 at 10:25:25AM +1000, herbert wrote:
> > 
> > > I think this netpoll wrinkle means we also have to make
> > > sure to set the xmit_lock_owner across the board.
> > 
> > You're right.  In fact this can deadlock today for those drivers that
> > already make use of xmit_lock without setting the owner.  So I suppose
> > something like net_xmit_lock to obtain xmit_lock is called for.
> 
> OK, here is a patch which does this.
> 
> [NET]: Add netif_tx_lock

IPOIB is going to BUG() with this change.  Because now, in their
multicast code, you're going to local_bh_disable() via
netif_tx_unlock() with hw IRQs disabled which is illegal.

It shows a bug here in the locking of the IPOIB driver.

We need to think about this change some more. :)

Re: [NET]: Add netif_tx_lock

[Roland Dreier]

 > IPOIB is going to BUG() with this change.  Because now, in their
 > multicast code, you're going to local_bh_disable() via
 > netif_tx_unlock() with hw IRQs disabled which is illegal.
 > 
 > It shows a bug here in the locking of the IPOIB driver.

Sorry, I haven't followed this thread closely.  Can you expand on what
the bug in ipoib's multicast locking is?

Thanks,
  Roland

Re: [NET]: Add netif_tx_lock

[Roland Dreier]

    Roland> Sorry, I haven't followed this thread closely.  Can you
    Roland> expand on what the bug in ipoib's multicast locking is?

You know, looking at the ipoib code, I can't even recreate why
xmit_lock is taken in the set_multicast_list method anyway, or how it
works at all -- it seems set_multicast_list will always be called with
xmit_lock already held.  What the heck is going on?

 - R.

Re: [NET]: Add netif_tx_lock

[Roland Dreier]

    Roland> You know, looking at the ipoib code, I can't even recreate
    Roland> why xmit_lock is taken in the set_multicast_list method
    Roland> anyway, or how it works at all -- it seems
    Roland> set_multicast_list will always be called with xmit_lock
    Roland> already held.  What the heck is going on?

Never mind -- I see that the set_multicast_list work needs to be
deferred to process context, so ipoib_mcast_restart_task() doesn't run
directly from the call to set_multicast_list.

I guess the fix in the current kernel is just something like the
below.  And in the netif_tx_lock() patch, the local_irq_save() /
local_irq_restore() calls can just be removed.

Am I on the right track?

Anyway I won't push the patch below since the bug is harmless right
now and it can be fixed up as part of the netif_tx_lock() patch.

Thanks,
  Roland

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index ec41c8f..5f3eaf1 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -814,13 +814,12 @@ void ipoib_mcast_restart_task(void *dev_
 	struct dev_mc_list *mclist;
 	struct ipoib_mcast *mcast, *tmcast;
 	LIST_HEAD(remove_list);
-	unsigned long flags;
 
 	ipoib_dbg_mcast(priv, "restarting multicast task\n");
 
 	ipoib_mcast_stop_thread(dev, 0);
 
-	spin_lock_irqsave(&dev->xmit_lock, flags);
+	spin_lock_bh(&dev->xmit_lock);
 	spin_lock(&priv->lock);
 
 	/*
@@ -895,7 +894,7 @@ void ipoib_mcast_restart_task(void *dev_
 	}
 
 	spin_unlock(&priv->lock);
-	spin_unlock_irqrestore(&dev->xmit_lock, flags);
+	spin_unlock_bh(&dev->xmit_lock);
 
 	/* We have to cancel outside of the spinlock */
 	list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {

Re: [NET]: Add netif_tx_lock

[David Miller]

From: Roland Dreier <rdreier@cisco.com>
Date: Mon, 05 Jun 2006 21:57:51 -0700

>     Roland> You know, looking at the ipoib code, I can't even recreate
>     Roland> why xmit_lock is taken in the set_multicast_list method
>     Roland> anyway, or how it works at all -- it seems
>     Roland> set_multicast_list will always be called with xmit_lock
>     Roland> already held.  What the heck is going on?
> 
> Never mind -- I see that the set_multicast_list work needs to be
> deferred to process context, so ipoib_mcast_restart_task() doesn't run
> directly from the call to set_multicast_list.
> 
> I guess the fix in the current kernel is just something like the
> below.  And in the netif_tx_lock() patch, the local_irq_save() /
> local_irq_restore() calls can just be removed.
> 
> Am I on the right track?
> 
> Anyway I won't push the patch below since the bug is harmless right
> now and it can be fixed up as part of the netif_tx_lock() patch.

As long as you never take priv->lock while ->xmit_lock is held
your patch should be OK.

Re: [NET]: Add netif_tx_lock

[Roland Dreier]

    David> As long as you never take priv->lock while ->xmit_lock is
    David> held your patch should be OK.

Duh ... unfortunately priv->lock is taken from interrupt context so
that patch isn't safe.  A correct fix would be the following, which
leads to a trivial conversion to using netif_tx_lock().

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 1dae4b2..7a8ca5d 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -821,8 +821,8 @@ void ipoib_mcast_restart_task(void *dev_
 
 	ipoib_mcast_stop_thread(dev, 0);
 
-	spin_lock_irqsave(&dev->xmit_lock, flags);
-	spin_lock(&priv->lock);
+	spin_lock_bh(&dev->xmit_lock);
+	spin_lock_irqsave(&priv->lock, flags);
 
 	/*
 	 * Unfortunately, the networking core only gives us a list of all of
@@ -895,8 +895,8 @@ void ipoib_mcast_restart_task(void *dev_
 		}
 	}
 
-	spin_unlock(&priv->lock);
-	spin_unlock_irqrestore(&dev->xmit_lock, flags);
+	spin_unlock_irqrestore(&priv->lock, flags);
+	spin_unlock_bh(&dev->xmit_lock);
 
 	/* We have to cancel outside of the spinlock */
 	list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {

Re: [NET]: Add netif_tx_lock

[David Miller]

From: Roland Dreier <rdreier@cisco.com>
Date: Mon, 05 Jun 2006 21:50:11 -0700

>     Roland> Sorry, I haven't followed this thread closely.  Can you
>     Roland> expand on what the bug in ipoib's multicast locking is?
> 
> You know, looking at the ipoib code, I can't even recreate why
> xmit_lock is taken in the set_multicast_list method anyway, or how it
> works at all -- it seems set_multicast_list will always be called with
> xmit_lock already held.  What the heck is going on?

Isn't the IPOIB driver LLTX and therefore the upper layers are
not taking the xmit_lock?

Re: [NET]: Add netif_tx_lock

[Roland Dreier]

    David> Isn't the IPOIB driver LLTX and therefore the upper layers
    David> are not taking the xmit_lock?

Yeah, but I think that should be OK.  If I'm remembering the intention
of the code correctly, the reason xmit_lock is being taken there is
just to protect dev->mc_list -- and this is needed because ipoib has
to defer handling set_multicast_list to process context.  Am I right
in thinking that dev->xmit_lock still serializes mc_list operations,
even for LLTX drivers?

 - R.

Re: [NET]: Add netif_tx_lock

[David Miller]

From: Roland Dreier <rdreier@cisco.com>
Date: Mon, 05 Jun 2006 22:04:34 -0700

> Am I right in thinking that dev->xmit_lock still serializes mc_list
> operations, even for LLTX drivers?

Correct.

Re: [NET]: Add netif_tx_lock

[David Miller]

From: Roland Dreier <rdreier@cisco.com>
Date: Mon, 05 Jun 2006 21:44:01 -0700

>  > IPOIB is going to BUG() with this change.  Because now, in their
>  > multicast code, you're going to local_bh_disable() via
>  > netif_tx_unlock() with hw IRQs disabled which is illegal.
>  > 
>  > It shows a bug here in the locking of the IPOIB driver.
> 
> Sorry, I haven't followed this thread closely.  Can you expand on what
> the bug in ipoib's multicast locking is?

It disables hw interrupts, and takes the xmit_lock.

xmit_lock is a BH only spinlock, you can't take it from hw
interrupts, or inside of a HW irq protected spinlock.

You can take it on the outside of a HW irq lock protected
area like this:

	spin_lock_bh(&dev->xmit_lock);

	spin_lock_irq(&whatever_lock);

	spin_unlock_irq(&whatever_lock);

	spin_unlock_bh(&dev->xmit_lock);

but not the other way around.

Re: [NET]: Add netif_tx_lock

[Herbert Xu]

On Mon, Jun 05, 2006 at 09:32:50PM -0700, David Miller wrote:
> 
> IPOIB is going to BUG() with this change.  Because now, in their
> multicast code, you're going to local_bh_disable() via
> netif_tx_unlock() with hw IRQs disabled which is illegal.
> 
> It shows a bug here in the locking of the IPOIB driver.

You had me woried there.

> We need to think about this change some more. :)

I thought about it a bit more and I'm not worried anymore :)

Notice that the patch does netif_tx_lock/netif_tx_unlock
for IB instead of netif_tx_lock_bh/netif_tx_unlock_bh.
So there is no BH enabling going on at all.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [NET]: Add netif_tx_lock

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

On Thu, Jun 01, 2006 at 09:15:03PM +1000, herbert wrote:
> 
> OK, here is a patch which does this.
> 
> [NET]: Add netif_tx_lock

Just noticed that I showed dyslexia in winbond.c :) Here is the corrected
version.

[NET]: Add netif_tx_lock

Various drivers use xmit_lock internally to synchronise with their
transmission routines.  They do so without setting xmit_lock_owner.
This is fine as long as netpoll is not in use.

With netpoll it is possible for deadlocks to occur if xmit_lock_owner
isn't set.  This is because if a printk occurs while xmit_lock is held
and xmit_lock_owner is not set can cause netpoll to attempt to take
xmit_lock recursively.

While it is possible to resolve this by getting netpoll to use trylock,
it is suboptimal because netpoll's sole objective is to maximise the
chance of getting the printk out on the wire.  So delaying or dropping
the message is to be avoided as much as possible.

So the only alternative is to always set xmit_lock_owner.  The following
patch does this by introducing the netif_tx_lock family of functions that 
take care of setting/unsetting xmit_lock_owner.

I renamed xmit_lock to _xmit_lock to indicate that it should not be used
directly.  I didn't provide irq versions of the netif_tx_lock functions
since xmit_lock is meant to be a BH-disabling lock.

This is pretty much a straight text substitution except for a small bug
fix in winbond.  It currently uses netif_stop_queue/spin_unlock_wait to
stop transmission.  This is unsafe as an IRQ can potentially wake up the
queue.  So it is safer to use netif_tx_disable.

The hamradio bits used spin_lock_irq but it is unnecessary as xmit_lock
must never be taken in an IRQ handler.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: netif-tx-lock-2.patch --]
[-- Type: text/plain, Size: 23979 bytes --]

diff --git a/Documentation/networking/netdevices.txt b/Documentation/networking/netdevices.txt
index 3c0a5ba..847cedb 100644
--- a/Documentation/networking/netdevices.txt
+++ b/Documentation/networking/netdevices.txt
@@ -42,9 +42,9 @@ dev->get_stats:
 	Context: nominally process, but don't sleep inside an rwlock
 
 dev->hard_start_xmit:
-	Synchronization: dev->xmit_lock spinlock.
+	Synchronization: netif_tx_lock spinlock.
 	When the driver sets NETIF_F_LLTX in dev->features this will be
-	called without holding xmit_lock. In this case the driver 
+	called without holding netif_tx_lock. In this case the driver
 	has to lock by itself when needed. It is recommended to use a try lock
 	for this and return -1 when the spin lock fails. 
 	The locking there should also properly protect against 
@@ -62,12 +62,12 @@ dev->hard_start_xmit:
 	  Only valid when NETIF_F_LLTX is set.
 
 dev->tx_timeout:
-	Synchronization: dev->xmit_lock spinlock.
+	Synchronization: netif_tx_lock spinlock.
 	Context: BHs disabled
 	Notes: netif_queue_stopped() is guaranteed true
 
 dev->set_multicast_list:
-	Synchronization: dev->xmit_lock spinlock.
+	Synchronization: netif_tx_lock spinlock.
 	Context: BHs disabled
 
 dev->poll:
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 1dae4b2..1d917ed 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -821,7 +821,8 @@ void ipoib_mcast_restart_task(void *dev_
 
 	ipoib_mcast_stop_thread(dev, 0);
 
-	spin_lock_irqsave(&dev->xmit_lock, flags);
+	local_irq_save(flags);
+	netif_tx_lock(dev);
 	spin_lock(&priv->lock);
 
 	/*
@@ -896,7 +897,8 @@ void ipoib_mcast_restart_task(void *dev_
 	}
 
 	spin_unlock(&priv->lock);
-	spin_unlock_irqrestore(&dev->xmit_lock, flags);
+	netif_tx_unlock(dev);
+	local_irq_restore(flags);
 
 	/* We have to cancel outside of the spinlock */
 	list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {
diff --git a/drivers/media/dvb/dvb-core/dvb_net.c b/drivers/media/dvb/dvb-core/dvb_net.c
index 2f0f358..9fd8752 100644
--- a/drivers/media/dvb/dvb-core/dvb_net.c
+++ b/drivers/media/dvb/dvb-core/dvb_net.c
@@ -1052,7 +1052,7 @@ static void wq_set_multicast_list (void 
 
 	dvb_net_feed_stop(dev);
 	priv->rx_mode = RX_MODE_UNI;
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 
 	if (dev->flags & IFF_PROMISC) {
 		dprintk("%s: promiscuous mode\n", dev->name);
@@ -1077,7 +1077,7 @@ static void wq_set_multicast_list (void 
 		}
 	}
 
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 	dvb_net_feed_start(dev);
 }
 
diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 54161ae..9c5a884 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -2009,7 +2009,7 @@ bnx2_poll(struct net_device *dev, int *b
 	return 1;
 }
 
-/* Called with rtnl_lock from vlan functions and also dev->xmit_lock
+/* Called with rtnl_lock from vlan functions and also netif_tx_lock
  * from set_multicast.
  */
 static void
@@ -4252,7 +4252,7 @@ bnx2_vlan_rx_kill_vid(struct net_device 
 }
 #endif
 
-/* Called with dev->xmit_lock.
+/* Called with netif_tx_lock.
  * hard_start_xmit is pseudo-lockless - a lock is only required when
  * the tx queue is full. This way, we get the benefit of lockless
  * operations most of the time without the complexities to handle
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 55d2367..46326cd 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4191,7 +4191,7 @@ static int bond_init(struct net_device *
 	 */
 	bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
 
-	/* don't acquire bond device's xmit_lock when 
+	/* don't acquire bond device's netif_tx_lock when
 	 * transmitting */
 	bond_dev->features |= NETIF_F_LLTX;
 
diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index feb5b22..5a8651b 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -533,9 +533,9 @@ #define NV_MSI_X_VECTOR_OTHER 0x2
  * critical parts:
  * - rx is (pseudo-) lockless: it relies on the single-threading provided
  *	by the arch code for interrupts.
- * - tx setup is lockless: it relies on dev->xmit_lock. Actual submission
+ * - tx setup is lockless: it relies on netif_tx_lock. Actual submission
  *	needs dev->priv->lock :-(
- * - set_multicast_list: preparation lockless, relies on dev->xmit_lock.
+ * - set_multicast_list: preparation lockless, relies on netif_tx_lock.
  */
 
 /* in dev: base, irq */
@@ -1213,7 +1213,7 @@ static void drain_ring(struct net_device
 
 /*
  * nv_start_xmit: dev->hard_start_xmit function
- * Called with dev->xmit_lock held.
+ * Called with netif_tx_lock held.
  */
 static int nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
@@ -1407,7 +1407,7 @@ static void nv_tx_done(struct net_device
 
 /*
  * nv_tx_timeout: dev->tx_timeout function
- * Called with dev->xmit_lock held.
+ * Called with netif_tx_lock held.
  */
 static void nv_tx_timeout(struct net_device *dev)
 {
@@ -1737,7 +1737,7 @@ static int nv_change_mtu(struct net_devi
 		 * Changing the MTU is a rare event, it shouldn't matter.
 		 */
 		nv_disable_irq(dev);
-		spin_lock_bh(&dev->xmit_lock);
+		netif_tx_lock_bh(dev);
 		spin_lock(&np->lock);
 		/* stop engines */
 		nv_stop_rx(dev);
@@ -1768,7 +1768,7 @@ static int nv_change_mtu(struct net_devi
 		nv_start_rx(dev);
 		nv_start_tx(dev);
 		spin_unlock(&np->lock);
-		spin_unlock_bh(&dev->xmit_lock);
+		netif_tx_unlock_bh(dev);
 		nv_enable_irq(dev);
 	}
 	return 0;
@@ -1803,7 +1803,7 @@ static int nv_set_mac_address(struct net
 	memcpy(dev->dev_addr, macaddr->sa_data, ETH_ALEN);
 
 	if (netif_running(dev)) {
-		spin_lock_bh(&dev->xmit_lock);
+		netif_tx_lock_bh(dev);
 		spin_lock_irq(&np->lock);
 
 		/* stop rx engine */
@@ -1815,7 +1815,7 @@ static int nv_set_mac_address(struct net
 		/* restart rx engine */
 		nv_start_rx(dev);
 		spin_unlock_irq(&np->lock);
-		spin_unlock_bh(&dev->xmit_lock);
+		netif_tx_unlock_bh(dev);
 	} else {
 		nv_copy_mac_to_hw(dev);
 	}
@@ -1824,7 +1824,7 @@ static int nv_set_mac_address(struct net
 
 /*
  * nv_set_multicast: dev->set_multicast function
- * Called with dev->xmit_lock held.
+ * Called with netif_tx_lock held.
  */
 static void nv_set_multicast(struct net_device *dev)
 {
diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index 102c1f0..d12605f 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c
@@ -308,9 +308,9 @@ static int sp_set_mac_address(struct net
 {
 	struct sockaddr_ax25 *sa = addr;
 
-	spin_lock_irq(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	memcpy(dev->dev_addr, &sa->sax25_call, AX25_ADDR_LEN);
-	spin_unlock_irq(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 
 	return 0;
 }
@@ -767,9 +767,9 @@ static int sixpack_ioctl(struct tty_stru
 			break;
 		}
 
-		spin_lock_irq(&dev->xmit_lock);
+		netif_tx_lock_bh(dev);
 		memcpy(dev->dev_addr, &addr, AX25_ADDR_LEN);
-		spin_unlock_irq(&dev->xmit_lock);
+		netif_tx_unlock_bh(dev);
 
 		err = 0;
 		break;
diff --git a/drivers/net/hamradio/mkiss.c b/drivers/net/hamradio/mkiss.c
index d81a8e1..3ebbbe5 100644
--- a/drivers/net/hamradio/mkiss.c
+++ b/drivers/net/hamradio/mkiss.c
@@ -357,9 +357,9 @@ static int ax_set_mac_address(struct net
 {
 	struct sockaddr_ax25 *sa = addr;
 
-	spin_lock_irq(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	memcpy(dev->dev_addr, &sa->sax25_call, AX25_ADDR_LEN);
-	spin_unlock_irq(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 
 	return 0;
 }
@@ -886,9 +886,9 @@ static int mkiss_ioctl(struct tty_struct
 			break;
 		}
 
-		spin_lock_irq(&dev->xmit_lock);
+		netif_tx_lock_bh(dev);
 		memcpy(dev->dev_addr, addr, AX25_ADDR_LEN);
-		spin_unlock_irq(&dev->xmit_lock);
+		netif_tx_unlock_bh(dev);
 
 		err = 0;
 		break;
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 31fb2d7..2e222ef 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -76,13 +76,13 @@ static void ri_tasklet(unsigned long dev
 	dp->st_task_enter++;
 	if ((skb = skb_peek(&dp->tq)) == NULL) {
 		dp->st_txq_refl_try++;
-		if (spin_trylock(&_dev->xmit_lock)) {
+		if (netif_tx_trylock(_dev)) {
 			dp->st_rxq_enter++;
 			while ((skb = skb_dequeue(&dp->rq)) != NULL) {
 				skb_queue_tail(&dp->tq, skb);
 				dp->st_rx2tx_tran++;
 			}
-			spin_unlock(&_dev->xmit_lock);
+			netif_tx_unlock(_dev);
 		} else {
 			/* reschedule */
 			dp->st_rxq_notenter++;
@@ -110,7 +110,7 @@ static void ri_tasklet(unsigned long dev
 		}
 	}
 
-	if (spin_trylock(&_dev->xmit_lock)) {
+	if (netif_tx_trylock(_dev)) {
 		dp->st_rxq_check++;
 		if ((skb = skb_peek(&dp->rq)) == NULL) {
 			dp->tasklet_pending = 0;
@@ -118,10 +118,10 @@ static void ri_tasklet(unsigned long dev
 				netif_wake_queue(_dev);
 		} else {
 			dp->st_rxq_rsch++;
-			spin_unlock(&_dev->xmit_lock);
+			netif_tx_unlock(_dev);
 			goto resched;
 		}
-		spin_unlock(&_dev->xmit_lock);
+		netif_tx_unlock(_dev);
 	} else {
 resched:
 		dp->tasklet_pending = 1;
diff --git a/drivers/net/irda/vlsi_ir.c b/drivers/net/irda/vlsi_ir.c
index 97a49e0..d70b9e8 100644
--- a/drivers/net/irda/vlsi_ir.c
+++ b/drivers/net/irda/vlsi_ir.c
@@ -959,7 +959,7 @@ static int vlsi_hard_start_xmit(struct s
 			    ||  (now.tv_sec==ready.tv_sec && now.tv_usec>=ready.tv_usec))
 			    	break;
 			udelay(100);
-			/* must not sleep here - we are called under xmit_lock! */
+			/* must not sleep here - called under netif_tx_lock! */
 		}
 	}
 
diff --git a/drivers/net/natsemi.c b/drivers/net/natsemi.c
index 9062775..2e4eced 100644
--- a/drivers/net/natsemi.c
+++ b/drivers/net/natsemi.c
@@ -318,12 +318,12 @@ performance critical codepaths:
 The rx process only runs in the interrupt handler. Access from outside
 the interrupt handler is only permitted after disable_irq().
 
-The rx process usually runs under the dev->xmit_lock. If np->intr_tx_reap
+The rx process usually runs under the netif_tx_lock. If np->intr_tx_reap
 is set, then access is permitted under spin_lock_irq(&np->lock).
 
 Thus configuration functions that want to access everything must call
 	disable_irq(dev->irq);
-	spin_lock_bh(dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	spin_lock_irq(&np->lock);
 
 IV. Notes
diff --git a/drivers/net/tulip/winbond-840.c b/drivers/net/tulip/winbond-840.c
index 136a70c..56d86c7 100644
--- a/drivers/net/tulip/winbond-840.c
+++ b/drivers/net/tulip/winbond-840.c
@@ -1605,11 +1605,11 @@ #ifdef CONFIG_PM
  * - get_stats:
  * 	spin_lock_irq(np->lock), doesn't touch hw if not present
  * - hard_start_xmit:
- * 	netif_stop_queue + spin_unlock_wait(&dev->xmit_lock);
+ * 	synchronize_irq + netif_tx_disable;
  * - tx_timeout:
- * 	netif_device_detach + spin_unlock_wait(&dev->xmit_lock);
+ * 	netif_device_detach + netif_tx_disable;
  * - set_multicast_list
- * 	netif_device_detach + spin_unlock_wait(&dev->xmit_lock);
+ * 	netif_device_detach + netif_tx_disable;
  * - interrupt handler
  * 	doesn't touch hw if not present, synchronize_irq waits for
  * 	running instances of the interrupt handler.
@@ -1635,11 +1635,10 @@ static int w840_suspend (struct pci_dev 
 		netif_device_detach(dev);
 		update_csr6(dev, 0);
 		iowrite32(0, ioaddr + IntrEnable);
-		netif_stop_queue(dev);
 		spin_unlock_irq(&np->lock);
 
-		spin_unlock_wait(&dev->xmit_lock);
 		synchronize_irq(dev->irq);
+		netif_tx_disable(dev);
 	
 		np->stats.rx_missed_errors += ioread32(ioaddr + RxMissed) & 0xffff;
 
diff --git a/drivers/net/wireless/orinoco.c b/drivers/net/wireless/orinoco.c
index c2d0b09..a5fcfcd 100644
--- a/drivers/net/wireless/orinoco.c
+++ b/drivers/net/wireless/orinoco.c
@@ -1833,7 +1833,9 @@ static int __orinoco_program_rids(struct
 	/* Set promiscuity / multicast*/
 	priv->promiscuous = 0;
 	priv->mc_count = 0;
-	__orinoco_set_multicast_list(dev); /* FIXME: what about the xmit_lock */
+
+	/* FIXME: what about netif_tx_lock */
+	__orinoco_set_multicast_list(dev);
 
 	return 0;
 }
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f4169bb..01699eb 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -406,7 +406,7 @@ #define NETIF_F_UFO             8192    
  * One part is mostly used on xmit path (device)
  */
 	/* hard_start_xmit synchronizer */
-	spinlock_t		xmit_lock ____cacheline_aligned_in_smp;
+	spinlock_t		_xmit_lock ____cacheline_aligned_in_smp;
 	/* cpu id of processor entered to hard_start_xmit or -1,
 	   if nobody entered there.
 	 */
@@ -889,11 +889,43 @@ static inline void __netif_rx_complete(s
 	clear_bit(__LINK_STATE_RX_SCHED, &dev->state);
 }
 
+static inline void netif_tx_lock(struct net_device *dev)
+{
+	spin_lock(&dev->_xmit_lock);
+	dev->xmit_lock_owner = smp_processor_id();
+}
+
+static inline void netif_tx_lock_bh(struct net_device *dev)
+{
+	spin_lock_bh(&dev->_xmit_lock);
+	dev->xmit_lock_owner = smp_processor_id();
+}
+
+static inline int netif_tx_trylock(struct net_device *dev)
+{
+	int err = spin_trylock(&dev->_xmit_lock);
+	if (!err)
+		dev->xmit_lock_owner = smp_processor_id();
+	return err;
+}
+
+static inline void netif_tx_unlock(struct net_device *dev)
+{
+	dev->xmit_lock_owner = -1;
+	spin_unlock(&dev->_xmit_lock);
+}
+
+static inline void netif_tx_unlock_bh(struct net_device *dev)
+{
+	dev->xmit_lock_owner = -1;
+	spin_unlock_bh(&dev->_xmit_lock);
+}
+
 static inline void netif_tx_disable(struct net_device *dev)
 {
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	netif_stop_queue(dev);
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 }
 
 /* These functions live elsewhere (drivers/net/net_init.c, but related) */
diff --git a/net/atm/clip.c b/net/atm/clip.c
index 72d8529..f92f9c9 100644
--- a/net/atm/clip.c
+++ b/net/atm/clip.c
@@ -98,7 +98,7 @@ static void unlink_clip_vcc(struct clip_
 		printk(KERN_CRIT "!clip_vcc->entry (clip_vcc %p)\n", clip_vcc);
 		return;
 	}
-	spin_lock_bh(&entry->neigh->dev->xmit_lock);	/* block clip_start_xmit() */
+	netif_tx_lock_bh(entry->neigh->dev);	/* block clip_start_xmit() */
 	entry->neigh->used = jiffies;
 	for (walk = &entry->vccs; *walk; walk = &(*walk)->next)
 		if (*walk == clip_vcc) {
@@ -122,7 +122,7 @@ static void unlink_clip_vcc(struct clip_
 	printk(KERN_CRIT "ATMARP: unlink_clip_vcc failed (entry %p, vcc "
 	       "0x%p)\n", entry, clip_vcc);
       out:
-	spin_unlock_bh(&entry->neigh->dev->xmit_lock);
+	netif_tx_unlock_bh(entry->neigh->dev);
 }
 
 /* The neighbour entry n->lock is held. */
diff --git a/net/core/dev.c b/net/core/dev.c
index 4fba549..8906f59 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1275,15 +1275,13 @@ int __skb_linearize(struct sk_buff *skb,
 
 #define HARD_TX_LOCK(dev, cpu) {			\
 	if ((dev->features & NETIF_F_LLTX) == 0) {	\
-		spin_lock(&dev->xmit_lock);		\
-		dev->xmit_lock_owner = cpu;		\
+		netif_tx_lock(dev);			\
 	}						\
 }
 
 #define HARD_TX_UNLOCK(dev) {				\
 	if ((dev->features & NETIF_F_LLTX) == 0) {	\
-		dev->xmit_lock_owner = -1;		\
-		spin_unlock(&dev->xmit_lock);		\
+		netif_tx_unlock(dev);			\
 	}						\
 }
 
@@ -1382,8 +1380,8 @@ #endif
 	/* The device has no queue. Common case for software devices:
 	   loopback, all the sorts of tunnels...
 
-	   Really, it is unlikely that xmit_lock protection is necessary here.
-	   (f.e. loopback and IP tunnels are clean ignoring statistics
+	   Really, it is unlikely that netif_tx_lock protection is necessary
+	   here.  (f.e. loopback and IP tunnels are clean ignoring statistics
 	   counters.)
 	   However, it is possible, that they rely on protection
 	   made by us here.
@@ -2785,7 +2783,7 @@ int register_netdevice(struct net_device
 	BUG_ON(dev->reg_state != NETREG_UNINITIALIZED);
 
 	spin_lock_init(&dev->queue_lock);
-	spin_lock_init(&dev->xmit_lock);
+	spin_lock_init(&dev->_xmit_lock);
 	dev->xmit_lock_owner = -1;
 #ifdef CONFIG_NET_CLS_ACT
 	spin_lock_init(&dev->ingress_lock);
diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
index 05d6085..c57d887 100644
--- a/net/core/dev_mcast.c
+++ b/net/core/dev_mcast.c
@@ -62,7 +62,7 @@ #include <net/arp.h>
  *	Device mc lists are changed by bh at least if IPv6 is enabled,
  *	so that it must be bh protected.
  *
- *	We block accesses to device mc filters with dev->xmit_lock.
+ *	We block accesses to device mc filters with netif_tx_lock.
  */
 
 /*
@@ -93,9 +93,9 @@ static void __dev_mc_upload(struct net_d
 
 void dev_mc_upload(struct net_device *dev)
 {
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	__dev_mc_upload(dev);
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 }
 
 /*
@@ -107,7 +107,7 @@ int dev_mc_delete(struct net_device *dev
 	int err = 0;
 	struct dev_mc_list *dmi, **dmip;
 
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 
 	for (dmip = &dev->mc_list; (dmi = *dmip) != NULL; dmip = &dmi->next) {
 		/*
@@ -139,13 +139,13 @@ int dev_mc_delete(struct net_device *dev
 			 */
 			__dev_mc_upload(dev);
 			
-			spin_unlock_bh(&dev->xmit_lock);
+			netif_tx_unlock_bh(dev);
 			return 0;
 		}
 	}
 	err = -ENOENT;
 done:
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 	return err;
 }
 
@@ -160,7 +160,7 @@ int dev_mc_add(struct net_device *dev, v
 
 	dmi1 = kmalloc(sizeof(*dmi), GFP_ATOMIC);
 
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	for (dmi = dev->mc_list; dmi != NULL; dmi = dmi->next) {
 		if (memcmp(dmi->dmi_addr, addr, dmi->dmi_addrlen) == 0 &&
 		    dmi->dmi_addrlen == alen) {
@@ -176,7 +176,7 @@ int dev_mc_add(struct net_device *dev, v
 	}
 
 	if ((dmi = dmi1) == NULL) {
-		spin_unlock_bh(&dev->xmit_lock);
+		netif_tx_unlock_bh(dev);
 		return -ENOMEM;
 	}
 	memcpy(dmi->dmi_addr, addr, alen);
@@ -189,11 +189,11 @@ int dev_mc_add(struct net_device *dev, v
 
 	__dev_mc_upload(dev);
 	
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 	return 0;
 
 done:
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 	kfree(dmi1);
 	return err;
 }
@@ -204,7 +204,7 @@ done:
 
 void dev_mc_discard(struct net_device *dev)
 {
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	
 	while (dev->mc_list != NULL) {
 		struct dev_mc_list *tmp = dev->mc_list;
@@ -215,7 +215,7 @@ void dev_mc_discard(struct net_device *d
 	}
 	dev->mc_count = 0;
 
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 }
 
 #ifdef CONFIG_PROC_FS
@@ -250,7 +250,7 @@ static int dev_mc_seq_show(struct seq_fi
 	struct dev_mc_list *m;
 	struct net_device *dev = v;
 
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	for (m = dev->mc_list; m; m = m->next) {
 		int i;
 
@@ -262,7 +262,7 @@ static int dev_mc_seq_show(struct seq_fi
 
 		seq_putc(seq, '\n');
 	}
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 	return 0;
 }
 
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index e8e05ce..9cb7818 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -273,24 +273,21 @@ static void netpoll_send_skb(struct netp
 
 	do {
 		npinfo->tries--;
-		spin_lock(&np->dev->xmit_lock);
-		np->dev->xmit_lock_owner = smp_processor_id();
+		netif_tx_lock(np->dev);
 
 		/*
 		 * network drivers do not expect to be called if the queue is
 		 * stopped.
 		 */
 		if (netif_queue_stopped(np->dev)) {
-			np->dev->xmit_lock_owner = -1;
-			spin_unlock(&np->dev->xmit_lock);
+			netif_tx_unlock(np->dev);
 			netpoll_poll(np);
 			udelay(50);
 			continue;
 		}
 
 		status = np->dev->hard_start_xmit(skb, np->dev);
-		np->dev->xmit_lock_owner = -1;
-		spin_unlock(&np->dev->xmit_lock);
+		netif_tx_unlock(np->dev);
 
 		/* success */
 		if(!status) {
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index c23e9c0..67ed14d 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -2897,7 +2897,7 @@ static __inline__ void pktgen_xmit(struc
 		}
 	}
 
-	spin_lock_bh(&odev->xmit_lock);
+	netif_tx_lock_bh(odev);
 	if (!netif_queue_stopped(odev)) {
 
 		atomic_inc(&(pkt_dev->skb->users));
@@ -2942,7 +2942,7 @@ static __inline__ void pktgen_xmit(struc
 		pkt_dev->next_tx_ns = 0;
 	}
 
-	spin_unlock_bh(&odev->xmit_lock);
+	netif_tx_unlock_bh(odev);
 
 	/* If pkt_dev->count is zero, then run forever */
 	if ((pkt_dev->count != 0) && (pkt_dev->sofar >= pkt_dev->count)) {
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 138ea92..b1e4c5e 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -72,9 +72,9 @@ void qdisc_unlock_tree(struct net_device
    dev->queue_lock serializes queue accesses for this device
    AND dev->qdisc pointer itself.
 
-   dev->xmit_lock serializes accesses to device driver.
+   netif_tx_lock serializes accesses to device driver.
 
-   dev->queue_lock and dev->xmit_lock are mutually exclusive,
+   dev->queue_lock and netif_tx_lock are mutually exclusive,
    if one is grabbed, another must be free.
  */
 
@@ -108,7 +108,7 @@ int qdisc_restart(struct net_device *dev
 		 * will be requeued.
 		 */
 		if (!nolock) {
-			if (!spin_trylock(&dev->xmit_lock)) {
+			if (!netif_tx_trylock(dev)) {
 			collision:
 				/* So, someone grabbed the driver. */
 				
@@ -126,8 +126,6 @@ int qdisc_restart(struct net_device *dev
 				__get_cpu_var(netdev_rx_stat).cpu_collision++;
 				goto requeue;
 			}
-			/* Remember that the driver is grabbed by us. */
-			dev->xmit_lock_owner = smp_processor_id();
 		}
 		
 		{
@@ -142,8 +140,7 @@ int qdisc_restart(struct net_device *dev
 				ret = dev->hard_start_xmit(skb, dev);
 				if (ret == NETDEV_TX_OK) { 
 					if (!nolock) {
-						dev->xmit_lock_owner = -1;
-						spin_unlock(&dev->xmit_lock);
+						netif_tx_unlock(dev);
 					}
 					spin_lock(&dev->queue_lock);
 					return -1;
@@ -157,8 +154,7 @@ int qdisc_restart(struct net_device *dev
 			/* NETDEV_TX_BUSY - we need to requeue */
 			/* Release the driver */
 			if (!nolock) { 
-				dev->xmit_lock_owner = -1;
-				spin_unlock(&dev->xmit_lock);
+				netif_tx_unlock(dev);
 			} 
 			spin_lock(&dev->queue_lock);
 			q = dev->qdisc;
@@ -187,7 +183,7 @@ static void dev_watchdog(unsigned long a
 {
 	struct net_device *dev = (struct net_device *)arg;
 
-	spin_lock(&dev->xmit_lock);
+	netif_tx_lock(dev);
 	if (dev->qdisc != &noop_qdisc) {
 		if (netif_device_present(dev) &&
 		    netif_running(dev) &&
@@ -203,7 +199,7 @@ static void dev_watchdog(unsigned long a
 				dev_hold(dev);
 		}
 	}
-	spin_unlock(&dev->xmit_lock);
+	netif_tx_unlock(dev);
 
 	dev_put(dev);
 }
@@ -227,17 +223,17 @@ void __netdev_watchdog_up(struct net_dev
 
 static void dev_watchdog_up(struct net_device *dev)
 {
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	__netdev_watchdog_up(dev);
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 }
 
 static void dev_watchdog_down(struct net_device *dev)
 {
-	spin_lock_bh(&dev->xmit_lock);
+	netif_tx_lock_bh(dev);
 	if (del_timer(&dev->watchdog_timer))
 		dev_put(dev);
-	spin_unlock_bh(&dev->xmit_lock);
+	netif_tx_unlock_bh(dev);
 }
 
 void netif_carrier_on(struct net_device *dev)
@@ -582,7 +578,7 @@ void dev_deactivate(struct net_device *d
 	while (test_bit(__LINK_STATE_SCHED, &dev->state))
 		yield();
 
-	spin_unlock_wait(&dev->xmit_lock);
+	spin_unlock_wait(&dev->_xmit_lock);
 }
 
 void dev_init_scheduler(struct net_device *dev)
diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index 79b8ef3..4c16ad5 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -302,20 +302,17 @@ restart:
 
 		switch (teql_resolve(skb, skb_res, slave)) {
 		case 0:
-			if (spin_trylock(&slave->xmit_lock)) {
-				slave->xmit_lock_owner = smp_processor_id();
+			if (netif_tx_trylock(slave)) {
 				if (!netif_queue_stopped(slave) &&
 				    slave->hard_start_xmit(skb, slave) == 0) {
-					slave->xmit_lock_owner = -1;
-					spin_unlock(&slave->xmit_lock);
+					netif_tx_unlock(slave);
 					master->slaves = NEXT_SLAVE(q);
 					netif_wake_queue(dev);
 					master->stats.tx_packets++;
 					master->stats.tx_bytes += len;
 					return 0;
 				}
-				slave->xmit_lock_owner = -1;
-				spin_unlock(&slave->xmit_lock);
+				netif_tx_unlock(slave);
 			}
 			if (netif_queue_stopped(dev))
 				busy = 1;

Re: [NET]: Add netif_tx_lock

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 9 Jun 2006 15:48:16 +1000

> On Thu, Jun 01, 2006 at 09:15:03PM +1000, herbert wrote:
> > 
> > OK, here is a patch which does this.
> > 
> > [NET]: Add netif_tx_lock
> 
> Just noticed that I showed dyslexia in winbond.c :) Here is the corrected
> version.
> 
> [NET]: Add netif_tx_lock

:-)  Applied, thanks a lot Herbert.

Re: netif_tx_disable and lockless TX

[Michael Chan]

David Miller wrote:

> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Wed, 31 May 2006 14:58:11 +1000
> 
> > Yes, TG3 does not disable IRQs when taking its TX lock.  So do you
see
> > any problems with replacing the TG3 TX lock using xmit_lock?
> 
> I don't see any.
> 

I also don't see any problem. It looks like we don't have to set
xmit_lock_owner, right?

Re: netif_tx_disable and lockless TX

[Herbert Xu]

On Tue, May 30, 2006 at 10:30:25PM -0700, Michael Chan wrote:
> 
> I also don't see any problem. It looks like we don't have to set
> xmit_lock_owner, right?

Right, unless you want to call dev_queue_xmit recursively :)
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

netif_tx_disable and lockless TX

[Herbert Xu]

Hi:

It has occured to me that the function netif_tx_disable may be unsafe
when used in drivers that do lockless transmission.  In particular,
the function relies on taking the xmit_lock to ensure that no further
transmissions will occur until the queue is started again.

However, lockless drivers do not take the xmit_lock so this method
is ineffective.  Such drivers need to do their own checking inside
whatever locks that they do take.  For example, tg3 could get around
this by checking whether the queue is stopped in its hard_start_xmit
function.

I must say though that I'm becoming less and less impressed by the
lockless feature based on the number of problems that it has caused.
Does anyone have any hard figures as to its effectiveness (excluding
any stats relating to the loopback interface which can be easily
separated from normal NIC drivers).

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netif_tx_disable and lockless TX

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 31 May 2006 14:03:07 +1000

> I must say though that I'm becoming less and less impressed by the
> lockless feature based on the number of problems that it has caused.
> Does anyone have any hard figures as to its effectiveness (excluding
> any stats relating to the loopback interface which can be easily
> separated from normal NIC drivers).

There were numbers posted for real drivers at one point, I
do remember that, and the gains were non-trivial.  At least
enough to justify the feature.

As per the bug, I always keep thinking about changing how we
do the lockless stuff by simply making xmit_lock a pointer.
Drivers like tg3 can make it point to their local driver lock.

This leaves still the cases like loopback which do no locking
whatsoever.  I don't know how to fit it into the above idea.

Re: netif_tx_disable and lockless TX

[Herbert Xu]

On Tue, May 30, 2006 at 09:13:22PM -0700, David Miller wrote:
> 
> There were numbers posted for real drivers at one point, I
> do remember that, and the gains were non-trivial.  At least
> enough to justify the feature.

I wonder if there are other ways of achieving the same improvement.

> As per the bug, I always keep thinking about changing how we
> do the lockless stuff by simply making xmit_lock a pointer.
> Drivers like tg3 can make it point to their local driver lock.

This is equivalent to just using the xmit_lock in the driver, right?
IIRC the problem was with IRQ disabling vs. BH disabling.

That's why I suggest that every NIC that uses this feature be forced
to do what TG3 does so only BH disabling is needed.  Once that's done
they can just use xmit_lock and everyone will be happy again.

> This leaves still the cases like loopback which do no locking
> whatsoever.  I don't know how to fit it into the above idea.

As far as I'm concerned loopback can do whatever it wants as long as
it doesn't create bugs in real NICs :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netif_tx_disable and lockless TX

[Roland Dreier]

    Herbert> However, lockless drivers do not take the xmit_lock so
    Herbert> this method is ineffective.  Such drivers need to do
    Herbert> their own checking inside whatever locks that they do
    Herbert> take.  For example, tg3 could get around this by checking
    Herbert> whether the queue is stopped in its hard_start_xmit
    Herbert> function.

Yes, I had to add this to the IPoIB driver, because calling
netif_stop_queue() when the transmit ring was full was sometimes still
allowing hard_start_xmit to be called again:

	/*
	 * Check if our queue is stopped.  Since we have the LLTX bit
	 * set, we can't rely on netif_stop_queue() preventing our
	 * xmit function from being called with a full queue.
	 */
	if (unlikely(netif_queue_stopped(dev))) {
		spin_unlock_irqrestore(&priv->tx_lock, flags);
		return NETDEV_TX_BUSY;
	}

this bug started a long thread a while back, but I don't remember if
there was any resolution.

    Herbert> I must say though that I'm becoming less and less
    Herbert> impressed by the lockless feature based on the number of
    Herbert> problems that it has caused.  Does anyone have any hard
    Herbert> figures as to its effectiveness (excluding any stats
    Herbert> relating to the loopback interface which can be easily
    Herbert> separated from normal NIC drivers).

I don't have exact figures at hand, but I remember something like a 2 or
3 percent throughput improvement for IPoIB.

 - R.