Re: [B.A.T.M.A.N.] [RFCv2 6/6] batman-adv: add bonding again

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

[-- Attachment #1: Type: text/plain, Size: 4191 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]