| Message ID | 522700EF.3000702@huawei.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 > > > -- 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
于 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 > -- 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
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 >> > -- 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
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
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(-)