[PATCH net-next v2 5/6] bonding: restructure and add rcu for bond_for_each_slave_next()

[PATCH net-next v2 5/6] bonding: restructure and add rcu for bond_for_each_slave_next()

[Ding Tianhong]

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

Suggested-by: Nikolay Aleksandrov <nikolay@redhat.com>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Cc: Nikolay Aleksandrov <nikolay@redhat.com>
---
 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(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 91f179d..c75d383 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -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;
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 39e5b1c..4190389 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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);
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index e97eee9..80b170a 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -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
-- 
1.8.2.1

Re: [PATCH net-next v2 5/6] bonding: restructure and add rcu for bond_for_each_slave_next()

[Veaceslav Falico]

On Wed, Sep 04, 2013 at 05:44:15PM +0800, Ding Tianhong wrote:
...snip...
>+/* 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.

>+
> /**
>  * 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)))

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.

>+
>+/**
>+ * 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)))

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

>
> /**
>  * bond_for_each_slave - iterate over all slaves
>-- 
>1.8.2.1
>
>
>

Re: [PATCH net-next v2 5/6] bonding: restructure and add rcu for bond_for_each_slave_next()

[Ding Tianhong]

于 2013/9/4 18:35, Veaceslav Falico 写道:
> On Wed, Sep 04, 2013 at 05:44:15PM +0800, Ding Tianhong wrote:
> ...snip...
>> +/* 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.
>
yes, more simple more beautiful, thanks.

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

>> +
>> /**
>> * 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)))
>
> 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.
>

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. :)

>> +
>> +/**
>> + * 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)))
>
> Ditto as bond_for_each_slave_from() and, also, see my comment about RCU
> from patch 1.
>
>>
>> /**
>> * bond_for_each_slave - iterate over all slaves
>> -- 
>> 1.8.2.1
>>
>>
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH net-next v2 5/6] bonding: restructure and add rcu for bond_for_each_slave_next()

[Veaceslav Falico]

On Wed, Sep 04, 2013 at 11:02:28PM +0800, Ding Tianhong wrote:
>于 2013/9/4 18:35, Veaceslav Falico 写道:
>>On Wed, Sep 04, 2013 at 05:44:15PM +0800, Ding Tianhong wrote:
>>...snip...
>>>+/* 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.
>>
>yes, more simple more beautiful, thanks.
>
>but if the pos is not our slave, it is the mistake, not bug. :)
>
>>>+
>>>/**
>>>* 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)))
>>
>>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.
>>
>
>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

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

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

>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).

>
>by the way, I test the function and works well. :)
>
>>>+
>>>+/**
>>>+ * 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)))
>>
>>Ditto as bond_for_each_slave_from() and, also, see my comment about RCU
>>from patch 1.
>>
>>>
>>>/**
>>>* bond_for_each_slave - iterate over all slaves
>>>-- 
>>>1.8.2.1
>>>
>>>
>>>
>>-- 
>>To unsubscribe from this list: send the line "unsubscribe netdev" in
>>the body of a message to majordomo@vger.kernel.org
>>More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>