net/bonding: ADD IPoIB support for the bonding driver
Subsystems (4)
v3 cover net/bonding: ADD IPoIB support for the bonding driver
This patch series is the third version (see below link to V2) of the suggested changes to the bonding driver so it would be able to support non ARPHRD_ETHER netdevices for its High-Availability (active-backup) mode.
The motivation is to enable the bonding driver on its HA mode to work with the IP over Infiniband (IPoIB) driver. With these patches I was able to enslave IPoIB netdevices and run TCP, UDP, IP (UDP) Multicast and ICMP traffic with fail-over and fail-back working fine. The working environment was the net-2.6 git.
More over, as IPoIB is also the IB ARP provider for the RDMA CM driver which is used by native IB ULPs whose addressing scheme is based on IP (e.g. iSER, SDP, Lustre, NFSoRDMA, RDS), bonding support for IPoIB devices **enables** HA for these ULPs. This holds as when the ULP is informed by the IB HW on the failure of the current IB connection, it just need to reconnect, where the bonding device will now issue the IB ARP over the active IPoIB slave.
This series also includes patches to the IPoIB driver that fix some fix some neighboring related issues.
There are still 2 open issues here:
1. When bonding enslaves an IPoIB device the bonding neighbor holds a reference to a cleanup function in the IPoIB drives. This makes it unsafe to unload the IPoIB module if there are bonding neighbors in the air. So, to avoid this race one must unload bonding before unloading IPoIB.
2. Patch No. 7 is a workaround to a problem where gratuitous were not sent quite often. I didn't find something better that fixes this and I would
appreciate advices and comments regarding it. However, this doesn't seem to me as an issue related exclusively to IPoIB.
Links to earlier discussion:
1. A discussion in netdev about bonding support for IPoIB.
http://lists.openwall.net/netdev/2006/11/30/46
2. A discussion in openfabrics regarding changes in the IPoIB that enable using it as a slave for bonding.
http://lists.openfabrics.org/pipermail/general/2007-March/034033.html
PATCH v3 1/7 IB/ipoib: Bound the net device to the ipoib_neigh structue
IPoIB uses a two layer neighboring scheme, such that for each struct neighbour whose device is an ipoib one, there is a struct ipoib_neigh buddy which is created on demand at the tx flow by an ipoib_neigh_alloc(skb->dst->neighbour) call.
When using the bonding driver, neighbours are created by the net stack on behalf of the bonding (master) device. On the tx flow the bonding code gets an skb such that skb->dev points to the master device, it changes this skb to point on the slave device and calls the slave hard_start_xmit function.
Under this scheme, ipoib_neigh_destructor assumption that for each struct neighbour it gets, n->dev is an ipoib device and hence netdev_priv(n->dev) can be casted to struct ipoib_dev_priv is buggy.
To fix it, this patch adds a dev field to struct ipoib_neigh which is used instead of the struct neighbour dev one, when n->dev->flags has the IFF_MASTER bit set.
--- drivers/infiniband/ulp/ipoib/ipoib.h | 4 +++- drivers/infiniband/ulp/ipoib/ipoib_main.c | 17 +++++++++++++++-- drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 2 +- 3 files changed, 19 insertions(+), 4 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib.h +3 -1
@@ -328,6 +328,7 @@ struct ipoib_neigh {struct sk_buff_head queue;struct neighbour *neighbour; +struct net_device *dev;struct list_head list; };
@@ -344,7 +345,8 @@ static inline struct ipoib_neigh **to_ip INFINIBAND_ALEN, sizeof(void *)); } -struct ipoib_neigh *ipoib_neigh_alloc (struct neighbour *neigh); +struct ipoib_neigh *ipoib_neigh_alloc (struct neighbour *neigh, +struct net_device *dev);void ipoib_neigh_free (struct net_device *dev,struct ipoib_neigh *neigh);extern struct workqueue_struct *ipoib_workqueue;
drivers/infiniband/ulp/ipoib/ipoib_main.c +15 -2
@@ -510,7 +510,7 @@ static void neigh_add_path(struct sk_bufstruct ipoib_path *path;struct ipoib_neigh *neigh; - neigh =ipoib_neigh_alloc (skb->dst->neighbour); + neigh =ipoib_neigh_alloc (skb->dst->neighbour, skb->dev);if (!neigh) { ++priv->stats.tx_dropped;dev_kfree_skb_any (skb);
@@ -817,6 +817,16 @@ static void ipoib_neigh_cleanup(struct nunsigned long flags;struct ipoib_ah *ah =NULL ; +if (n->dev->flags &IFF_MASTER ) { +/* n->dev is not an IPoIB device and we have to take priv from elsewhere */ + neigh = *to_ipoib_neigh (n); +if (neigh){ + priv =netdev_priv (neigh->dev); +ipoib_dbg (priv,"neigh_destructor for bonding device: %s\n" , + n->dev->name); + }else +return ; + } ipoib_dbg(priv, "neigh_cleanup for %06x " IPOIB_GID_FMT "\n", IPOIB_QPN(n->ha),
@@ -838,7 +848,9 @@ static void ipoib_neigh_cleanup(struct nipoib_put_ah (ah); } -struct ipoib_neigh *ipoib_neigh_alloc (struct neighbour *neighbour) +struct ipoib_neigh *ipoib_neigh_alloc (struct neighbour *neighbour, +struct net_device *dev) + {struct ipoib_neigh *neigh;
@@ -847,6 +859,7 @@ struct ipoib_neigh *ipoib_neigh_alloc(streturn NULL ; neigh->neighbour = neighbour; + neigh->dev = dev; *to_ipoib_neigh (neighbour) = neigh;skb_queue_head_init (&neigh->queue);ipoib_cm_set (neigh,NULL );
drivers/infiniband/ulp/ipoib/ipoib_multicast.c +1 -1
@@ -727,7 +727,7 @@ out:if (skb->dst && skb->dst->neighbour && !*to_ipoib_neigh (skb->dst->neighbour)) { -struct ipoib_neigh *neigh =ipoib_neigh_alloc (skb->dst->neighbour); +struct ipoib_neigh *neigh =ipoib_neigh_alloc (skb->dst->neighbour, skb->dev);if (neigh) {kref_get (&mcast->ah->ref);
PATCH v3 2/7 IB/ipoib: Verify address handle validity on send
In reply to Moni ShouaWhen the bonding device senses a carrier loss of its active slave it replaces that slave with a new one. In between the times when the carrier of an IPoIB device goes down and ipoib_neigh is destroyed, it is possible that the bonding driver will send a packet on a new slave that uses an old ipoib_neigh. This patch detects and prevents this from happenning.
--- drivers/infiniband/ulp/ipoib/ipoib_main.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_main.c +3 -2
@@ -685,9 +685,10 @@ static int ipoib_start_xmit(struct sk_bu goto out; } }else if (neigh->ah) { -if (unlikely (memcmp (&neigh->dgid.raw, +if (unlikely ((memcmp (&neigh->dgid.raw, skb->dst->neighbour->ha +4 , -sizeof (union ib_gid )))) { +sizeof (union ib_gid ))) || + (neigh->dev != dev))) {spin_lock (&priv->lock);/* * It's safe to call ipoib_put_ah() inside
PATCH v3 3/7 net/bonding: Enable bonding to enslave non ARPHRD_ETHER
In reply to Moni ShouaThis patch changes some of the bond netdevice attributes and functions to be that of the active slave for the case of the enslaved device not being of ARPHRD_ETHER type. Basically it overrides those setting done by ether_setup(), which are netdevice **type** dependent and hence might be not appropriate for devices of other types. It also enforces mutual exclusion on bonding slaves from dissimilar ether types, as was concluded over the v1 discussion.
IPoIB (see Documentation/infiniband/ipoib.txt) MAC address is made of a 3 bytes IB QP (Queue Pair) number and 16 bytes IB port GID (Global ID) of the port this IPoIB device is bounded to. The QP is a resource created by the IB HW and the GID is an identifier burned into the HCA (i have omitted here some details which are not important for the bonding RFC).
--- drivers/net/bonding/bond_main.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 files changed, 38 insertions(+)
drivers/net/bonding/bond_main.c +38 -0
@@ -1277,6 +1277,26 @@ static int bond_compute_features(structreturn 0 ; } + +static void bond_setup_by_slave (struct net_device *bond_dev, +struct net_device *slave_dev) +{ + bond_dev->hard_header = slave_dev->hard_header; + bond_dev->rebuild_header = slave_dev->rebuild_header; + bond_dev->hard_header_cache = slave_dev->hard_header_cache; + bond_dev->header_cache_update = slave_dev->header_cache_update; + bond_dev->hard_header_parse = slave_dev->hard_header_parse; + + bond_dev->neigh_setup = slave_dev->neigh_setup; + + bond_dev->type = slave_dev->type; + bond_dev->hard_header_len = slave_dev->hard_header_len; + bond_dev->addr_len = slave_dev->addr_len; + +memcpy (bond_dev->broadcast, slave_dev->broadcast, + slave_dev->addr_len); +} +/* enslave device <slave> to bond device <master> */ int bond_enslave (struct net_device *bond_dev,struct net_device *slave_dev) {
@@ -1351,6 +1371,24 @@ int bond_enslave(struct net_device *bond goto err_undo_flags; } +/* set bonding device ether type by slave - bonding netdevices are +* created with ether_setup, so when the slave type is not ARPHRD_ETHER +* there is a need to override some of the type dependent attribs/funcs. +* +* bond ether type mutual exclusion - don't allow slaves of dissimilar +* ether type (eg ARPHRD_ETHER and ARPHRD_INFINIBAND) share the same bond +*/ +if (bond->slave_cnt ==0 ) { +if (slave_dev->type !=ARPHRD_ETHER ) +bond_setup_by_slave (bond_dev, slave_dev); + }else if (bond_dev->type != slave_dev->type) { + printk(KERN_ERR DRV_NAME ": %s ether type (%d) is different from " +"other slaves (%d), can not enslave it.\n" , slave_dev->name, + slave_dev->type, bond_dev->type); + res = -EINVAL ; + goto err_undo_flags; + } +if (slave_dev->set_mac_address ==NULL ) { printk(KERN_ERR DRV_NAME": %s: Error: The slave device you specified does "
PATCH v3 4/7 net/bonding: Enable bonding to enslave netdevices not supporting set_mac_address()
In reply to Moni ShouaThis patch allows for enslaving netdevices which do not support the set_mac_address() function. In that case the bond mac address is the one of the active slave, where remote peers are notified on the mac address (neighbour) change by Gratuitous ARP sent by bonding when fail-over occurs (this is already done by the bonding code).
--- drivers/net/bonding/bond_main.c | 88 +++++++++++++++++++++++++++------------- drivers/net/bonding/bonding.h | 1
2 files changed, 61 insertions(+), 28 deletions(-)
drivers/net/bonding/bond_main.c +60 -28
@@ -1127,6 +1127,14 @@ void bond_change_active_slave(struct bonif (new_active) {bond_set_slave_active_flags (new_active); } + +/* when bonding does not set the slave MAC address, the bond MAC +* address is the one of the active slave. +*/ +if (new_active && !bond->do_set_mac_addr) +memcpy (bond->dev->dev_addr, new_active->dev->dev_addr, + new_active->dev->addr_len); +bond_send_gratuitous_arp (bond); } }
@@ -1390,13 +1398,22 @@ int bond_enslave(struct net_device *bond }if (slave_dev->set_mac_address ==NULL ) { - printk(KERN_ERR DRV_NAME -": %s: Error: The slave device you specified does " -"not support setting the MAC address. " -"Your kernel likely does not support slave " - "devices.\n", bond_dev->name); - res = -EOPNOTSUPP ; - goto err_undo_flags; +if (bond->slave_cnt ==0 ) { + printk(KERN_WARNING DRV_NAME +": %s: Warning: The first slave device you " +"specified does not support setting the MAC " +"address. This bond MAC address would be that " + "of the active slave.\n", bond_dev->name); + bond->do_set_mac_addr =0 ; + }else if (bond->do_set_mac_addr) { + printk(KERN_ERR DRV_NAME +": %s: Error: The slave device you specified " +"does not support setting the MAC addres,." +"but this bond uses this practice. \n" + , bond_dev->name); + res = -EOPNOTSUPP ; + goto err_undo_flags; + } } new_slave =kzalloc (sizeof (struct slave ),GFP_KERNEL );
@@ -1417,16 +1434,18 @@ int bond_enslave(struct net_device *bond*/ memcpy (new_slave->perm_hwaddr, slave_dev->dev_addr,ETH_ALEN ); -/* -* Set slave to master's mac address. The application already -* set the master's mac address to that of the first slave -*/ -memcpy (addr.sa_data, bond_dev->dev_addr, bond_dev->addr_len); - addr.sa_family = slave_dev->type; - res =dev_set_mac_address (slave_dev, &addr); -if (res) { -dprintk ("Error %d calling set_mac_address\n" , res); - goto err_free; +if (bond->do_set_mac_addr) { +/* +* Set slave to master's mac address. The application already +* set the master's mac address to that of the first slave +*/ +memcpy (addr.sa_data, bond_dev->dev_addr, bond_dev->addr_len); + addr.sa_family = slave_dev->type; + res =dev_set_mac_address (slave_dev, &addr); +if (res) { +dprintk ("Error %d calling set_mac_address\n" , res); + goto err_free; + } } res =netdev_set_master (slave_dev, bond_dev);
@@ -1651,9 +1670,11 @@ err_close:dev_close (slave_dev); err_restore_mac: -memcpy (addr.sa_data, new_slave->perm_hwaddr,ETH_ALEN ); - addr.sa_family = slave_dev->type; -dev_set_mac_address (slave_dev, &addr); +if (bond->do_set_mac_addr) { +memcpy (addr.sa_data, new_slave->perm_hwaddr,ETH_ALEN ); + addr.sa_family = slave_dev->type; +dev_set_mac_address (slave_dev, &addr); + } err_free:kfree (new_slave);
@@ -1831,10 +1852,12 @@ int bond_release(struct net_device *bond/* close slave before restoring its mac address */ dev_close (slave_dev); -/* restore original ("permanent") mac address */ -memcpy (addr.sa_data, slave->perm_hwaddr,ETH_ALEN ); - addr.sa_family = slave_dev->type; -dev_set_mac_address (slave_dev, &addr); +if (bond->do_set_mac_addr) { +/* restore original ("permanent") mac address */ +memcpy (addr.sa_data, slave->perm_hwaddr,ETH_ALEN ); + addr.sa_family = slave_dev->type; +dev_set_mac_address (slave_dev, &addr); + } slave_dev->priv_flags &= ~(IFF_MASTER_8023AD | IFF_MASTER_ALB | IFF_SLAVE_INACTIVE | IFF_BONDING |
@@ -1921,10 +1944,12 @@ static int bond_release_all(struct net_d/* close slave before restoring its mac address */ dev_close (slave_dev); -/* restore original ("permanent") mac address*/ -memcpy (addr.sa_data, slave->perm_hwaddr,ETH_ALEN ); - addr.sa_family = slave_dev->type; -dev_set_mac_address (slave_dev, &addr); +if (bond->do_set_mac_addr) { +/* restore original ("permanent") mac address*/ +memcpy (addr.sa_data, slave->perm_hwaddr,ETH_ALEN ); + addr.sa_family = slave_dev->type; +dev_set_mac_address (slave_dev, &addr); + } slave_dev->priv_flags &= ~(IFF_MASTER_8023AD |IFF_MASTER_ALB |IFF_SLAVE_INACTIVE );
@@ -3961,6 +3986,10 @@ static int bond_set_mac_address(struct ndprintk ("bond=%p, name=%s\n" , bond, (bond_dev ? bond_dev->name :"None" )); +if (!bond->do_set_mac_addr) { +return -EOPNOTSUPP ; + } +if (!is_valid_ether_addr (sa->sa_data)) {return -EADDRNOTAVAIL ; }
@@ -4351,6 +4380,9 @@ static int bond_init(struct net_device *bond_create_proc_entry (bond);#endif +/* set do_set_mac_addr to true on startup */ + bond->do_set_mac_addr =1 ; +list_add_tail (&bond->bond_list, &bond_dev_list);return 0 ;
drivers/net/bonding/bonding.h +1 -0
@@ -201,6 +201,7 @@ struct bonding {struct list_head vlan_list;struct vlan_group *vlgrp;struct packet_type arp_mon_pt; +s8 do_set_mac_addr; };/**
PATCH v3 5/7 net/bonding: Enable IP multicast for bonding IPoIB devices
In reply to Moni ShouaAllow to enslave devices when the bonding device is not up. Over the discussion held at the previous post this seemed to be the most clean way to go, where it is not expected to cause instabilities.
Normally, the bonding driver is UP before any enslavement takes place. Once a netdevice is UP, the network stack acts to have it join some multicast groups (eg the all-hosts 224.0.0.1). Now, since ether_setup() have set the bonding device type to be ARPHRD_ETHER and address len to be ETHER_ALEN, the net core code computes a wrong multicast link address. This is b/c ip_eth_mc_map() is called where for multicast joins taking place after the enslavement another ip_xxx_mc_map() is called (eg ip_ib_mc_map() when the bond type is ARPHRD_INFINIBAND)
--- drivers/net/bonding/bond_main.c | 4 ++-- drivers/net/bonding/bond_sysfs.c | 6 ++---- 2 files changed, 4 insertions(+), 6 deletions(-)
drivers/net/bonding/bond_main.c +2 -2
@@ -1325,8 +1325,8 @@ int bond_enslave(struct net_device *bond/* bond must be initialized by bond_open() before enslaving */ if (!(bond_dev->flags &IFF_UP )) { -dprintk ("Error, master_dev is not up\n" ); -return -EPERM ; + printk(KERN_WARNING DRV_NAME + " %s: master_dev is not up in bond_enslave\n", bond_dev->name); }/* already enslaved */
drivers/net/bonding/bond_sysfs.c +2 -4
@@ -266,11 +266,9 @@ static ssize_t bonding_store_slaves(stru/* Quick sanity check -- is the bond interface up? */ if (!(bond->dev->flags &IFF_UP )) { - printk(KERN_ERR DRV_NAME -": %s: Unable to update slaves because interface is down.\n" , + printk(KERN_WARNING DRV_NAME +": %s: doing slave updates when interface is down.\n" , bond->dev->name); - ret = -EPERM ; - goto out; }/* Note: We can't hold bond->lock here, as bond_create grabs it. */
PATCH v3 6/7 net/bonding: Handlle wrong assumptions that slave is always an Ethernet device
In reply to Moni Shouabonding sometimes uses Ethernet constants (such as MTU and address length) which are not good when it enslaves non Ethernet devices (such as InfiniBand).
--- drivers/net/bonding/bond_main.c | 2 +- drivers/net/bonding/bond_sysfs.c | 10 ++++++++-- drivers/net/bonding/bonding.h | 1 + 3 files changed, 10 insertions(+), 3 deletions(-)
drivers/net/bonding/bond_main.c +1 -1
@@ -1255,7 +1255,7 @@ static int bond_compute_features(structunsigned long features =BOND_INTERSECT_FEATURES ;struct slave *slave;struct net_device *bond_dev = bond->dev; -unsigned short max_hard_header_len =ETH_HLEN ; +u16 max_hard_header_len =max ((u16 )ETH_HLEN , bond_dev->hard_header_len);int i;bond_for_each_slave (bond, slave, i) {
drivers/net/bonding/bond_sysfs.c +8 -2
@@ -260,6 +260,7 @@ static ssize_t bonding_store_slaves(struchar command[IFNAMSIZ +1 ] = {0 , };char *ifname;int i, res, found, ret = count; +u32 original_mtu;struct slave *slave;struct net_device *dev =NULL ;struct bonding *bond =to_bond (d);
@@ -325,6 +326,7 @@ static ssize_t bonding_store_slaves(stru }/* Set the slave's MTU to match the bond */ + original_mtu = dev->mtu;if (dev->mtu != bond->dev->mtu) {if (dev->change_mtu) { res = dev->change_mtu(dev,
@@ -339,6 +341,9 @@ static ssize_t bonding_store_slaves(stru }rtnl_lock (); res =bond_enslave (bond->dev, dev); +bond_for_each_slave (bond, slave, i) +if (strnicmp (slave->dev->name, ifname,IFNAMSIZ ) ==0 ) + slave->original_mtu=original_mtu;rtnl_unlock ();if (res) { ret = res;
@@ -351,6 +356,7 @@ static ssize_t bonding_store_slaves(strubond_for_each_slave (bond, slave, i)if (strnicmp (slave->dev->name, ifname,IFNAMSIZ ) ==0 ) { dev = slave->dev; + original_mtu = slave->original_mtu;break ; }if (dev) {
@@ -365,9 +371,9 @@ static ssize_t bonding_store_slaves(stru }/* set the slave MTU to the default */ if (dev->change_mtu) { - dev->change_mtu (dev,1500 ); + dev->change_mtu (dev, original_mtu); }else { - dev->mtu =1500 ; + dev->mtu = original_mtu; } }else {
drivers/net/bonding/bonding.h +1 -0
@@ -156,6 +156,7 @@ struct slave {s8 link;/* one of BOND_LINK_XXXX */ s8 state;/* one of BOND_STATE_XXXX */ u32 original_flags; +u32 original_mtu;u32 link_failure_count;u16 speed;u8 duplex;
PATCH v3 7/7 net/bonding: Delay sending of gratuitous ARP to avoid failure
In reply to Moni ShouaDelay sending a gratuitous_arp when LINK_STATE_LINKWATCH_PENDING bit in dev->state field is on. This improves the chances for the arp packet to be transmitted.
--- drivers/net/bonding/bond_main.c | 25 +++++++++++++++++++++---- drivers/net/bonding/bonding.h | 1 + 2 files changed, 22 insertions(+), 4 deletions(-)
drivers/net/bonding/bond_main.c +21 -4
@@ -1134,8 +1134,13 @@ void bond_change_active_slave(struct bonif (new_active && !bond->do_set_mac_addr)memcpy (bond->dev->dev_addr, new_active->dev->dev_addr, new_active->dev->addr_len); - -bond_send_gratuitous_arp (bond); +if (bond->curr_active_slave && +test_bit (__LINK_STATE_LINKWATCH_PENDING, &bond->curr_active_slave->dev->state)){ +dprintk ("delaying gratuitous arp on %s\n" ,bond->curr_active_slave->dev->name); + bond->send_grat_arp=1 ; + }else { +bond_send_gratuitous_arp (bond); + } } }
@@ -2120,6 +2125,15 @@ void bond_mii_monitor(struct net_device* program could monitor the link itself if needed. */ +if (bond->send_grat_arp) { +if (bond->curr_active_slave &&test_bit (__LINK_STATE_LINKWATCH_PENDING, &bond->curr_active_slave->dev->state)) +dprintk ("Needs to send gratuitous arp but not yet\n" ,__FUNCTION__); +else { +dprintk ("sending delayed gratuitous arp on ond->curr_active_slave->dev->name\n" ); +bond_send_gratuitous_arp (bond); + bond->send_grat_arp=0 ; + } + }read_lock (&bond->curr_slave_lock); oldcurrent = bond->curr_active_slave;read_unlock (&bond->curr_slave_lock);
@@ -2513,6 +2527,7 @@ static void bond_send_gratuitous_arp(strstruct slave *slave = bond->curr_active_slave;struct vlan_entry *vlan;struct net_device *vlan_dev; +int i;dprintk ("bond_send_grat_arp: bond %s slave %s\n" , bond->dev->name, slave ? slave->dev->name :"NULL" );
@@ -2520,8 +2535,9 @@ static void bond_send_gratuitous_arp(strreturn ;if (bond->master_ip) { -bond_arp_send (slave->dev,ARPOP_REPLY , bond->master_ip, - bond->master_ip,0 ); +for (i=0 ;i<3 ;i++) +bond_arp_send (slave->dev,ARPOP_REPLY , bond->master_ip, + bond->master_ip,0 ); }list_for_each_entry (vlan, &bond->vlan_list, vlan_list) {
@@ -4331,6 +4347,7 @@ static int bond_init(struct net_device * bond->current_arp_slave =NULL ; bond->primary_slave =NULL ; bond->dev = bond_dev; + bond->send_grat_arp=0 ;INIT_LIST_HEAD (&bond->vlan_list);/* Initialize the device entry points */
drivers/net/bonding/bonding.h +1 -0
@@ -203,6 +203,7 @@ struct bonding {struct vlan_group *vlgrp;struct packet_type arp_mon_pt;s8 do_set_mac_addr; +int send_grat_arp; };/**
[ofa-general] Re: [PATCH V3 7/7] net/bonding: Delay sending of gratuitous ARP to avoid failure
Quoted 3 lines · Moni Shoua
Delay sending a
gratuitous_arpwhenLINK_STATE_LINKWATCH_PENDINGbit indev->statefield is on. This improves the chances for the arp packet to be transmitted.
Under what circumstances were you seeing problems that delaying
the gratuitous ARP until linkwatch is done improves things? Is this really an IB thing, or did you experience problems here over regular ethernet?
Quoted 13 lines
Signed-off-by Moni Shoua <[email protected]>---
drivers/net/bonding/bond_main.c| 25 +++++++++++++++++++++----drivers/net/bonding/bonding.h| 1 +2 files changed, 22 insertions(+), 4 deletions(-)Index:
net-2.6/drivers/net/bonding/bond_main.c
===================================================================drivers/net/bonding/bond_main.c +0
-0@@ -1134,8 +1134,13 @@ void bond_change_active_slave(struct bonif (new_active && !bond->do_set_mac_addr) memcpy(bond->dev->dev_addr, new_active->dev->dev_addr,
drivers/net/bonding/bond_main.c void bond_change_active_slave(struct bon · line 1143
Quoted 10 lines
drivers/net/bonding/bond_main.c +7
-2new_active->dev->addr_len); - -bond_send_gratuitous_arp (bond); +if (bond->curr_active_slave && +test_bit (__LINK_STATE_LINKWATCH_PENDING, &bond->curr_active_slave->dev->state)){ +dprintk ("delaying gratuitous arp on %s\n" ,bond->curr_active_slave->dev->name); + bond->send_grat_arp=1 ; + }else { +bond_send_gratuitous_arp (bond); + }
Style issues throughout the patch series: many lines are too
long, many things are all smashed together, e.g., "}else{" instead of "} else {", "send_grat_arp=1" instead of "send_grat_arp = 1", and so on.
Quoted 30 lines
drivers/net/bonding/bond_main.c +9
-0} }@@ -2120,6 +2125,15 @@ void bond_mii_monitor(struct net_device* program could monitor the link itself if needed. */ +if (bond->send_grat_arp) { +if (bond->curr_active_slave &&test_bit (__LINK_STATE_LINKWATCH_PENDING, &bond->curr_active_slave->dev->state)) +dprintk ("Needs to send gratuitous arp but not yet\n" ,__FUNCTION__); +else { +dprintk ("sending delayed gratuitous arp on ond->curr_active_slave->dev->name\n" ); +bond_send_gratuitous_arp (bond); + bond->send_grat_arp=0 ; + } + }read_lock (&bond->curr_slave_lock);oldcurrent = bond->curr_active_slave; read_unlock(&bond->curr_slave_lock);drivers/net/bonding/bond_main.c +1
-0@@ -2513,6 +2527,7 @@ static void bond_send_gratuitous_arp(strstruct slave *slave = bond->curr_active_slave;struct vlan_entry *vlan;struct net_device *vlan_dev; +int i;dprintk ("bond_send_grat_arp: bond %s slave %s\n" , bond->dev->name, slave ? slave->dev->name :"NULL" );
drivers/net/bonding/bond_main.c static void bond_send_gratuitous_arp(str · line 2541
Quoted 9 lines
drivers/net/bonding/bond_main.c +3
-2@@ -2520,8 +2535,9 @@ static void bond_send_gratuitous_arp(strreturn ;if (bond->master_ip) { -bond_arp_send (slave->dev,ARPOP_REPLY , bond->master_ip, - bond->master_ip,0 ); +for (i=0 ;i<3 ;i++) +bond_arp_send (slave->dev,ARPOP_REPLY , bond->master_ip, + bond->master_ip,0 ); }
If you delay the grat ARP until linkwatch is done, why is it
also necessary to shotgun several ARPs instead of one? Why are the ARPs sent for VLANs not also shotgunned in a similar fashion?
If shotgunning like this really is useful, would it not make
more sense to space them out a bit, e.g., over successive monitor passes?
Quoted 14 lines
drivers/net/bonding/bond_main.c +1
-0list_for_each_entry (vlan, &bond->vlan_list, vlan_list) {@@ -4331,6 +4347,7 @@ static int bond_init(struct net_device * bond->current_arp_slave =NULL ; bond->primary_slave =NULL ; bond->dev = bond_dev; + bond->send_grat_arp=0 ;INIT_LIST_HEAD (&bond->vlan_list);/* Initialize the device entry points */ Index:
net-2.6/drivers/net/bonding/bonding.h
===================================================================
drivers/net/bonding/bonding.h struct bonding { · line 206
Quoted 4 lines
drivers/net/bonding/bonding.h +1
-0@@ -203,6 +203,7 @@ struct bonding {struct vlan_group *vlgrp;struct packet_type arp_mon_pt;s8 do_set_mac_addr; +int send_grat_arp;
This need not be a full int, and (this applies todo_set_mac_addr, also) could probably be squeezed into gaps already existing within the struct bonding somewhere.
-J
---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]
Re: [ofa-general] Re: [PATCH V3 7/7] net/bonding: Delay sending of gratuitous ARP to avoid failure
Quoted 11 lines · Jay Vosburgh
Quoted 3 lines · Moni Shoua
Delay sending a
gratuitous_arpwhenLINK_STATE_LINKWATCH_PENDINGbit indev->statefield is on. This improves the chances for the arp packet to be transmitted.Under what circumstances were you seeing problems that delaying
the gratuitous ARP until linkwatch is done improves things? Is this really an IB thing, or did you experience problems here over regular ethernet?
I tried to figure out what is the difference in the state/flags of the device when grat. ARP send succeeds and when it fails. I found exact correlation with the LINK_STATE_LINKWATCH_PENDING bit on.
I don't think that this is an IB issue but I can't be sure. I didn't run tests for Ethernet.
Quoted 19 lines
Quoted 19 lines
Signed-off-by Moni Shoua <[email protected]>---
drivers/net/bonding/bond_main.c| 25 +++++++++++++++++++++----drivers/net/bonding/bonding.h| 1 +2 files changed, 22 insertions(+), 4 deletions(-)Index:
net-2.6/drivers/net/bonding/bond_main.c
===================================================================drivers/net/bonding/bond_main.c +3
-2@@ -1134,8 +1134,13 @@ void bond_change_active_slave(struct bonif (new_active && !bond->do_set_mac_addr)memcpy (bond->dev->dev_addr, new_active->dev->dev_addr, new_active->dev->addr_len); - -bond_send_gratuitous_arp (bond); +if (bond->curr_active_slave && +test_bit (__LINK_STATE_LINKWATCH_PENDING, &bond->curr_active_slave->dev->state)){ +dprintk ("delaying gratuitous arp on %s\n" ,bond->curr_active_slave->dev->name);
Quoted 9 lines
drivers/net/bonding/bond_main.c void bond_change_active_slave(struct bon · line 1143
Quoted 4 lines
drivers/net/bonding/bond_main.c +4
-0+ bond->send_grat_arp=1 ; + }else { +bond_send_gratuitous_arp (bond); + }Style issues throughout the patch series: many lines are too
long, many things are all smashed together, e.g., "}else{" instead of "} else {", "send_grat_arp=1" instead of "send_grat_arp= 1", and so on.
OK thanks. I'll fix and repost.
Quoted 32 lines
Quoted 32 lines
drivers/net/bonding/bond_main.c +9
-0} }@@ -2120,6 +2125,15 @@ void bond_mii_monitor(struct net_device* program could monitor the link itself if needed. */ +if (bond->send_grat_arp) { +if (bond->curr_active_slave &&test_bit (__LINK_STATE_LINKWATCH_PENDING, &bond->curr_active_slave->dev->state)) +dprintk ("Needs to send gratuitous arp but not yet\n" ,__FUNCTION__); +else { +dprintk ("sending delayed gratuitous arp on ond->curr_active_slave->dev->name\n" ); +bond_send_gratuitous_arp (bond); + bond->send_grat_arp=0 ; + } + }read_lock (&bond->curr_slave_lock);oldcurrent = bond->curr_active_slave; read_unlock(&bond->curr_slave_lock);drivers/net/bonding/bond_main.c +1
-0@@ -2513,6 +2527,7 @@ static void bond_send_gratuitous_arp(strstruct slave *slave = bond->curr_active_slave;struct vlan_entry *vlan;struct net_device *vlan_dev; +int i;dprintk ("bond_send_grat_arp: bond %s slave %s\n" , bond->dev->name, slave ? slave->dev->name :"NULL" );@@ -2520,8 +2535,9 @@ static void bond_send_gratuitous_arp(strreturn ;
Quoted 11 lines
drivers/net/bonding/bond_main.c static void bond_send_gratuitous_arp(str · line 2541
Quoted 7 lines
drivers/net/bonding/bond_main.c +3
-2if (bond->master_ip) { -bond_arp_send (slave->dev,ARPOP_REPLY , bond->master_ip, - bond->master_ip,0 ); +for (i=0 ;i<3 ;i++) +bond_arp_send (slave->dev,ARPOP_REPLY , bond->master_ip, + bond->master_ip,0 ); }If you delay the grat ARP until linkwatch is done, why is it
also necessary to shotgun several ARPs instead of one? Why are the ARPs sent for VLANs not also shotgunned in a similar fashion?
Besides the linkwatch issue I also noticed that on rare occasions, grat. ARPs that found their way to the slave's xmit function were not xmitted. The 3 times send is just an another attempt to improve chances.
I'd like to emphasize here that with IB slaves, grat. ARP is much more crucial to a successful change of slaves and that was my focus.
Quoted 4 lines
If shotgunning like this really is useful, would it not make
more sense to space them out a bit, e.g., over successive monitor passes?
I guess you are right about that.
Quoted 8 lines
Quoted 8 lines
drivers/net/bonding/bond_main.c +1
-0list_for_each_entry (vlan, &bond->vlan_list, vlan_list) {@@ -4331,6 +4347,7 @@ static int bond_init(struct net_device * bond->current_arp_slave =NULL ; bond->primary_slave =NULL ; bond->dev = bond_dev; + bond->send_grat_arp=0 ;INIT_LIST_HEAD (&bond->vlan_list);
Quoted 14 lines
drivers/net/bonding/bonding.h struct bonding { · line 206
Quoted 10 lines
drivers/net/bonding/bond_main.c +0
-0/* Initialize the device entry points */ Index:
net-2.6/drivers/net/bonding/bonding.h
===================================================================drivers/net/bonding/bonding.h +1
-0@@ -203,6 +203,7 @@ struct bonding {struct vlan_group *vlgrp;struct packet_type arp_mon_pt;s8 do_set_mac_addr; +int send_grat_arp;This need not be a full int, and (this applies to
do_set_mac_addr, also) could probably be squeezed into gaps already existing within the struct bonding somewhere.
Thanks. Will be fixed.
Quoted 12 lines
-J---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]
_______________________________________________
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/generalTo unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Quoted 4 lines
1. When bonding enslaves an IPoIB device the bonding neighbor holds a reference to a cleanup function in the IPoIB drives. This makes it unsafe to unload the IPoIB module if there are bonding neighbors in the air. So, to avoid this race one must unload bonding before unloading IPoIB.
I think we really want to resolve this somehow. Getting an oops by doing "modprobe -r ipoib" isn't that friendly.
Also, what happened to the problem of having an address handle belonging to the wrong device on bond failover? Did you figure out a way to fix that one?
- R.
Quoted 8 lines · Roland Dreier
Quoted 4 lines
1. When bonding enslaves an IPoIB device the bonding neighbor holds a reference to a cleanup function in the IPoIB drives. This makes it unsafe to unload the IPoIB module if there are bonding neighbors in the air. So, to avoid this race one must unload bonding before unloading IPoIB.
I think we really want to resolve this somehow. Getting an oops by doing "modprobe -r ipoib" isn't that friendly.
You are right and we want to resolve that.
One way is to clean the neigh destructor function from all IPoIB neighs. The other way is to prevent ipoib unload if device is a slave or is referenced from somewhere else.
I guess I would like an advice here.
Quoted 3 lines
Also, what happened to the problem of having an address handle belonging to the wrong device on bond failover? Did you figure out a way to fix that one?
This is what patch 2 handles.
Quoted 9 lines
- R.
_______________________________________________
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/generalTo unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Quoted 12 lines
Quoting Moni Shoua <[email protected]>:
Subject: Re: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for?the bonding driverQuoted 8 lines · Roland Dreier
Quoted 4 lines
1. When bonding enslaves an IPoIB device the bonding neighbor holds a reference to a cleanup function in the IPoIB drives. This makes it unsafe to unload the IPoIB module if there are bonding neighbors in the air. So, to avoid this race one must unload bonding before unloading IPoIB.
I think we really want to resolve this somehow. Getting an oops by doing "modprobe -r ipoib" isn't that friendly.
Quoted 6 lines
You are right and we want to resolve that.
One way is to clean the neigh destructor function from all IPoIB neighs. The other way is to prevent ipoib unload if device is a slave or is referenced from somewhere else.I guess I would like an advice here.
I had this idea:
Maybe we could use hard_header_cache/header_cache_update methods instead of neighbour cleanup calls.
To do this, it is possible that we'll have to switch from storing pointers inside the neighbour to keeping an index there, but I expect the performance impact to be minimal.
As a result, bonding would not have to copy pointers into ipoib module and module removal would get fixed.
Signature · 1 line
MST
Quoted 9 lines · Michael S. Tsirkin
Maybe we could use hard_header_cache/header_cache_update methods instead of neighbour cleanup calls.
To do this, it is possible that we'll have to switch from storing pointers inside the neighbour to keeping an index there, but I expect the performance impact to be minimal.
As a result, bonding would not have to copy pointers into ipoib module and module removal would get fixed.
To be precise, bonding will copy all the symbols it copies today from the slave module (ipoib), see bond_setup_by_slave() in patch 3/7, except for the neighbour cleanup callback which is copied through coping the neigh_setup function.
Or.
Quoted 11 lines
Quoting Or Gerlitz <[email protected]>:
Subject: Re: Re: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for?the bonding driverQuoted 7 lines · Michael S. Tsirkin
Maybe we could use hard_header_cache/header_cache_update methods instead of neighbour cleanup calls.
To do this, it is possible that we'll have to switch from storing pointers inside the neighbour to keeping an index there, but I expect the performance impact to be minimal.
Quoted 8 lines
Quoted 2 lines
As a result, bonding would not have to copy pointers into ipoib module and module removal would get fixed.
To be precise, bonding will copy all the symbols it copies today from the slave module (ipoib),
seebond_setup_by_slave()in patch 3/7, except
for the neighbour cleanup callback which is copied through coping theneigh_setupfunction.
Not really.
This copying of symbols is something that you added, isn't it? So with this approach, it won't be needed.
It's always wrong to copy symbols from another module without referencing it.
Signature · 1 line
MST
Quoted 10 lines · Michael S. Tsirkin
Quoted 3 lines
Quoting Or Gerlitz <[email protected]>:
To be precise, bonding will copy all the symbols it copies today from the slave module (ipoib), see
bond_setup_by_slave()in patch 3/7Not really.
This copying of symbols is something that you added, isn't it? So with this approach, it won't be needed.It's always wrong to copy symbols from another module without referencing it.
Its the --first-- time you make this comment, please suggest a different approach, the relevant code is below.
Quoted 44 lines
(quoted diff) +37
-0+static void bond_setup_by_slave(struct net_device *bond_dev, + struct net_device *slave_dev) +{ + bond_dev->hard_header = slave_dev->hard_header; + bond_dev->rebuild_header = slave_dev->rebuild_header; + bond_dev->hard_header_cache = slave_dev->hard_header_cache; + bond_dev->header_cache_update = slave_dev->header_cache_update; + bond_dev->hard_header_parse = slave_dev->hard_header_parse; + + bond_dev->neigh_setup = slave_dev->neigh_setup; + + bond_dev->type = slave_dev->type; + bond_dev->hard_header_len = slave_dev->hard_header_len; + bond_dev->addr_len = slave_dev->addr_len; + + memcpy(bond_dev->broadcast, slave_dev->broadcast, + slave_dev->addr_len); +} + /* enslave device <slave> to bond device <master> */ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) {@@ -1351,6 +1371,24 @@ int bond_enslave(struct net_device *bond goto err_undo_flags; } + /* set bonding device ether type by slave - bonding netdevices are + * created with ether_setup, so when the slave type is not ARPHRD_ETHER + * there is a need to override some of the type dependent attribs/funcs. + * + * bond ether type mutual exclusion - don't allow slaves of dissimilar + * ether type (eg ARPHRD_ETHER and ARPHRD_INFINIBAND) share the same bond + */ + if (bond->slave_cnt == 0) { + if (slave_dev->type != ARPHRD_ETHER) + bond_setup_by_slave(bond_dev, slave_dev); + } else if (bond_dev->type != slave_dev->type) { + printk(KERN_ERR DRV_NAME ": %s ether type (%d) is different from " + "other slaves (%d), can not enslave it.\n", slave_dev->name, + slave_dev->type, bond_dev->type); + res = -EINVAL; + goto err_undo_flags; + } +
Quoted 9 lines
Quoting Or Gerlitz <[email protected]>:
Subject: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for?the?bonding driverQuoted 4 lines · Michael S. Tsirkin
Quoted 4 lines
Quoting Or Gerlitz <[email protected]>:
To be precise, bonding will copy all the symbols it copies today from the slave module (ipoib), see
bond_setup_by_slave()in patch 3/7
Quoted 8 lines
Quoted 6 lines
Not really.
This copying of symbols is something that you added, isn't it? So with this approach, it won't be needed.It's always wrong to copy symbols from another module without referencing it.
Its the --first-- time you make this comment,
It's really a well known fact. That's where the crash with modprobe -r comes from, right?
Quoted 1 line
please suggest a different approach,
I don't know, really - if you want to access a module, you really must get a reference to it, or to the device.
How about adding the module pointer to struct net_device?
Quoted 11 lines
the relevant code is below.
(quoted diff) +9
-0+static void bond_setup_by_slave(struct net_device *bond_dev, + struct net_device *slave_dev) +{ + bond_dev->hard_header = slave_dev->hard_header; + bond_dev->rebuild_header = slave_dev->rebuild_header; + bond_dev->hard_header_cache = slave_dev->hard_header_cache; + bond_dev->header_cache_update = slave_dev->header_cache_update; + bond_dev->hard_header_parse = slave_dev->hard_header_parse; +
Review
Quoted 10 lines
(quoted diff) +10
-0+ bond_dev->neigh_setup = slave_dev->neigh_setup; + + bond_dev->type = slave_dev->type; + bond_dev->hard_header_len = slave_dev->hard_header_len; + bond_dev->addr_len = slave_dev->addr_len; + + memcpy(bond_dev->broadcast, slave_dev->broadcast, + slave_dev->addr_len); +} +
Hmm, it seems that switching to hard_header_cache as I suggested won't help at all. I wonder: is bonding currently broken with devices that implement hard_header_cache/header_cache_update?
Signature · 1 line
MST
Quoted 9 lines · Michael S. Tsirkin
Quoted 6 lines
Quoted 2 lines · Or Gerlitz
It's always wrong to copy symbols from another module without referencing it.
Its the --first-- time you make this comment,
It's really a well known fact. That's where the crash with modprobe -r comes from, right?
no, the crash --only-- comes from the neighbour cleanup function being called while ipoib is now probed out of the kernel. The other symbols are not problematic. I got positive feedback that this --is-- the problem in the previous posts and from Roland during my Sonoma presentation.
Quoted 5 lines
Quoted 1 line
please suggest a different approach,
I don't know, really - if you want to access a module, you really must get a reference to it, or to the device.
How about adding the module pointer to structnet_device?
I think there used to be there owner field of type struct module and it was removed... we will check that.
Quoted 15 lines
Quoted 15 lines
the relevant code is below.
(quoted diff) +13
-0+static void bond_setup_by_slave(struct net_device *bond_dev, + struct net_device *slave_dev) +{ + bond_dev->hard_header = slave_dev->hard_header; + bond_dev->rebuild_header = slave_dev->rebuild_header; + bond_dev->hard_header_cache = slave_dev->hard_header_cache; + bond_dev->header_cache_update = slave_dev->header_cache_update; + bond_dev->hard_header_parse = slave_dev->hard_header_parse; + + bond_dev->neigh_setup = slave_dev->neigh_setup; + + bond_dev->type = slave_dev->type; + bond_dev->hard_header_len = slave_dev->hard_header_len;
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ bond_dev->addr_len = slave_dev->addr_len; + + memcpy(bond_dev->broadcast, slave_dev->broadcast, + slave_dev->addr_len); +} +Hmm, it seems that switching to
hard_header_cacheas I suggested won't help at all.
why? please clarify.
Quoted 2 lines
I wonder: is bonding currently broken with devices that implement hard_header_cache/header_cache_update?
I don't think so. Note that bond_setup_by_slave is only called for slaves whose ether type is --not-- Ethernet.
Or.
Quoted 3 lines
It's always wrong to copy symbols from another module without referencing it.
Michael,
It seems like the preferred approach is to prevent ib_ipoib from being unloaded while bonding is on top it, right?
It seems like it would handle all safety issues (not just neigh cleanup).
Quoted 11 lines
Quoting Moni Shoua <[email protected]>:
Subject: Re: [ofa-general] Re: Re: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for?the bonding driverQuoted 2 lines
It's always wrong to copy symbols from another module without referencing it.
Michael,
It seems like the preferred approach is to preventib_ipoibfrom being unloaded while bonding is on top it, right?
It seems like it would handle all safety issues (not just neigh cleanup).
Donnu about preferred, but that'll work I think.
Signature · 1 line
MST