[RFCv2,6/6] batman-adv: add bonding again

  From: Simon Wunderlich <simon@open-mesh.com>

With the new interface alternating, the first hop may send packets
in a round robin fashion to it's neighbors because it has multiple
valid routes built by the multi interface optimization. This patch
enables the feature if bonding is selected. Note that unlike the
bonding implemented before, this version is much simpler and may
even enable multi path routing to a certain degree.

Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
---
 routing.c |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 routing.h |    2 +-
 types.h   |    2 ++
 3 files changed, 104 insertions(+), 4 deletions(-)
  

On Wed, Sep 04, 2013 at 06:27:40PM +0200, Simon Wunderlich wrote:
> From: Simon Wunderlich <simon@open-mesh.com>
> 
> With the new interface alternating, the first hop may send packets
> in a round robin fashion to it's neighbors because it has multiple
> valid routes built by the multi interface optimization. This patch
> enables the feature if bonding is selected. Note that unlike the
> bonding implemented before, this version is much simpler and may
> even enable multi path routing to a certain degree.

y0! :)

> 
> Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
> ---
>  routing.c |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  routing.h |    2 +-
>  types.h   |    2 ++
>  3 files changed, 104 insertions(+), 4 deletions(-)
> 
> diff --git a/routing.c b/routing.c
> index 304e44b..013958f 100644
> --- a/routing.c
> +++ b/routing.c
> @@ -407,16 +407,114 @@ static int batadv_check_unicast_packet(struct batadv_priv *bat_priv,

[..]

>  {
> -	struct batadv_neigh_node *router;
> +	struct batadv_algo_ops *bao = bat_priv->bat_algo_ops;
> +	struct batadv_neigh_node *router, *cand_router,
> +				 *first_candidate_router = NULL,
> +				 *next_candidate_router;

Can you avoid this strange declaration style? :)
One variable (with its type) per line.

> +	struct batadv_neigh_node_ifinfo *router_ifinfo, *cand_ifinfo;
> +	struct batadv_orig_node_ifinfo *cand, *first_candidate = NULL,
> +				       *next_candidate = NULL;

here too.

[...]

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

> +	}
> +
> +	if (next_candidate) {
> +		/* found a possible candidate after the last chosen bonding
> +		 * candidate, return it.
> +		 */
> +		batadv_neigh_node_free_ref(router);
> +		if (first_candidate)
> +			batadv_neigh_node_free_ref(first_candidate_router);
> +		router = next_candidate_router;
> +		orig_node->last_bonding_candidate = next_candidate;
> +	} else if (first_candidate) {
> +		/* found no possible candidate after the last candidate, return
> +		 * the first candidate if available, or the already selected
> +		 * router otherwise.
> +		 */
> +		batadv_neigh_node_free_ref(router);
> +		router = first_candidate_router;
> +		orig_node->last_bonding_candidate = first_candidate;
> +	} else {
> +		orig_node->last_bonding_candidate = NULL;
> +	}
> +

Also this if/else block could be simplified a little bit? But still it is
readable and understandable :)

> +	rcu_read_unlock();
>  
>  	return router;
>  }

[..]

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

Cheers,
  

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