bonding: simplify and use RCU protection for 3ad xmit path

PATCH In review Ding Tianhong 2 participants 4 messages Sep 25, 2013 – Sep 26, 2013

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

Ding Tianhong

author

2013-09-25 09:53 UTC #0 lore raw

PATCH v5 1/6 bonding: simplify and use RCU protection for 3ad 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 3ad mode.

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

Signed-off-by Wang Yufen <[email protected]>

Cc Nikolay Aleksandrov <[email protected]>

Cc Veaceslav Falico <[email protected]>

---
 drivers/net/bonding/bond_3ad.c | 32 ++++++++++++++------------------
 drivers/net/bonding/bonding.h  | 30 +++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 19 deletions(-)

drivers/net/bonding/bond_3ad.c +14 ~~-18~~

index 0d8f427..13f1deb 100644
@@ -143,7 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
  */
 static inline struct port *__get_first_port(struct bonding *bond)
 {
-	struct slave *first_slave = bond_first_slave(bond);
+	struct slave *first_slave = bond_first_slave_rcu(bond);
 
 	return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
 }

@@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct port *port)
 	// If there's no bond for this port, or this is the last slave
 	if (bond == NULL)
 		return NULL;
-	slave_next = bond_next_slave(bond, slave);
+	slave_next = bond_next_slave_rcu(bond, slave);
 	if (!slave_next || bond_is_first_slave(bond, slave_next))
 		return NULL;

