Re: [B.A.T.M.A.N.] [PATCHv3 6/9] batman-adv: adapt bonding to use the new API functions

[Marek Lindner <lindner_marek@yahoo.de>]

On Tuesday, August 13, 2013 14:43:49 Antonio Quartulli wrote:
> -void batadv_bonding_candidate_add(struct batadv_orig_node *orig_node,
> +void batadv_bonding_candidate_add(struct batadv_priv *bat_priv,
> +				  struct batadv_orig_node *orig_node,
>  				  struct batadv_neigh_node *neigh_node)
>  {

Kernel doc ?


> @@ -133,8 +136,9 @@ void batadv_bonding_candidate_add(struct
> batadv_orig_node *orig_node, goto candidate_del;
> 
>  	/* ... and is good enough to be considered */
> -	if (neigh_node->bat_iv.tq_avg <
> -	    router->bat_iv.tq_avg - BATADV_BONDING_TQ_THRESHOLD)
> +	router_metric = bao->bat_metric_get(router);
> +	neigh_metric = bao->bat_metric_get(neigh_node);
> +	if (bao->bat_metric_is_equiv_or_better(neigh_metric, router_metric))
>  		goto candidate_del;

[..]


>  	/* check if we have another candidate with the same mac address or
> @@ -486,11 +490,14 @@ out:
>   * Increases the returned router's refcount
>   */
>  static struct batadv_neigh_node *
> -batadv_find_ifalter_router(struct batadv_orig_node *primary_orig,
> +batadv_find_ifalter_router(struct batadv_priv *bat_priv,
> +			   struct batadv_orig_node *primary_orig,
>  			   const struct batadv_hard_iface *recv_if)
>  {

Kernel doc ?


> -	struct batadv_neigh_node *tmp_neigh_node;
>  	struct batadv_neigh_node *router = NULL, *first_candidate = NULL;
> +	struct batadv_algo_ops *bao = bat_priv->bat_algo_ops;
> +	struct batadv_neigh_node *tmp_neigh_node;
> +	uint32_t tmp_metric, router_metric = UINT_MAX;
> 
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(tmp_neigh_node, &primary_orig->bond_list,
> @@ -502,8 +509,8 @@ batadv_find_ifalter_router(struct batadv_orig_node
> *primary_orig, if (tmp_neigh_node->if_incoming == recv_if)
>  			continue;
> 
> -		if (router &&
> -		    tmp_neigh_node->bat_iv.tq_avg <= router->bat_iv.tq_avg)
> +		tmp_metric = bao->bat_metric_get(tmp_neigh_node);
> +		if (router && (tmp_metric <= router_metric))
>  			continue;

This last comparison looks horribly wrong. Isn't that what we have the API 
for?

Besides, it seems to me the bat_metric_is_equiv_or_better() call has been 
misdesigned. Instead of passing the metric and having each function store + 
process the metric this call should accept 2 neighbor nodes as argument. This 
should save 2 out of 3 API calls (2 * bao->bat_metric_get()) and some code in 
all the functions needing the bat_metric_is_equiv_or_better() API. Moreover, 
it should reduce the impact on performance because each callback will cost us 
a little bit ..

Cheers,
Marek