Skip to contentSkip to conversation

bonding: restructure and add rcu for bond_for_each_slave_next()

PATCH Superseded Ding Tianhong 3 participants 4 messages Sep 4, 2013

Subsystems (3)
Networking Network drivers Bonding driver

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Ding Tianhong

author
#0 lore raw

PATCH v2 5/6 bonding: restructure and add rcu for bond_for_each_slave_next()

Remove the wordy int and add bond_for_each_slave_next_rcu() for future use.

Suggested-by Nikolay Aleksandrov <[email protected]>
Signed-off-by Ding Tianhong <[email protected]>
Cc Nikolay Aleksandrov <[email protected]>
---
 drivers/net/bonding/bond_alb.c  |  3 +--
 drivers/net/bonding/bond_main.c |  6 ++----
 drivers/net/bonding/bonding.h   | 30 ++++++++++++++++++++++++++----
 3 files changed, 29 insertions(+), 10 deletions(-)
drivers/net/bonding/bond_alb.c +1 -2
index 91f179d..c75d383 100644
@@ -383,7 +383,6 @@ static struct slave *rlb_next_rx_slave(struct bonding *bond)
 {
 	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
 	struct slave *rx_slave, *slave, *start_at;
-	int i = 0;
 
 	if (bond_info->next_rx_slave)
 		start_at = bond_info->next_rx_slave;

@@ -392,7 +391,7 @@ static struct slave *rlb_next_rx_slave(struct bonding *bond)
 
 	rx_slave = NULL;
 
-	bond_for_each_slave_from(bond, slave, i, start_at) {
+	bond_for_each_slave_from(bond, slave, start_at) {
 		if (SLAVE_IS_OK(slave)) {
 			if (!rx_slave) {
 				rx_slave = slave;
drivers/net/bonding/bond_main.c +2 -4
index 39e5b1c..4190389 100644
@@ -782,7 +782,6 @@ static struct slave *bond_find_best_slave(struct bonding *bond)
 	struct slave *new_active, *old_active;
 	struct slave *bestslave = NULL;
 	int mintime = bond->params.updelay;
-	int i;
 
 	new_active = bond->curr_active_slave;
 

@@ -801,7 +800,7 @@ static struct slave *bond_find_best_slave(struct bonding *bond)
 	/* remember where to stop iterating over the slaves */
 	old_active = new_active;
 
-	bond_for_each_slave_from(bond, new_active, i, old_active) {
+	bond_for_each_slave_from(bond, new_active, old_active) {
 		if (new_active->link == BOND_LINK_UP) {
 			return new_active;
 		} else if (new_active->link == BOND_LINK_BACK &&

@@ -2756,7 +2755,6 @@ do_failover:
 static void bond_ab_arp_probe(struct bonding *bond)
 {
 	struct slave *slave, *next_slave;
-	int i;
 
 	read_lock(&bond->curr_slave_lock);
 

@@ -2788,7 +2786,7 @@ static void bond_ab_arp_probe(struct bonding *bond)
 
 	/* search for next candidate */
 	next_slave = bond_next_slave(bond, bond->current_arp_slave);
-	bond_for_each_slave_from(bond, slave, i, next_slave) {
+	bond_for_each_slave_from(bond, slave, next_slave) {
 		if (IS_UP(slave->dev)) {
 			slave->link = BOND_LINK_BACK;
 			bond_set_slave_active_flags(slave);
drivers/net/bonding/bonding.h +26 -4
index e97eee9..80b170a 100644
@@ -115,18 +115,40 @@
 	(bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \
 					  bond_to_slave_rcu((pos)->list.prev))
 
+/* Check whether the slave is the only one in bond */
+#define bond_is_only_slave(bond, pos) \
+	(((pos)->list.prev == &(bond)->slave_list) && \
+				    ((pos)->list.next == &(bond)->slave_list))
+
 /**
  * bond_for_each_slave_from - iterate the slaves list from a starting point
  * @bond:	the bond holding this list.
  * @pos:	current slave.
- * @cnt:	counter for max number of moves
  * @start:	starting point.
  *
  * Caller must hold bond->lock
  */
-#define bond_for_each_slave_from(bond, pos, cnt, start) \
-	for (cnt = 0, pos = start; pos && cnt < (bond)->slave_cnt; \
-	     cnt++, pos = bond_next_slave(bond, pos))
+#define bond_for_each_slave_from(bond, pos, start) \
+	for (pos = start; pos && (bond_is_only_slave(bond, start) ? \
+	    &pos->list != &bond->slave_list : \
+	    &pos->list != &start->list); bond_is_only_slave(bond, start) ? \
+	    (pos = list_entry(pos->list.next, typeof(*pos), list)) : \
+	    (pos = bond_next_slave(bond, pos)))
+
+/**
+ * bond_for_each_slave_from_rcu - iterate the slaves list from a starting point
+ * @bond:	the bond holding this list.
+ * @pos:	current slave.
+ * @start:	starting point.
+ *
+ * Caller must hold rcu_read_lock
+ */
+#define bond_for_each_slave_from_rcu(bond, pos, start) \
+	for (pos = start; pos && (bond_is_only_slave(bond, start) ? \
+	    &pos->list != &bond->slave_list : \
+	    &pos->list != &start->list); bond_is_only_slave(bond, start) ? \
+	    (pos = list_entry_rcu(pos->list.next, typeof(*pos), list)) : \
+	    (pos = bond_next_slave_rcu(bond, pos)))
 
 /**
  * bond_for_each_slave - iterate over all slaves
Signature · 1 line
1.8.2.1
#1 lore raw

On Wed, Sep 04, 2013 at 05:44:15PM +0800, Ding Tianhong wrote:
...snip...

drivers/net/bonding/bonding.h line 121

Quoted 4 lines
drivers/net/bonding/bonding.h +4 -0
+/* Check whether the slave is the only one in bond */
+#define bond_is_only_slave(bond, pos) \
+	(((pos)->list.prev == &(bond)->slave_list) && \
+				    ((pos)->list.next == &(bond)->slave_list))

Could be done without pos at all -

!list_empty(&(bond)->slave_list) && \
&(bond)->slave_list.next == &(bond)->slave_list.prev

If we have only one slave and pos is NOT our slave then... well.. we have big troubles.

Quoted 10 lines

+
/**
* bond_for_each_slave_from - iterate the slaves list from a starting point
* @bond: the bond holding this list.
* @pos: current slave.
- * @cnt: counter for max number of moves
* @start: starting point.
*
* Caller must hold bond->lock
*/

drivers/net/bonding/bonding.h line 136

Quoted 9 lines
drivers/net/bonding/bonding.h +6 -3
-#define bond_for_each_slave_from(bond, pos, cnt, start) \
-	for (cnt = 0, pos = start; pos && cnt < (bond)->slave_cnt; \
-	     cnt++, pos = bond_next_slave(bond, pos))
+#define bond_for_each_slave_from(bond, pos, start) \
+	for (pos = start; pos && (bond_is_only_slave(bond, start) ? \
+	    &pos->list != &bond->slave_list : \
+	    &pos->list != &start->list); bond_is_only_slave(bond, start) ? \
+	    (pos = list_entry(pos->list.next, typeof(*pos), list)) : \
+	    (pos = bond_next_slave(bond, pos)))

Did you check that?

pos = slave1 (bond has more than one slave);
pos && &pos->list != &slave1->list - false.

We won't ever enter this loop if we have >1 slaves.

I don't understand this at all.

Quoted 5 lines
drivers/net/bonding/bonding.h +5 -0
+
+/**
+ * bond_for_each_slave_from_rcu - iterate the slaves list from a starting point
+ * @bond:	the bond holding this list.
+ * @pos:	current slave.

drivers/net/bonding/bonding.h line 151

Quoted 10 lines
drivers/net/bonding/bonding.h +10 -0
+ * @start:	starting point.
+ *
+ * Caller must hold rcu_read_lock
+ */
+#define bond_for_each_slave_from_rcu(bond, pos, start) \
+	for (pos = start; pos && (bond_is_only_slave(bond, start) ? \
+	    &pos->list != &bond->slave_list : \
+	    &pos->list != &start->list); bond_is_only_slave(bond, start) ? \
+	    (pos = list_entry_rcu(pos->list.next, typeof(*pos), list)) : \
+	    (pos = bond_next_slave_rcu(bond, pos)))

Ditto as bond_for_each_slave_from() and, also, see my comment about RCU from patch 1.

Quoted 8 lines

/**
* bond_for_each_slave - iterate over all slaves

Signature · 1 line
1.8.2.1
#2 lore raw
Quoted 9 lines · Veaceslav Falico

On Wed, Sep 04, 2013 at 05:44:15PM +0800, Ding Tianhong wrote:
...snip...

drivers/net/bonding/bonding.h line 121

Quoted 4 lines
drivers/net/bonding/bonding.h +4 -0
+/* Check whether the slave is the only one in bond */
+#define bond_is_only_slave(bond, pos) \
+ (((pos)->list.prev == &(bond)->slave_list) && \
+ ((pos)->list.next == &(bond)->slave_list))

Could be done without pos at all -

Quoted 6 lines

!list_empty(&(bond)->slave_list) && \
&(bond)->slave_list.next == &(bond)->slave_list.prev

If we have only one slave and pos is NOT our slave then... well.. we have big troubles.

yes, more simple more beautiful, thanks.

but if the pos is not our slave, it is the mistake, not bug. :)

Quoted 22 lines
Quoted 10 lines

+
/**
* bond_for_each_slave_from - iterate the slaves list from a starting point
* @bond: the bond holding this list.
* @pos: current slave.
- * @cnt: counter for max number of moves
* @start: starting point.
*
* Caller must hold bond->lock
*/

drivers/net/bonding/bonding.h line 136

Quoted 9 lines
drivers/net/bonding/bonding.h +6 -3
-#define bond_for_each_slave_from(bond, pos, cnt, start) \
- for (cnt = 0, pos = start; pos && cnt < (bond)->slave_cnt; \
- cnt++, pos = bond_next_slave(bond, pos))
+#define bond_for_each_slave_from(bond, pos, start) \
+ for (pos = start; pos && (bond_is_only_slave(bond, start) ? \
+ &pos->list != &bond->slave_list : \
+ &pos->list != &start->list); bond_is_only_slave(bond, start) ? \
+ (pos = list_entry(pos->list.next, typeof(*pos), list)) : \
+ (pos = bond_next_slave(bond, pos)))

Did you check that?

Quoted 7 lines

pos = slave1 (bond has more than one slave);
pos && &pos->list != &slave1->list - false.

We won't ever enter this loop if we have >1 slaves.

I don't understand this at all.

ok, the logic is : if slaves == 1, run once for the slave.
if slaves > 1, run loops until reach the end list.
I could not get a better way to simplify the function, I think it is a suitable scheme.

by the way, I test the function and works well. :)

Quoted 33 lines
Quoted 8 lines
drivers/net/bonding/bonding.h +3 -0
+
+/**
+ * bond_for_each_slave_from_rcu - iterate the slaves list from a

starting point
+ * @bond: the bond holding this list.
+ * @pos: current slave.
+ * @start: starting point.
+ *

Quoted 8 lines

+ * Caller must hold rcu_read_lock
+ */
+#define bond_for_each_slave_from_rcu(bond, pos, start) \
+ for (pos = start; pos && (bond_is_only_slave(bond, start) ? \
+ &pos->list != &bond->slave_list : \
+ &pos->list != &start->list); bond_is_only_slave(bond, start) ? \
+ (pos = list_entry_rcu(pos->list.next, typeof(*pos), list)) : \
+ (pos = bond_next_slave_rcu(bond, pos)))

Ditto as bond_for_each_slave_from() and, also, see my comment about RCU from patch 1.

Quoted 8 lines

/**
* bond_for_each_slave - iterate over all slaves

Signature · 1 line
1.8.2.1
Signature · 3 lines
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
#3 lore raw
Quoted 45 lines · Ding Tianhong
Quoted 9 lines · Veaceslav Falico

On Wed, Sep 04, 2013 at 05:44:15PM +0800, Ding Tianhong wrote:
...snip...

drivers/net/bonding/bonding.h line 121

Quoted 4 lines
drivers/net/bonding/bonding.h +4 -0
+/* Check whether the slave is the only one in bond */
+#define bond_is_only_slave(bond, pos) \
+ (((pos)->list.prev == &(bond)->slave_list) && \
+ ((pos)->list.next == &(bond)->slave_list))

Could be done without pos at all -

Quoted 6 lines

!list_empty(&(bond)->slave_list) && \
&(bond)->slave_list.next == &(bond)->slave_list.prev

If we have only one slave and pos is NOT our slave then... well.. we have big troubles.

yes, more simple more beautiful, thanks.

but if the pos is not our slave, it is the mistake, not bug. :)

Quoted 25 lines
Quoted 10 lines

+
/**
* bond_for_each_slave_from - iterate the slaves list from a starting point
* @bond: the bond holding this list.
* @pos: current slave.
- * @cnt: counter for max number of moves
* @start: starting point.
*
* Caller must hold bond->lock
*/

drivers/net/bonding/bonding.h line 136

Quoted 9 lines
drivers/net/bonding/bonding.h +6 -3
-#define bond_for_each_slave_from(bond, pos, cnt, start) \
- for (cnt = 0, pos = start; pos && cnt < (bond)->slave_cnt; \
- cnt++, pos = bond_next_slave(bond, pos))
+#define bond_for_each_slave_from(bond, pos, start) \
+ for (pos = start; pos && (bond_is_only_slave(bond, start) ? \
+ &pos->list != &bond->slave_list : \
+ &pos->list != &start->list); bond_is_only_slave(bond, start) ? \
+ (pos = list_entry(pos->list.next, typeof(*pos), list)) : \
+ (pos = bond_next_slave(bond, pos)))

Did you check that?

pos = slave1 (bond has more than one slave);
pos && &pos->list != &slave1->list - false.

Quoted 6 lines
Quoted 4 lines

We won't ever enter this loop if we have >1 slaves.

I don't understand this at all.

ok, the logic is : if slaves == 1, run once for the slave.

your code, actually, says differently:

(bond_is_only_slave(bond, start) ? &pos->list != &bond->slave_list :

which means that if bond_is_only_slave(bond, start) == true then we check &pos->list != &bond->slave_list, so we run till the end of the list.

Ternary operator works like that

expression ? what_to_do_if_true : what_to_do_if_false

http://en.wikipedia.org/wiki/?:#C

Quoted 1 line

if slaves > 1, run loops until reach the end list.

Here we test for &pos->list != &start->list, which is false cause pos == start.

Quoted 1 line

I could not get a better way to simplify the function, I think it is a suitable scheme.

Nope. bond_for_each_slave_from() is supposed to loop through slaves from a starting slave and till that starting slave, not included.

Your loop... I don't understand what it does. But clearly not what it was doing (and supposed to do).

Quoted 37 lines

by the way, I test the function and works well. :)

Quoted 33 lines
Quoted 8 lines
drivers/net/bonding/bonding.h +3 -0
+
+/**
+ * bond_for_each_slave_from_rcu - iterate the slaves list from a

starting point
+ * @bond: the bond holding this list.
+ * @pos: current slave.
+ * @start: starting point.
+ *

Quoted 8 lines

+ * Caller must hold rcu_read_lock
+ */
+#define bond_for_each_slave_from_rcu(bond, pos, start) \
+ for (pos = start; pos && (bond_is_only_slave(bond, start) ? \
+ &pos->list != &bond->slave_list : \
+ &pos->list != &start->list); bond_is_only_slave(bond, start) ? \
+ (pos = list_entry_rcu(pos->list.next, typeof(*pos), list)) : \
+ (pos = bond_next_slave_rcu(bond, pos)))

Ditto as bond_for_each_slave_from() and, also, see my comment about RCU from patch 1.

Quoted 8 lines

/**
* bond_for_each_slave - iterate over all slaves

Signature · 1 line
1.8.2.1
Signature · 3 lines
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