From fb4fa76a1fa59340154c42d998d700e1f8bf21e0 Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Mon, 6 Dec 2010 09:05:50 +0000 Subject: net: Convert netpoll blocking api in bonding driver to be a counter A while back I made some changes to enable netpoll in the bonding driver. Among them was a per-cpu flag that indicated we were in a path that held locks which could cause the netpoll path to block in during tx, and as such the tx path should queue the frame for later use. This appears to have given rise to a regression. If one of those paths on which we hold the per-cpu flag yields the cpu, its possible for us to come back on a different cpu, leading to us clearing a different flag than we set. This results in odd netpoll drops, and BUG backtraces appearing in the log, as we check to make sure that we only clear set bits, and only set clear bits. I had though briefly about changing the offending paths so that they wouldn't sleep, but looking at my origional work more closely, it doesn't appear that a per-cpu flag is warranted. We alrady gate the checking of this flag on IFF_IN_NETPOLL, so we don't hit this in the normal tx case anyway. And practically speaking, the normal use case for netpoll is to only have one client anyway, so we're not going to erroneously queue netpoll frames when its actually safe to do so. As such, lets just convert that per-cpu flag to an atomic counter. It fixes the rescheduling bugs, is equivalent from a performance perspective and actually eliminates some code in the process. Tested by the reporter and myself, successfully Reported-by: Liang Zheng CC: Jay Vosburgh CC: Andy Gospodarek CC: David S. Miller Signed-off-by: Neil Horman Signed-off-by: David S. Miller --- drivers/net/bonding/bond_main.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) (limited to 'drivers/net/bonding/bond_main.c') diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 2fee00a4c9ef..d0ea760ce419 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -171,7 +171,7 @@ MODULE_PARM_DESC(resend_igmp, "Number of IGMP membership reports to send on link /*----------------------------- Global variables ----------------------------*/ #ifdef CONFIG_NET_POLL_CONTROLLER -cpumask_var_t netpoll_block_tx; +atomic_t netpoll_block_tx = ATOMIC_INIT(0); #endif static const char * const version = @@ -5299,13 +5299,6 @@ static int __init bonding_init(void) if (res) goto out; -#ifdef CONFIG_NET_POLL_CONTROLLER - if (!alloc_cpumask_var(&netpoll_block_tx, GFP_KERNEL)) { - res = -ENOMEM; - goto out; - } -#endif - res = register_pernet_subsys(&bond_net_ops); if (res) goto out; @@ -5334,9 +5327,6 @@ err: rtnl_link_unregister(&bond_link_ops); err_link: unregister_pernet_subsys(&bond_net_ops); -#ifdef CONFIG_NET_POLL_CONTROLLER - free_cpumask_var(netpoll_block_tx); -#endif goto out; } @@ -5353,7 +5343,10 @@ static void __exit bonding_exit(void) unregister_pernet_subsys(&bond_net_ops); #ifdef CONFIG_NET_POLL_CONTROLLER - free_cpumask_var(netpoll_block_tx); + /* + * Make sure we don't have an imbalance on our netpoll blocking + */ + WARN_ON(atomic_read(&netpoll_block_tx)); #endif } -- cgit v1.2.3 From 8387451e558853f7b513790c0070e3b6f0c135aa Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Mon, 13 Dec 2010 08:19:28 +0000 Subject: bonding/vlan: Remove redundant VLAN tag insertion logic A bond may have a mixture of slave devices with and without hardware VLAN tag insertion capability. Therefore it always claims this capability and performs software VLAN tag insertion if the slave does not. Since commit 7b9c60903714bf0a19d746b228864bad3497284e, this has also been done by dev_hard_start_xmit(). The result is that VLAN- tagged skbs are now double-tagged when transmitted through slave devices without hardware VLAN tag insertion! Remove the now-redundant logic from bond_dev_queue_xmit(). Signed-off-by: Ben Hutchings Signed-off-by: Jay Vosburgh Reviewed-by: Jesse Gross Signed-off-by: David S. Miller --- drivers/net/bonding/bond_main.c | 27 +-------------------------- 1 file changed, 1 insertion(+), 26 deletions(-) (limited to 'drivers/net/bonding/bond_main.c') diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index d0ea760ce419..ef370c923341 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -418,36 +418,11 @@ struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr) * @bond: bond device that got this skb for tx. * @skb: hw accel VLAN tagged skb to transmit * @slave_dev: slave that is supposed to xmit this skbuff - * - * When the bond gets an skb to transmit that is - * already hardware accelerated VLAN tagged, and it - * needs to relay this skb to a slave that is not - * hw accel capable, the skb needs to be "unaccelerated", - * i.e. strip the hwaccel tag and re-insert it as part - * of the payload. */ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, struct net_device *slave_dev) { - unsigned short uninitialized_var(vlan_id); - - /* Test vlan_list not vlgrp to catch and handle 802.1p tags */ - if (!list_empty(&bond->vlan_list) && - !(slave_dev->features & NETIF_F_HW_VLAN_TX) && - vlan_get_tag(skb, &vlan_id) == 0) { - skb->dev = slave_dev; - skb = vlan_put_tag(skb, vlan_id); - if (!skb) { - /* vlan_put_tag() frees the skb in case of error, - * so return success here so the calling functions - * won't attempt to free is again. - */ - return 0; - } - } else { - skb->dev = slave_dev; - } - + skb->dev = slave_dev; skb->priority = 1; #ifdef CONFIG_NET_POLL_CONTROLLER if (unlikely(bond->dev->priv_flags & IFF_IN_NETPOLL)) { -- cgit v1.2.3 From ffa95ed50f9fb2d8faaa6bd73086a7056ea46a06 Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Mon, 13 Dec 2010 08:19:56 +0000 Subject: bonding: Change active slave quietly when bond is down bond_change_active_slave() may be called when a slave is added, even if the bond has not been brought up yet. It may then attempt to send packets, and further it may use mcast_work which is uninitialised before the bond is brought up. Add the necessary checks for netif_running(bond->dev). Signed-off-by: Ben Hutchings Signed-off-by: Jay Vosburgh Signed-off-by: David S. Miller --- drivers/net/bonding/bond_main.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) (limited to 'drivers/net/bonding/bond_main.c') diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index ef370c923341..3b16c34ed86e 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1178,11 +1178,13 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active) bond_do_fail_over_mac(bond, new_active, old_active); - bond->send_grat_arp = bond->params.num_grat_arp; - bond_send_gratuitous_arp(bond); + if (netif_running(bond->dev)) { + bond->send_grat_arp = bond->params.num_grat_arp; + bond_send_gratuitous_arp(bond); - bond->send_unsol_na = bond->params.num_unsol_na; - bond_send_unsolicited_na(bond); + bond->send_unsol_na = bond->params.num_unsol_na; + bond_send_unsolicited_na(bond); + } write_unlock_bh(&bond->curr_slave_lock); read_unlock(&bond->lock); @@ -1196,8 +1198,9 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active) /* resend IGMP joins since active slave has changed or * all were sent on curr_active_slave */ - if ((USES_PRIMARY(bond->params.mode) && new_active) || - bond->params.mode == BOND_MODE_ROUNDROBIN) { + if (((USES_PRIMARY(bond->params.mode) && new_active) || + bond->params.mode == BOND_MODE_ROUNDROBIN) && + netif_running(bond->dev)) { bond->igmp_retrans = bond->params.resend_igmp; queue_delayed_work(bond->wq, &bond->mcast_work, 0); } -- cgit v1.2.3