* [PATCH] bond_alb: do not disable BH under netpoll
@ 2012-01-04 2:20 Maxim Uvarov
2012-01-04 2:49 ` David Miller
0 siblings, 1 reply; 5+ messages in thread
From: Maxim Uvarov @ 2012-01-04 2:20 UTC (permalink / raw)
To: netdev; +Cc: Maxim Uvarov
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(-)
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 */
--
1.7.4.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] bond_alb: do not disable BH under netpoll
2012-01-04 2:20 [PATCH] bond_alb: do not disable BH under netpoll Maxim Uvarov
@ 2012-01-04 2:49 ` David Miller
2012-01-04 8:14 ` Maxim Uvarov
0 siblings, 1 reply; 5+ messages in thread
From: David Miller @ 2012-01-04 2:49 UTC (permalink / raw)
To: maxim.uvarov; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] bond_alb: do not disable BH under netpoll
2012-01-04 2:49 ` David Miller
@ 2012-01-04 8:14 ` Maxim Uvarov
2012-01-04 18:25 ` David Miller
0 siblings, 1 reply; 5+ messages in thread
From: Maxim Uvarov @ 2012-01-04 8:14 UTC (permalink / raw)
To: David Miller; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] bond_alb: do not disable BH under netpoll
2012-01-04 8:14 ` Maxim Uvarov
@ 2012-01-04 18:25 ` David Miller
2012-01-04 19:35 ` Maxim Uvarov
0 siblings, 1 reply; 5+ messages in thread
From: David Miller @ 2012-01-04 18:25 UTC (permalink / raw)
To: maxim.uvarov; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] bond_alb: do not disable BH under netpoll
2012-01-04 18:25 ` David Miller
@ 2012-01-04 19:35 ` Maxim Uvarov
0 siblings, 0 replies; 5+ messages in thread
From: Maxim Uvarov @ 2012-01-04 19:35 UTC (permalink / raw)
To: David Miller; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-01-04 19:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-04 2:20 [PATCH] bond_alb: do not disable BH under netpoll Maxim Uvarov
2012-01-04 2:49 ` David Miller
2012-01-04 8:14 ` Maxim Uvarov
2012-01-04 18:25 ` David Miller
2012-01-04 19:35 ` Maxim Uvarov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).