On Tue, Oct 01, 2013 at 11:24:19AM +0200, Antonio Quartulli wrote:
- /* bonding: loop through the list of possible routers found
* for the various outgoing interfaces and find a candidate after* the last chosen bonding candidate (next_candidate). If no such* router is found, use the first candidate found (the last chosen* bonding candidate might have been the last one in the list).* If this can't be found either, return the previously choosen* router - obviously there are no other candidates.*/- rcu_read_lock();
- router_ifinfo = batadv_neigh_node_get_ifinfo(router, recv_if);
- hlist_for_each_entry_rcu(cand, &orig_node->ifinfo_list, list) {
cand_router = rcu_dereference(cand->router);if (!cand_router)continue;if (!atomic_inc_not_zero(&cand_router->refcount))continue;cand_ifinfo = batadv_neigh_node_get_ifinfo(cand_router,cand->if_outgoing);if (!cand_ifinfo)goto next;/* alternative candidate should be good enough to be* considered*/if (!bao->bat_neigh_ifinfo_is_equiv_or_better(cand_ifinfo,router_ifinfo))goto next;/* don't use the same router twice */if (orig_node->last_bonding_candidate &&(orig_node->last_bonding_candidate->router ==cand_router))goto next;/* mark the first possible candidate */if (!first_candidate) {atomic_inc(&cand_router->refcount);first_candidate = cand;first_candidate_router = cand_router;}What if you change this:
/* check if already passed the next candidate ... this function* should get the next candidate AFTER the last used bonding* candidate.*/if (orig_node->last_bonding_candidate) {if (orig_node->last_bonding_candidate == cand) {last_candidate_found = true;goto next;}if (!last_candidate_found)goto next;}next_candidate = cand;next_candidate_router = cand_router;break;+next:
batadv_neigh_node_free_ref(cand_router);to this:
if (!orig_node->last_bonding_candidate || last_candidate_found) { next_candidate = cand; next_candidate_router = cand_router; break; }
if (orig_node->last_bonding_candidate == cand) last_candidate_found = true;
batadv_neigh_node_free_ref(cand_router);
? (I hope the two are equivalent..I took some time to fully get the flow of your if/else block)
Removing the "goto next" makes me look at it in a more linear way (and we also reduce the max indentation by one tab).
Yup, that should work too, good suggestion. We need to keep the next label though. I'll change it according to your suggestion.
diff --git a/types.h b/types.h index 7255500..fa58a14 100644 --- a/types.h +++ b/types.h @@ -173,6 +173,7 @@ struct batadv_orig_bat_iv {
- @orig: originator ethernet address
- @primary_addr: hosts primary interface address
- @ifinfo_list: list for routers per outgoing interface
- @last_bonding_candidate: pointer to last ifinfo of last used router
- @batadv_dat_addr_t: address of the orig node in the distributed hash
- @last_seen: time when last packet from this node was received
- @bcast_seqno_reset: time when the broadcast seqno window was reset
@@ -213,6 +214,7 @@ struct batadv_orig_node { uint8_t orig[ETH_ALEN]; uint8_t primary_addr[ETH_ALEN]; struct hlist_head ifinfo_list;
- struct batadv_orig_node_ifinfo *last_bonding_candidate;
Do you think it is not needed to set this field to NULL when disabling bonding mode? Can an old value trigger some strange behaviour when bonding is turned on again? Maybe not..just wondering..
Hmm, maybe, can't say now but will look into that.
Thanks for all your suggestions! Simon