@@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
 
 int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 {
-	struct slave *slave, *start_at;
 	struct bonding *bond = netdev_priv(dev);
+	struct slave *slave;
 	int slave_agg_no;
 	int slaves_in_agg;
 	int agg_id;
-	int i;
 	struct ad_info ad_info;
 	int res = 1;
 
-	read_lock(&bond->lock);
 	if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
 		pr_debug("%s: Error: __bond_3ad_get_active_agg_info failed\n",
 			 dev->name);

@@ -2444,13 +2442,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 
 	slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
 
-	bond_for_each_slave(bond, slave) {
+	bond_for_each_slave_rcu(bond, slave) {
 		struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
 
 		if (agg && (agg->aggregator_identifier == agg_id)) {
-			slave_agg_no--;
-			if (slave_agg_no < 0)
-				break;
+			if (--slave_agg_no < 0) {
+				if (SLAVE_IS_OK(slave)) {
+					res = bond_dev_queue_xmit(bond,
+						skb, slave->dev);
+					goto out;
+				}
+			}
 		}
 	}

@@ -2460,23 +2462,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 		goto out;
 	}
 
-	start_at = slave;
-
-	bond_for_each_slave_from(bond, slave, i, start_at) {
-		int slave_agg_id = 0;
+	bond_for_each_slave_rcu(bond, slave) {
 		struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
 
-		if (agg)
-			slave_agg_id = agg->aggregator_identifier;
-
-		if (SLAVE_IS_OK(slave) && agg && (slave_agg_id == agg_id)) {
+		if (SLAVE_IS_OK(slave) && agg &&
+			agg->aggregator_identifier == agg_id) {
 			res = bond_dev_queue_xmit(bond, skb, slave->dev);
 			break;
 		}
 	}
 
 out:
-	read_unlock(&bond->lock);
 	if (res) {
 		/* no suitable interface, frame not sent */
 		kfree_skb(skb);

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

index 03cf3fd..eb36f57 100644
@@ -74,13 +74,31 @@
 /* slave list primitives */
 #define bond_to_slave(ptr) list_entry(ptr, struct slave, list)
 
+/* slave list primitives, Caller must hold rcu_read_lock */
+#define bond_to_slave_rcu(ptr) list_entry_rcu(ptr, struct slave, list)
+
+/* bond_is_empty return NULL if slave list is empty*/
+#define bond_is_empty(bond) \
+	(list_empty(&(bond)->slave_list))
+
+/* bond_is_empty_rcu return NULL if slave list is empty*/
+#define bond_is_empty_rcu(bond) \
+	(!list_first_or_null_rcu(&(bond)->slave_list, struct slave, list))
+
 /* IMPORTANT: bond_first/last_slave can return NULL in case of an empty list */
 #define bond_first_slave(bond) \
 	list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
 #define bond_last_slave(bond) \
-	(list_empty(&(bond)->slave_list) ? NULL : \
+	(bond_is_empty(bond) ? NULL : \
 					   bond_to_slave((bond)->slave_list.prev))
 
+/**
+ * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list
+ * Caller must hold rcu_read_lock
+ */
+#define bond_first_slave_rcu(bond) \
+	list_first_or_null_rcu(&(bond)->slave_list, struct slave, list);
+
 #define bond_is_first_slave(bond, pos) ((pos)->list.prev == &(bond)->slave_list)
 #define bond_is_last_slave(bond, pos) ((pos)->list.next == &(bond)->slave_list)

@@ -93,6 +111,16 @@
 	(bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
 					  bond_to_slave((pos)->list.prev))
 
+/* Since bond_first/last_slave_rcu can return NULL, these can return NULL too */
+#define bond_next_slave_rcu(bond, pos) \
+	({struct list_head *__slave_list = &(bond)->slave_list; \
+	 struct list_head __rcu *__next = list_next_rcu(__slave_list); \
+	 struct list_head __rcu *__pos_next = list_next_rcu(&(pos)->list); \
+	 likely(__pos_next != __slave_list) ? \
+	 container_of(__pos_next, struct slave, list) : \
+	 container_of(__next, struct slave, list); \
+	 })
+
 /**
  * bond_for_each_slave_from - iterate the slaves list from a starting point
  * @bond:	the bond holding this list.

Signature · 1 line

1.8.0

Veaceslav Falico

2013-09-25 10:51 UTC #1 lore raw

Quoted 113 lines · Ding Tianhong

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

Signed-off-by Wang Yufen <[email protected]>

Cc Nikolay Aleksandrov <[email protected]>

Cc Veaceslav Falico <[email protected]>

---
drivers/net/bonding/bond_3ad.c | 32 ++++++++++++++------------------ drivers/net/bonding/bonding.h | 30 +++++++++++++++++++++++++++++-

2 files changed, 43 insertions(+), 19 deletions(-)

drivers/net/bonding/bond_3ad.c +14 ~~-17~~

index 0d8f427..13f1deb 100644
@@ -143,7 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
 */
 static inline struct port *__get_first_port(struct bonding *bond)
 {
-	struct slave *first_slave = bond_first_slave(bond);
+	struct slave *first_slave = bond_first_slave_rcu(bond);
 
 	return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
 }

@@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct port *port)
 	// If there's no bond for this port, or this is the last slave
 	if (bond == NULL)
 		return NULL;
-	slave_next = bond_next_slave(bond, slave);
+	slave_next = bond_next_slave_rcu(bond, slave);
 	if (!slave_next || bond_is_first_slave(bond, slave_next))
 		return NULL;

@@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
 
 int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 {
-	struct slave *slave, *start_at;
 	struct bonding *bond = netdev_priv(dev);
+	struct slave *slave;
 	int slave_agg_no;
 	int slaves_in_agg;
 	int agg_id;
-	int i;
 	struct ad_info ad_info;
 	int res = 1;
 
-	read_lock(&bond->lock);
 	if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
 		pr_debug("%s: Error: __bond_3ad_get_active_agg_info failed\n",
 			 dev->name);

@@ -2444,13 +2442,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 
 	slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
 
-	bond_for_each_slave(bond, slave) {
+	bond_for_each_slave_rcu(bond, slave) {
 		struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
 
 		if (agg && (agg->aggregator_identifier == agg_id)) {
-			slave_agg_no--;
-			if (slave_agg_no < 0)
-				break;
+			if (--slave_agg_no < 0) {
+				if (SLAVE_IS_OK(slave)) {
+					res = bond_dev_queue_xmit(bond,
+						skb, slave->dev);
+					goto out;
+				}
+			}
 		}
 	}

@@ -2460,23 +2462,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 		goto out;
 	}
 
-	start_at = slave;
-
-	bond_for_each_slave_from(bond, slave, i, start_at) {
-		int slave_agg_id = 0;
+	bond_for_each_slave_rcu(bond, slave) {
 		struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
 
-		if (agg)
-			slave_agg_id = agg->aggregator_identifier;
-
-		if (SLAVE_IS_OK(slave) && agg && (slave_agg_id == agg_id)) {
+		if (SLAVE_IS_OK(slave) && agg &&
+			agg->aggregator_identifier == agg_id) {
 			res = bond_dev_queue_xmit(bond, skb, slave->dev);
 			break;
 		}
 	}

out:

drivers/net/bonding/bond_3ad.c +0 -1

-	read_unlock(&bond->lock);

	if (res) {
		/* no suitable interface, frame not sent */
		kfree_skb(skb);

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

index 03cf3fd..eb36f57 100644
@@ -74,13 +74,31 @@
 /* slave list primitives */
 #define bond_to_slave(ptr) list_entry(ptr, struct slave, list)

drivers/net/bonding/bonding.h line 86

Quoted 10 lines

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

+/* slave list primitives, Caller must hold rcu_read_lock */
+#define bond_to_slave_rcu(ptr) list_entry_rcu(ptr, struct slave, list)
+
+/* bond_is_empty return NULL if slave list is empty*/
+#define bond_is_empty(bond) \
+	(list_empty(&(bond)->slave_list))
+
+/* bond_is_empty_rcu return NULL if slave list is empty*/
+#define bond_is_empty_rcu(bond) \
+	(!list_first_or_null_rcu(&(bond)->slave_list, struct slave, list))

Useless/dangerous function. It's not used in this patch and there can be no constructs like:

1 if (!bond_is_empty_rcu(bond)) {
2 slave = bond_first_slave_rcu(bond);
3 do_something(slave);

because between 1) and 2) the slave can go away, and we'll end up with slave == NULL.

Quoted 5 lines

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

+
 /* IMPORTANT: bond_first/last_slave can return NULL in case of an empty list */
 #define bond_first_slave(bond) \
 	list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
 #define bond_last_slave(bond) \

drivers/net/bonding/bonding.h line 100

Quoted 10 lines

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

-	(list_empty(&(bond)->slave_list) ? NULL : \
+	(bond_is_empty(bond) ? NULL : \
 					   bond_to_slave((bond)->slave_list.prev))
 
+/**
+ * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list
+ * Caller must hold rcu_read_lock
+ */
+#define bond_first_slave_rcu(bond) \
+	list_first_or_null_rcu(&(bond)->slave_list, struct slave, list);

bond_first_slave_or_null_rcu() ?

Quoted 7 lines

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

+
 #define bond_is_first_slave(bond, pos) ((pos)->list.prev == &(bond)->slave_list)
 #define bond_is_last_slave(bond, pos) ((pos)->list.next == &(bond)->slave_list)

@@ -93,6 +111,16 @@
 	(bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
 					  bond_to_slave((pos)->list.prev))

drivers/net/bonding/bonding.h line 122

Quoted 10 lines

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

 
+/* Since bond_first/last_slave_rcu can return NULL, these can return NULL too */
+#define bond_next_slave_rcu(bond, pos) \
+	({struct list_head *__slave_list = &(bond)->slave_list; \
+	 struct list_head __rcu *__next = list_next_rcu(__slave_list); \
+	 struct list_head __rcu *__pos_next = list_next_rcu(&(pos)->list); \
+	 likely(__pos_next != __slave_list) ? \
+	 container_of(__pos_next, struct slave, list) : \
+	 container_of(__next, struct slave, list); \
+	 })

And if bond has no slaves, whilst pos being the slave that has just detached? Then bond->slave_list->next == pos->list->next == &bond->slave_list, and we'll return the container_of(bond->slave_list), which is garbage, but not NULL.

Quoted 13 lines

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

+
 /**
 * bond_for_each_slave_from - iterate the slaves list from a starting point
 * @bond:	the bond holding this list.

Signature · 8 lines

1.8.0



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ding Tianhong

author

2013-09-26 02:33 UTC #2 lore raw

Quoted 128 lines · Veaceslav Falico

Quoted 113 lines · Ding Tianhong

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

Signed-off-by Wang Yufen <[email protected]>

Cc Nikolay Aleksandrov <[email protected]>

Cc Veaceslav Falico <[email protected]>

---
drivers/net/bonding/bond_3ad.c | 32 ++++++++++++++------------------ drivers/net/bonding/bonding.h | 30 +++++++++++++++++++++++++++++-

2 files changed, 43 insertions(+), 19 deletions(-)

drivers/net/bonding/bond_3ad.c +14 ~~-17~~

index 0d8f427..13f1deb 100644
@@ -143,7 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
 */
 static inline struct port *__get_first_port(struct bonding *bond)
 {
-    struct slave *first_slave = bond_first_slave(bond);
+    struct slave *first_slave = bond_first_slave_rcu(bond);
 
    return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
 }

@@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct port *port)
    // If there's no bond for this port, or this is the last slave
    if (bond == NULL)
        return NULL;
-    slave_next = bond_next_slave(bond, slave);
+    slave_next = bond_next_slave_rcu(bond, slave);
    if (!slave_next || bond_is_first_slave(bond, slave_next))
        return NULL;

@@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
 
 int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 {
-    struct slave *slave, *start_at;
    struct bonding *bond = netdev_priv(dev);
+    struct slave *slave;
    int slave_agg_no;
    int slaves_in_agg;
    int agg_id;
-    int i;
    struct ad_info ad_info;
    int res = 1;
 
-    read_lock(&bond->lock);
    if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
        pr_debug("%s: Error: __bond_3ad_get_active_agg_info failed\n",
             dev->name);

@@ -2444,13 +2442,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 
    slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
 
-    bond_for_each_slave(bond, slave) {
+    bond_for_each_slave_rcu(bond, slave) {
        struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
 
        if (agg && (agg->aggregator_identifier == agg_id)) {
-            slave_agg_no--;
-            if (slave_agg_no < 0)
-                break;
+            if (--slave_agg_no < 0) {
+                if (SLAVE_IS_OK(slave)) {
+                    res = bond_dev_queue_xmit(bond,
+                        skb, slave->dev);
+                    goto out;
+                }
+            }
        }
    }

@@ -2460,23 +2462,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
        goto out;
    }
 
-    start_at = slave;
-
-    bond_for_each_slave_from(bond, slave, i, start_at) {
-        int slave_agg_id = 0;
+    bond_for_each_slave_rcu(bond, slave) {
        struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
 
-        if (agg)
-            slave_agg_id = agg->aggregator_identifier;
-
-        if (SLAVE_IS_OK(slave) && agg && (slave_agg_id == agg_id)) {
+        if (SLAVE_IS_OK(slave) && agg &&
+            agg->aggregator_identifier == agg_id) {
            res = bond_dev_queue_xmit(bond, skb, slave->dev);
            break;
        }
    }

out:

drivers/net/bonding/bond_3ad.c +0 -1

-    read_unlock(&bond->lock);
    if (res) {
        /* no suitable interface, frame not sent */
        kfree_skb(skb);

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

index 03cf3fd..eb36f57 100644
@@ -74,13 +74,31 @@
 /* slave list primitives */
 #define bond_to_slave(ptr) list_entry(ptr, struct slave, list)

drivers/net/bonding/bonding.h line 86

Quoted 10 lines

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

+/* slave list primitives, Caller must hold rcu_read_lock */
+#define bond_to_slave_rcu(ptr) list_entry_rcu(ptr, struct slave, list)
+
+/* bond_is_empty return NULL if slave list is empty*/
+#define bond_is_empty(bond) \
+    (list_empty(&(bond)->slave_list))
+
+/* bond_is_empty_rcu return NULL if slave list is empty*/
+#define bond_is_empty_rcu(bond) \
+    (!list_first_or_null_rcu(&(bond)->slave_list, struct slave, list))

Useless/dangerous function. It's not used in this patch and there can be no constructs like:

Quoted 4 lines

1 if (!bond_is_empty_rcu(bond)) {
2 slave = bond_first_slave_rcu(bond);
3 do_something(slave);

agree, forgot to remove it.

Quoted 12 lines

because between 1) and 2) the slave can go away, and we'll end up with slave == NULL.

Quoted 9 lines

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

+
 /* IMPORTANT: bond_first/last_slave can return NULL in case of an empty list */
 #define bond_first_slave(bond) \
    list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
 #define bond_last_slave(bond) \
-    (list_empty(&(bond)->slave_list) ? NULL : \
+    (bond_is_empty(bond) ? NULL : \
                       bond_to_slave((bond)->slave_list.prev))

Quoted 9 lines

drivers/net/bonding/bonding.h line 100
Quoted 6 lines
drivers/net/bonding/bonding.h +6 -0
+/**
+ * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list
+ * Caller must hold rcu_read_lock
+ */
+#define bond_first_slave_rcu(bond) \
+    list_first_or_null_rcu(&(bond)->slave_list, struct slave, list);
bond_first_slave_or_null_rcu() ?

both is suitable.

Quoted 39 lines

Quoted 7 lines

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

+
 #define bond_is_first_slave(bond, pos) ((pos)->list.prev == &(bond)->slave_list)
 #define bond_is_last_slave(bond, pos) ((pos)->list.next == &(bond)->slave_list)

@@ -93,6 +111,16 @@
    (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
                      bond_to_slave((pos)->list.prev))

drivers/net/bonding/bonding.h line 122

Quoted 10 lines

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

 
+/* Since bond_first/last_slave_rcu can return NULL, these can return NULL too */
+#define bond_next_slave_rcu(bond, pos) \
+    ({struct list_head *__slave_list = &(bond)->slave_list; \
+     struct list_head __rcu *__next = list_next_rcu(__slave_list); \
+     struct list_head __rcu *__pos_next = list_next_rcu(&(pos)->list); \
+     likely(__pos_next != __slave_list) ? \
+     container_of(__pos_next, struct slave, list) : \
+     container_of(__next, struct slave, list); \
+     })

Quoted 13 lines

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

+
 /**
 * bond_for_each_slave_from - iterate the slaves list from a starting point
 * @bond:    the bond holding this list.

Signature · 8 lines

1.8.0



-- 
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Veaceslav Falico

2013-09-26 06:09 UTC #3 lore raw

In reply to Ding Tianhong

Quoted 67 lines · Ding Tianhong

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

Signed-off-by Wang Yufen <[email protected]>

Cc Nikolay Aleksandrov <[email protected]>

Cc Veaceslav Falico <[email protected]>

---
drivers/net/bonding/bond_3ad.c | 32 ++++++++++++++------------------ drivers/net/bonding/bonding.h | 30 +++++++++++++++++++++++++++++-

2 files changed, 43 insertions(+), 19 deletions(-)

drivers/net/bonding/bond_3ad.c +4 -8

index 0d8f427..13f1deb 100644
@@ -143,7 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
 */
 static inline struct port *__get_first_port(struct bonding *bond)
 {
-	struct slave *first_slave = bond_first_slave(bond);
+	struct slave *first_slave = bond_first_slave_rcu(bond);
 
 	return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
 }

@@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct port *port)
 	// If there's no bond for this port, or this is the last slave
 	if (bond == NULL)
 		return NULL;
-	slave_next = bond_next_slave(bond, slave);
+	slave_next = bond_next_slave_rcu(bond, slave);
 	if (!slave_next || bond_is_first_slave(bond, slave_next))
 		return NULL;

@@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
 
 int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 {
-	struct slave *slave, *start_at;
 	struct bonding *bond = netdev_priv(dev);
+	struct slave *slave;
 	int slave_agg_no;
 	int slaves_in_agg;
 	int agg_id;
-	int i;
 	struct ad_info ad_info;
 	int res = 1;
 
-	read_lock(&bond->lock);
 	if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
 		pr_debug("%s: Error: __bond_3ad_get_active_agg_info failed\n",
 			 dev->name);

@@ -2444,13 +2442,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 
 	slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
 
-	bond_for_each_slave(bond, slave) {
+	bond_for_each_slave_rcu(bond, slave) {
 		struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
 
 		if (agg && (agg->aggregator_identifier == agg_id)) {
-			slave_agg_no--;
-			if (slave_agg_no < 0)

drivers/net/bonding/bond_3ad.c int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev) · line 2457

Quoted 10 lines

drivers/net/bonding/bond_3ad.c +7 -1

-				break;
+			if (--slave_agg_no < 0) {
+				if (SLAVE_IS_OK(slave)) {
+					res = bond_dev_queue_xmit(bond,
+						skb, slave->dev);
+					goto out;
+				}
+			}
 		}
 	}

So here you are checking for any suitable slave from slave number slave_agg_no+1 and till the last slave. Ok.

Some nitpicks, though not critical - slave_agg_no will always get decremented, even if it's negative. It's ok though harder to read/understand. The triple cascaded ifs can be omited, from (used the same code):

if (agg && (agg->aggregator_identifier == agg_id)) {
	if (--slave_agg_no < 0) {
		if (SLAVE_IS_OK(slave)) {
			do_something();
			goto out;
		}
	}
}

to (again, used the same code):

if (!agg || agg->aggregator_identifier != agg_id)

	continue;

if (--slave_agg_no >= 0)

	continue;

if (SLAVE_IS_OK(slave)) {
	do_something();
	goto out;
}

Which is a lot easier to read/understand. Though, again, there are small nitpicks and I'm ok with your approach.

Quoted 12 lines

drivers/net/bonding/bond_3ad.c +1 -4

@@ -2460,23 +2462,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 		goto out;
 	}
 
-	start_at = slave;
-
-	bond_for_each_slave_from(bond, slave, i, start_at) {
-		int slave_agg_id = 0;
+	bond_for_each_slave_rcu(bond, slave) {
 		struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;

drivers/net/bonding/bond_3ad.c int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev) · line 2473

Quoted 10 lines

drivers/net/bonding/bond_3ad.c +2 -4

-		if (agg)
-			slave_agg_id = agg->aggregator_identifier;
-
-		if (SLAVE_IS_OK(slave) && agg && (slave_agg_id == agg_id)) {
+		if (SLAVE_IS_OK(slave) && agg &&
+			agg->aggregator_identifier == agg_id) {
 			res = bond_dev_queue_xmit(bond, skb, slave->dev);
 			break;
 		}
 	}

Ok, so if the first approach fails - you loop through *all* the slaves again, checking the slaves already checked by the first loop - i.e. the slaves from slave number slave_agg_no and till the last slave.

It's suboptimal, but still should work. You can optimize this by storing a 'first ok slave' in the first loop, and if the first loop fails to find any slave *after* slave_agg_no, you can use that first_ok_slave (if found) to send. This way you'll drop the second loop entirely. It's the way that I've done in my patch - pls consult my function :) :

http://patchwork.ozlabs.org/patch/277701/

bond_for_each_slave(bond, slave, iter) {
        agg = SLAVE_AD_INFO(slave).port.aggregator;
        if (!agg || agg->aggregator_identifier != agg_id)
                continue;

        if (slave_agg_no >= 0) {
                if (!first_ok_slave && SLAVE_IS_OK(slave))
                        first_ok_slave = slave;
                slave_agg_no--;
                continue;
        }

        if (SLAVE_IS_OK(slave)) {
                res = bond_dev_queue_xmit(bond, skb, slave->dev);
                goto out;
        }
}

...

if (first_ok_slave)

	res = bond_dev_queue_xmit(bond, skb, first_ok_slave->dev);

Quoted 65 lines

out:

drivers/net/bonding/bond_3ad.c +0 -1

-	read_unlock(&bond->lock);

	if (res) {
		/* no suitable interface, frame not sent */
		kfree_skb(skb);

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

index 03cf3fd..eb36f57 100644
@@ -74,13 +74,31 @@
 /* slave list primitives */
 #define bond_to_slave(ptr) list_entry(ptr, struct slave, list)
 
+/* slave list primitives, Caller must hold rcu_read_lock */
+#define bond_to_slave_rcu(ptr) list_entry_rcu(ptr, struct slave, list)
+
+/* bond_is_empty return NULL if slave list is empty*/
+#define bond_is_empty(bond) \
+	(list_empty(&(bond)->slave_list))
+
+/* bond_is_empty_rcu return NULL if slave list is empty*/
+#define bond_is_empty_rcu(bond) \
+	(!list_first_or_null_rcu(&(bond)->slave_list, struct slave, list))
+
 /* IMPORTANT: bond_first/last_slave can return NULL in case of an empty list */
 #define bond_first_slave(bond) \
 	list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
 #define bond_last_slave(bond) \
-	(list_empty(&(bond)->slave_list) ? NULL : \
+	(bond_is_empty(bond) ? NULL : \
 					   bond_to_slave((bond)->slave_list.prev))
 
+/**
+ * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list
+ * Caller must hold rcu_read_lock
+ */
+#define bond_first_slave_rcu(bond) \
+	list_first_or_null_rcu(&(bond)->slave_list, struct slave, list);
+
 #define bond_is_first_slave(bond, pos) ((pos)->list.prev == &(bond)->slave_list)
 #define bond_is_last_slave(bond, pos) ((pos)->list.next == &(bond)->slave_list)

@@ -93,6 +111,16 @@
 	(bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
 					  bond_to_slave((pos)->list.prev))
 
+/* Since bond_first/last_slave_rcu can return NULL, these can return NULL too */
+#define bond_next_slave_rcu(bond, pos) \
+	({struct list_head *__slave_list = &(bond)->slave_list; \
+	 struct list_head __rcu *__next = list_next_rcu(__slave_list); \
+	 struct list_head __rcu *__pos_next = list_next_rcu(&(pos)->list); \
+	 likely(__pos_next != __slave_list) ? \
+	 container_of(__pos_next, struct slave, list) : \
+	 container_of(__next, struct slave, list); \
+	 })
+
 /**
 * bond_for_each_slave_from - iterate the slaves list from a starting point
 * @bond:	the bond holding this list.

Signature · 1 line

1.8.0