Re: [B.A.T.M.A.N.] [PATCH v2 5/5] batman-adv: B.A.T.M.A.N. V - implement GW selection logic

[Marek Lindner <mareklindner@neomailbox.ch>]

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

On Tuesday, May 10, 2016 18:06:09 Antonio Quartulli wrote:
> +static int batadv_v_gw_throughput_get(struct batadv_gw_node *gw_node, u32
> *bw)

> +static struct batadv_gw_node *
> +batadv_v_gw_get_best_gw_node(struct batadv_priv *bat_priv)

Kernel doc ?


> +	struct batadv_gw_node *gw_node, *curr_gw = NULL;
> +	u32 max_bw = 0, bw, threshold;
> +
> +	threshold = atomic_read(&bat_priv->gw.sel_class);
> +
> +	rcu_read_lock();
> +	hlist_for_each_entry_rcu(gw_node, &bat_priv->gw.list, list) {
> +		if (!kref_get_unless_zero(&gw_node->refcount))
> +			continue;
> +
> +		if (batadv_v_gw_throughput_get(gw_node, &bw) < 0)
> +			goto next;
> +
> +		if (curr_gw && (bw <= (max_bw + threshold)))
> +			goto next;
> +
> +		if (curr_gw)
> +			batadv_gw_node_put(curr_gw);
> +
> +		curr_gw = gw_node;
> +		kref_get(&curr_gw->refcount);
> +		max_bw = bw;
> +
> +next:
> +		batadv_gw_node_put(gw_node);
> +	}
> +	rcu_read_unlock();
> +
> +	return curr_gw;
> +}

I don't quite follow the use of 'threshold' in this function. The threshold is 
meant to decide whether the switch to another gateway is worth breaking the 
existing stateful connections. Here, the code is retrieving the best gateway 
of all - no need for the threshold ? Moreover, the threshold might lead to 
unpredictable results. If the threshold is 5 MBit/s and the first gateway in 
the list offers 1 MBit/s while the second offers 5 MBit/s the better gateway 
is never chosen. 


> +static bool batadv_v_gw_is_eligible(struct batadv_priv *bat_priv,
> +				    struct batadv_orig_node *curr_gw_orig,
> +				    struct batadv_orig_node *orig_node)

> +/* fails if orig_node has no router */
> +static int batadv_v_gw_write_buffer_text(struct batadv_priv *bat_priv,
> +					 struct seq_file *seq,
> +					 const struct batadv_gw_node *gw_node)

> +static void batadv_v_gw_print(struct batadv_priv *bat_priv,
> +			      struct seq_file *seq)

More room for kernel doc ..


> @@ -387,7 +569,16 @@ static struct batadv_algo_ops batadv_batman_v
> __read_mostly = { */
>  int batadv_v_mesh_init(struct batadv_priv *bat_priv)
>  {
> -	return batadv_v_ogm_init(bat_priv);
> +	int ret = 0;
> +
> +	ret = batadv_v_ogm_init(bat_priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* set default throughput difference threshold to 5Mbps */
> +	atomic_set(&bat_priv->gw.sel_class, 50);
> +
> +	return 0;
>  }

There is no need to initial 'ret' if the next line is setting a proper value.
If you moved the atomic_set() above the batadv_v_ogm_init() call most of the 
chachacha would go away.

In batadv_softif_init_late() bat_priv->gw.sel_class is initialized for 
B.A.T.M.A.N. IV. Do you intend to keep it there ? If not, a per-routing 
algorithm init might be cleaner ..  :)

Cheers,
Marek

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