Re: [NET]: Add netif_tx_lock
From: Herbert Xu <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [NET]: Add netif_tx_lock
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 <[EMAIL PROTECTED]>
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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);
}
Re: [NET]: Add netif_tx_lock
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~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [NET]: Add netif_tx_lock
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) {
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [NET]: Add netif_tx_lock
From: Roland Dreier <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [NET]: Add netif_tx_lock
From: Roland Dreier <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [NET]: Add netif_tx_lock
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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [NET]: Add netif_tx_lock
From: Roland Dreier <[EMAIL PROTECTED]> 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? - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [NET]: Add netif_tx_lock
From: Roland Dreier <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [NET]: Add netif_tx_lock
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) {
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [NET]: Add netif_tx_lock
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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [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. Sorry, I haven't followed this thread closely. Can you expand on what the bug in ipoib's multicast locking is? Thanks, Roland - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [NET]: Add netif_tx_lock
From: Herbert Xu <[EMAIL PROTECTED]> 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. :) - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[NET]: Add netif_tx_lock
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 <[EMAIL PROTECTED]>
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 m
