| Message ID | 1325643618-3051-1-git-send-email-maxim.uvarov@oracle.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Maxim Uvarov <maxim.uvarov@oracle.com> Date: Tue, 3 Jan 2012 18:20:18 -0800 > Do not disable BH if interrupts are already disabled > (netpoll case). > Signed-off-by: Maxim Uvarov <maxim.uvarov@oracle.com> Barf... We should never use conditional locking like this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03.01.2012 18:49, David Miller wrote: > From: Maxim Uvarov<maxim.uvarov@oracle.com> > Date: Tue, 3 Jan 2012 18:20:18 -0800 > >> Do not disable BH if interrupts are already disabled >> (netpoll case). >> Signed-off-by: Maxim Uvarov<maxim.uvarov@oracle.com> > Barf... > > We should never use conditional locking like this. How about change spin_lock_bh to spin_lock_irqsave at this place? Maxim. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Maxim Uvarov <maxim.uvarov@oracle.com> Date: Wed, 04 Jan 2012 00:14:39 -0800 > On 03.01.2012 18:49, David Miller wrote: >> From: Maxim Uvarov<maxim.uvarov@oracle.com> >> Date: Tue, 3 Jan 2012 18:20:18 -0800 >> >>> Do not disable BH if interrupts are already disabled >>> (netpoll case). >>> Signed-off-by: Maxim Uvarov<maxim.uvarov@oracle.com> >> Barf... >> >> We should never use conditional locking like this. > > > How about change spin_lock_bh to spin_lock_irqsave at this place? Then it's ambiguous whether it's a softirq safe lock or a hardirq safe one. It's just another way to make the locking inconsistent. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/04/2012 10:25 AM, David Miller wrote: > From: Maxim Uvarov<maxim.uvarov@oracle.com> > Date: Wed, 04 Jan 2012 00:14:39 -0800 > >> On 03.01.2012 18:49, David Miller wrote: >>> From: Maxim Uvarov<maxim.uvarov@oracle.com> >>> Date: Tue, 3 Jan 2012 18:20:18 -0800 >>> >>>> Do not disable BH if interrupts are already disabled >>>> (netpoll case). >>>> Signed-off-by: Maxim Uvarov<maxim.uvarov@oracle.com> >>> Barf... >>> >>> We should never use conditional locking like this. >> >> >> How about change spin_lock_bh to spin_lock_irqsave at this place? > > Then it's ambiguous whether it's a softirq safe lock or a hardirq > safe one. > > It's just another way to make the locking inconsistent. at bond_start_xmit() there is check if it's netpoll or not: /* * If we risk deadlock from transmitting this in the * netpoll path, tell netpoll to queue the frame for later tx */ if (is_netpoll_tx_blocked(dev)) return NETDEV_TX_BUSY; which is in the end: static inline int netpoll_tx_running(struct net_device *dev) { return irqs_disabled(); } So the original patch was in the way as it already implemented. BTW, I'm trying to remove warning generated by local_bh_enable_ip: http://marc.info/?l=linux-netdev&m=132528368523980&w=2 Maxim. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c index d4fbd2e..69eeb36 100644 --- a/drivers/net/bonding/bond_alb.c +++ b/drivers/net/bonding/bond_alb.c @@ -101,12 +101,18 @@ static inline u8 _simple_hash(const u8 *hash_start, int hash_size) static inline void _lock_tx_hashtbl(struct bonding *bond) { - spin_lock_bh(&(BOND_ALB_INFO(bond).tx_hashtbl_lock)); + if (unlikely(irqs_disabled())) /*netpoll case*/ + spin_lock(&(BOND_ALB_INFO(bond).tx_hashtbl_lock)); + else + spin_lock_bh(&(BOND_ALB_INFO(bond).tx_hashtbl_lock)); } static inline void _unlock_tx_hashtbl(struct bonding *bond) { - spin_unlock_bh(&(BOND_ALB_INFO(bond).tx_hashtbl_lock)); + if (unlikely(irqs_disabled())) /*netpoll case*/ + spin_unlock(&(BOND_ALB_INFO(bond).tx_hashtbl_lock)); + else + spin_unlock_bh(&(BOND_ALB_INFO(bond).tx_hashtbl_lock)); } /* Caller must hold tx_hashtbl lock */
Do not disable BH if interrupts are already disabled (netpoll case). Signed-off-by: Maxim Uvarov <maxim.uvarov@oracle.com> --- drivers/net/bonding/bond_alb.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)