bonding: restructure and add rcu for bond_for_each_slave_next()

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

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

Ding Tianhong

author

2013-09-05 07:49 UTC #0 lore raw

PATCH v3 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.

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

Cc Nikolay Aleksandrov <[email protected]>

Cc Veaceslav Falico <[email protected]>

---
 drivers/net/bonding/bond_alb.c  |  3 +--
 drivers/net/bonding/bond_main.c |  6 ++----
 drivers/net/bonding/bonding.h   | 23 +++++++++++++++++++----
 3 files changed, 22 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 +19 -4

index f013b12..48fd41a 100644
@@ -123,18 +123,33 @@
 	(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 */
+
 /**
  * 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 && &pos->list != &bond->slave_list; \
+	    (pos = bond_next_slave(bond, pos)) != start ? pos : \
+	    (pos = list_entry(&bond->slave_list, typeof(*pos), list)))
+
+/**
+ * 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 && &pos->list != &bond->slave_list; \
+	    (pos = bond_next_slave_rcu(bond, pos)) != start ? pos : \
+	    (pos = list_entry_rcu(&bond->slave_list, typeof(*pos), list)))
 
 /**
  * bond_for_each_slave - iterate over all slaves

Signature · 1 line

1.8.2.1

Veaceslav Falico

2013-09-05 13:32 UTC #1 lore raw

Quoted 88 lines · Ding Tianhong

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

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

Cc Nikolay Aleksandrov <[email protected]>

Cc Veaceslav Falico <[email protected]>

---

drivers/net/bonding/bond_alb.c  |  3 +--
drivers/net/bonding/bond_main.c |  6 ++----
drivers/net/bonding/bonding.h   | 23 +++++++++++++++++++----
3 files changed, 22 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 +2 -1

index f013b12..48fd41a 100644
@@ -123,18 +123,33 @@
 	(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 */
+
 /**
 * 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.

drivers/net/bonding/bonding.h line 139

Quoted 10 lines

drivers/net/bonding/bonding.h +4 -3

 *
 * 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 && &pos->list != &bond->slave_list; \
+	    (pos = bond_next_slave(bond, pos)) != start ? pos : \
+	    (pos = list_entry(&bond->slave_list, typeof(*pos), list)))

Do you understand the differences of these two implementations?

pos && cnt < (bond)->slave_cnt

pos && &pos->list != &bond->slave_list

I'm sorry, but you should listen to comments.

Quoted 21 lines

drivers/net/bonding/bonding.h +13 -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.
+ *
+ * Caller must hold rcu_read_lock
+ */
+#define bond_for_each_slave_from_rcu(bond, pos, start) \
+	for (pos = start; pos && &pos->list != &bond->slave_list; \
+	    (pos = bond_next_slave_rcu(bond, pos)) != start ? pos : \
+	    (pos = list_entry_rcu(&bond->slave_list, typeof(*pos), list)))
 
 /**
 * bond_for_each_slave - iterate over all slaves

Signature · 1 line

1.8.2.1

Ding Tianhong

2013-09-05 14:39 UTC #2 lore raw

Quoted 107 lines · Veaceslav Falico

Quoted 93 lines · Ding Tianhong

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

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

Cc Nikolay Aleksandrov <[email protected]>

Cc Veaceslav Falico <[email protected]>

---
drivers/net/bonding/bond_alb.c | 3 +-- drivers/net/bonding/bond_main.c | 6 ++---- drivers/net/bonding/bonding.h | 23 +++++++++++++++++++----

3 files changed, 22 insertions(+), 10 deletions(-)

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

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;

drivers/net/bonding/bond_alb.c +1 -1

@@ -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 +1 -2

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 &&

drivers/net/bonding/bond_main.c +1 -2

@@ -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 +2 -1

index f013b12..48fd41a 100644
@@ -123,18 +123,33 @@
 (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 */
+
 /**
 * 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.

drivers/net/bonding/bonding.h line 139

Quoted 10 lines

drivers/net/bonding/bonding.h +4 -3

 *
 * 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 && &pos->list != &bond->slave_list; \
+ (pos = bond_next_slave(bond, pos)) != start ? pos : \
+ (pos = list_entry(&bond->slave_list, typeof(*pos), list)))

Do you understand the differences of these two implementations?

Quoted 8 lines

pos && cnt < (bond)->slave_cnt
vs
pos && &pos->list != &bond->slave_list
I'm sorry, but you should listen to comments.

if the slave is only one, it will run once, and out the loop, else if the slave is more than one, it will run until reach the start again, here I use the &bond->slave_list to control the loop, as it will not change any time, maybe it is hard to understand first.

ok, I think they are same, just in the purpose to remove the cnt, so maybe it make you unconfortable, sorry about that, if you strangly disagree, I will miss this patch, but thanks again for your different opinions. :)

Quoted 27 lines

Quoted 22 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.
+ *
+ * Caller must hold rcu_read_lock
+ */
+#define bond_for_each_slave_from_rcu(bond, pos, start) \
+ for (pos = start; pos && &pos->list != &bond->slave_list; \
+ (pos = bond_next_slave_rcu(bond, pos)) != start ? pos : \
+ (pos = list_entry_rcu(&bond->slave_list, typeof(*pos), list)))
/**
* 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