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

Marek Lindner
2013-08-26T03:11:37Z

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