bonding: use RCU protection for alb xmit path

The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb (bonding: initial RCU conversion) has convert the roundrobin, active-backup, broadcast and xor xmit path to rcu protection, the performance will be better for these mode, so this time, convert xmit path for alb mode.

Signed-off-by Ding Tianhong <[email protected]>

Signed-off-by Yang Yingliang <[email protected]>

Cc Nikolay Aleksandrov <[email protected]>

Cc Veaceslav Falico <[email protected]>

---
drivers/net/bonding/bond_alb.c | 23 +++++++++-------------- drivers/net/bonding/bonding.h | 2 +-

2 files changed, 10 insertions(+), 15 deletions(-)

drivers/net/bonding/bond_alb.c +9 -14

index e960418..a1444d5 100644
@@ -230,7 +230,7 @@ static struct slave *tlb_get_least_loaded_slave(struct bonding *bond)
 	max_gap = LLONG_MIN;
 
 	/* Find the slave with the largest gap */
-	bond_for_each_slave(bond, slave, iter) {
+	bond_for_each_slave_rcu(bond, slave, iter) {
 		if (SLAVE_IS_OK(slave)) {
 			long long gap = compute_gap(slave);
 

@@ -387,7 +387,7 @@ static struct slave *rlb_next_rx_slave(struct bonding *bond)
 	struct list_head *iter;
 	bool found = false;
 
-	bond_for_each_slave(bond, slave, iter) {
+	bond_for_each_slave_rcu(bond, slave, iter) {
 		if (!SLAVE_IS_OK(slave))
 			continue;
 		if (!found) {

@@ -628,12 +628,14 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
 {
 	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
 	struct arp_pkt *arp = arp_pkt(skb);
-	struct slave *assigned_slave;
+	struct slave *assigned_slave, *curr_active_slave;
 	struct rlb_client_info *client_info;
 	u32 hash_index = 0;
 
 	_lock_rx_hashtbl(bond);
 
+	curr_active_slave = rcu_dereference(bond->curr_active_slave);
+
 	hash_index = _simple_hash((u8 *)&arp->ip_dst, sizeof(arp->ip_dst));
 	client_info = &(bond_info->rx_hashtbl[hash_index]);
 

@@ -658,8 +660,8 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
 			 * that the new client can be assigned to this entry.
 			 */
 			if (bond->curr_active_slave &&
-			    client_info->slave != bond->curr_active_slave) {
-				client_info->slave = bond->curr_active_slave;
+			    client_info->slave != curr_active_slave) {
+				client_info->slave = curr_active_slave;
 				rlb_update_client(client_info);
 			}
 		}

@@ -1343,11 +1345,6 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
 	skb_reset_mac_header(skb);
 	eth_data = eth_hdr(skb);
 
-	/* make sure that the curr_active_slave do not change during tx
-	 */
-	read_lock(&bond->lock);
-	read_lock(&bond->curr_slave_lock);
-
 	switch (ntohs(skb->protocol)) {
 	case ETH_P_IP: {
 		const struct iphdr *iph = ip_hdr(skb);

@@ -1429,12 +1426,12 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
 
 	if (!tx_slave) {
 		/* unbalanced or unassigned, send through primary */
-		tx_slave = bond->curr_active_slave;
+		tx_slave = rcu_dereference(bond->curr_active_slave);
 		bond_info->unbalanced_load += skb->len;
 	}
 
 	if (tx_slave && SLAVE_IS_OK(tx_slave)) {
-		if (tx_slave != bond->curr_active_slave) {
+		if (tx_slave != rcu_dereference(bond->curr_active_slave)) {
 			memcpy(eth_data->h_source,
 			       tx_slave->dev->dev_addr,
 			       ETH_ALEN);

@@ -1449,8 +1446,6 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
 		}
 	}
 
-	read_unlock(&bond->curr_slave_lock);
-	read_unlock(&bond->lock);
 	if (res) {
 		/* no suitable interface, frame not sent */
 		kfree_skb(skb);

drivers/net/bonding/bonding.h +0 -0

index 713b6af..1c7d559 100644

drivers/net/bonding/bonding.h +0 -0

 		if (ether_addr_equal_64bits(mac, tmp->dev->dev_addr))
 			return tmp;
 

Signature · 1 line

1.8.2.1

Quoted 105 lines · Ding Tianhong

The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb (bonding: initial RCU conversion) has convert the roundrobin, active-backup, broadcast and xor xmit path to rcu protection, the performance will be better for these mode, so this time, convert xmit path for alb mode.

Signed-off-by Ding Tianhong <[email protected]>

Signed-off-by Yang Yingliang <[email protected]>

Cc Nikolay Aleksandrov <[email protected]>

Cc Veaceslav Falico <[email protected]>

---
drivers/net/bonding/bond_alb.c | 23 +++++++++-------------- drivers/net/bonding/bonding.h | 2 +-

2 files changed, 10 insertions(+), 15 deletions(-)

drivers/net/bonding/bond_alb.c +9 -14

index e960418..a1444d5 100644
@@ -230,7 +230,7 @@ static struct slave *tlb_get_least_loaded_slave(struct bonding *bond)
    max_gap = LLONG_MIN;
 
    /* Find the slave with the largest gap */
-    bond_for_each_slave(bond, slave, iter) {
+    bond_for_each_slave_rcu(bond, slave, iter) {
        if (SLAVE_IS_OK(slave)) {
            long long gap = compute_gap(slave);
 

@@ -387,7 +387,7 @@ static struct slave *rlb_next_rx_slave(struct bonding *bond)
    struct list_head *iter;
    bool found = false;
 
-    bond_for_each_slave(bond, slave, iter) {
+    bond_for_each_slave_rcu(bond, slave, iter) {
        if (!SLAVE_IS_OK(slave))
            continue;
        if (!found) {

@@ -628,12 +628,14 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
 {
    struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
    struct arp_pkt *arp = arp_pkt(skb);
-    struct slave *assigned_slave;
+    struct slave *assigned_slave, *curr_active_slave;
    struct rlb_client_info *client_info;
    u32 hash_index = 0;
 
    _lock_rx_hashtbl(bond);
 
+    curr_active_slave = rcu_dereference(bond->curr_active_slave);
+
    hash_index = _simple_hash((u8 *)&arp->ip_dst, sizeof(arp->ip_dst));
    client_info = &(bond_info->rx_hashtbl[hash_index]);
 

@@ -658,8 +660,8 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
             * that the new client can be assigned to this entry.
             */
            if (bond->curr_active_slave &&
-                client_info->slave != bond->curr_active_slave) {
-                client_info->slave = bond->curr_active_slave;
+                client_info->slave != curr_active_slave) {
+                client_info->slave = curr_active_slave;
                rlb_update_client(client_info);
            }
        }

@@ -1343,11 +1345,6 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
    skb_reset_mac_header(skb);
    eth_data = eth_hdr(skb);
 
-    /* make sure that the curr_active_slave do not change during tx
-     */
-    read_lock(&bond->lock);
-    read_lock(&bond->curr_slave_lock);
-
    switch (ntohs(skb->protocol)) {
    case ETH_P_IP: {
        const struct iphdr *iph = ip_hdr(skb);

@@ -1429,12 +1426,12 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
 
    if (!tx_slave) {
        /* unbalanced or unassigned, send through primary */
-        tx_slave = bond->curr_active_slave;
+        tx_slave = rcu_dereference(bond->curr_active_slave);
        bond_info->unbalanced_load += skb->len;
    }
 
    if (tx_slave && SLAVE_IS_OK(tx_slave)) {
-        if (tx_slave != bond->curr_active_slave) {
+        if (tx_slave != rcu_dereference(bond->curr_active_slave)) {
            memcpy(eth_data->h_source,
                   tx_slave->dev->dev_addr,
                   ETH_ALEN);

@@ -1449,8 +1446,6 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
        }
    }
 
-    read_unlock(&bond->curr_slave_lock);
-    read_unlock(&bond->lock);
    if (res) {
        /* no suitable interface, frame not sent */
        kfree_skb(skb);

drivers/net/bonding/bonding.h +0 -0

index 713b6af..1c7d559 100644

drivers/net/bonding/bonding.h static inline struct slave *bond_slave_has_mac(struct bonding *bond, · line 462

Quoted 5 lines

drivers/net/bonding/bonding.h +1 -1

@@ -459,7 +459,7 @@ static inline struct slave *bond_slave_has_mac(struct bonding *bond,
    struct list_head *iter;
    struct slave *tmp;
 
-    bond_for_each_slave(bond, tmp, iter)
+    bond_for_each_slave_rcu(bond, tmp, iter)

This suggests that rcu_read_lock() is always held here, however it's not the case, as far as I can tell:

bond_release()/bond_uninit() ->
__bond_release_one() ->
bond_alb_deinit_slave() ->
alb_change_hw_addr_on_detach() ->
bond_slave_has_mac()

There is nobody that takes rcu_read_lock() there. And, for example, when doing:

echo -$slave > /sys/class/net/$bond_int/bonding/slaves

nobody is holding it. And, btw, in bond_for_each_slave_rcu(), which is actually a wrapper for netdev_for_each_lower_private_rcu(), which calls netdev_lower_get_next_private_rcu(), which has:

4543 void *netdev_lower_get_next_private_rcu(struct net_device *dev,
4544 struct list_head **iter)
4545 {
4546 struct netdev_adjacent *lower;
4547 4548 WARN_ON_ONCE(!rcu_read_lock_held());

so you should have seen that warning when trying to remove a slave in alb mode.

Quoted 8 lines

drivers/net/bonding/bonding.h +0 -0

        if (ether_addr_equal_64bits(mac, tmp->dev->dev_addr))
            return tmp;
 

Signature · 1 line

1.8.2.1

.