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
signature.asc
Description: Digital signature
