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

[Marek Lindner <mareklindner@neomailbox.ch>]

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

On Wednesday 09 October 2013 15:05:37 Simon Wunderlich wrote:
> --- a/routing.c
> +++ b/routing.c
> @@ -407,16 +407,104 @@ static int batadv_check_unicast_packet(struct
> batadv_priv *bat_priv, struct batadv_neigh_node *
>  batadv_find_router(struct batadv_priv *bat_priv,
>  		   struct batadv_orig_node *orig_node,
> -		   const struct batadv_hard_iface *recv_if)
> +		   struct batadv_hard_iface *recv_if)
>  {
> -	struct batadv_neigh_node *router;
> +	struct batadv_algo_ops *bao = bat_priv->bat_algo_ops;
> +	struct batadv_neigh_node *first_candidate_router = NULL;
> +	struct batadv_neigh_node *next_candidate_router;
> +	struct batadv_neigh_node *router, *cand_router;
> +	struct batadv_orig_node_ifinfo *cand, *first_candidate = NULL;
> +	struct batadv_orig_node_ifinfo *next_candidate = NULL;
> +	bool last_candidate_found = false;
> 
>  	if (!orig_node)
>  		return NULL;
> 
>  	router = batadv_orig_node_get_router(orig_node, recv_if);
> 
> -	/* TODO: fill this later with new bonding mechanism */
> +	/* only consider bonding for recv_if == NULL (first hop) and if
> +	 * activated.
> +	 */
> +	if (recv_if || !atomic_read(&bat_priv->bonding) || !router)
> +		return router;
> +
> +	/* 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();
> +	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;
> +
> +		/* alternative candidate should be good enough to be
> +		 * considered
> +		 */
> +		if (!bao->bat_neigh_is_equiv_or_better(cand_router,
> +						       cand->if_outgoing,
> +						       router, recv_if))
> +			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;

Again, this if-statement won't pass.


> +		/* check if already passed the next candidate ... this function
> +		 * should the next candidate AFTER the last used bonding
> +		 * candidate.
> +		 */
> +		if (!orig_node->last_bonding_candidate ||
> +		    last_candidate_found) {
> +			next_candidate = cand;
> +			next_candidate_router = cand_router;
> +			break;
> +		}

The comment above would benefit from additional love. :)


> +	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;
> +	}

It is not safe to hold a pointer to first_candidate or next_candidate without 
having a refcounter protecting it.

Cheers,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]