Re: [B.A.T.M.A.N.] [RFC 08/10] batman-adv: adapt the gateway feature to use the new API functions

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

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

On Wed, May 29, 2013 at 12:20:48AM +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@open-mesh.com>
> 
> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> ---
>  gateway_client.c | 74 +++++++++++++++++++++++++++++++-------------------------
>  main.h           |  1 +
>  2 files changed, 42 insertions(+), 33 deletions(-)
> 
> diff --git a/gateway_client.c b/gateway_client.c
> index 69488b2..0a9f1d0 100644
> --- a/gateway_client.c
> +++ b/gateway_client.c
> @@ -113,13 +113,13 @@ void batadv_gw_deselect(struct batadv_priv *bat_priv)
>  static struct batadv_gw_node *
>  batadv_gw_get_best_gw_node(struct batadv_priv *bat_priv)
>  {
> -	struct batadv_neigh_node *router;
> +	struct batadv_algo_ops *bao = bat_priv->bat_algo_ops;
>  	struct batadv_gw_node *gw_node, *curr_gw = NULL;
>  	uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
> -	uint32_t gw_divisor;
> -	uint8_t max_tq = 0;
> -	uint8_t tq_avg;
>  	struct batadv_orig_node *orig_node;
> +	struct batadv_neigh_node *router;
> +	uint32_t metric, max_metric = 0;
> +	uint32_t gw_divisor;
>  
>  	gw_divisor = BATADV_TQ_LOCAL_WINDOW_SIZE * BATADV_TQ_LOCAL_WINDOW_SIZE;
>  	gw_divisor *= 64;
> @@ -137,18 +137,19 @@ batadv_gw_get_best_gw_node(struct batadv_priv *bat_priv)
>  		if (!atomic_inc_not_zero(&gw_node->refcount))
>  			goto next;
>  
> -		tq_avg = router->bat_iv.tq_avg;
> +		metric = bao->bat_metric_get(router);
>  
>  		switch (atomic_read(&bat_priv->gw_sel_class)) {
>  		case 1: /* fast connection */
> -			tmp_gw_factor = tq_avg * tq_avg;
> +			tmp_gw_factor = metric * metric;

Hmm, that is rather strange ... I think fiddling with the metric directly
this way is weird when abstracting. For example:
 1.) Assuming we don't know how the metric looks like, we can't just
     multiplicate them. A logarithmic scaled metric or even arbritrary
     metric would look different compared to the linear metrics as we
     use now.
 2.) This might overflow because metric is u32 and tmp_gw_factor is too.
     It should work for batman IV where the metric is <256, but might
     not for BATMAN V.

I think this "bandwidth magic" should be abstracted as well somehow, if
we want to keep on using it that way.

>  			tmp_gw_factor *= gw_node->bandwidth_down;
>  			tmp_gw_factor *= 100 * 100;
>  			tmp_gw_factor /= gw_divisor;
>  
>  			if ((tmp_gw_factor > max_gw_factor) ||
>  			    ((tmp_gw_factor == max_gw_factor) &&
> -			     (tq_avg > max_tq))) {
> +			     (bao->bat_metric_compare(metric,
> +						      max_metric) > 0))) {
>  				if (curr_gw)
>  					batadv_gw_node_free_ref(curr_gw);
>  				curr_gw = gw_node;
> @@ -163,7 +164,7 @@ batadv_gw_get_best_gw_node(struct batadv_priv *bat_priv)
>  			  *     soon as a better gateway appears which has
>  			  *     $routing_class more tq points)
>  			  */
> -			if (tq_avg > max_tq) {
> +			if (bao->bat_metric_compare(metric, max_metric) > 0) {
>  				if (curr_gw)
>  					batadv_gw_node_free_ref(curr_gw);
>  				curr_gw = gw_node;
> @@ -172,8 +173,8 @@ batadv_gw_get_best_gw_node(struct batadv_priv *bat_priv)
>  			break;
>  		}
>  
> -		if (tq_avg > max_tq)
> -			max_tq = tq_avg;
> +		if (bao->bat_metric_compare(metric, max_metric) > 0)
> +			max_metric = metric;
>  
>  		if (tmp_gw_factor > max_gw_factor)
>  			max_gw_factor = tmp_gw_factor;
> @@ -229,22 +230,24 @@ void batadv_gw_election(struct batadv_priv *bat_priv)
>  				    NULL);
>  	} else if ((!curr_gw) && (next_gw)) {
>  		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> -			   "Adding route to gateway %pM (bandwidth: %u.%u/%u.%u MBit, tq: %i)\n",
> +			   "Adding route to gateway %pM (bandwidth: %u.%u/%u.%u MBit, metric: %u)\n",
>  			   next_gw->orig_node->orig,
>  			   next_gw->bandwidth_down / 10,
>  			   next_gw->bandwidth_down % 10,
>  			   next_gw->bandwidth_up / 10,
> -			   next_gw->bandwidth_up % 10, router->bat_iv.tq_avg);
> +			   next_gw->bandwidth_up % 10,
> +			   bat_priv->bat_algo_ops->bat_metric_get(router));
>  		batadv_throw_uevent(bat_priv, BATADV_UEV_GW, BATADV_UEV_ADD,
>  				    gw_addr);
>  	} else {
>  		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> -			   "Changing route to gateway %pM (bandwidth: %u.%u/%u.%u MBit, tq: %i)\n",
> +			   "Changing route to gateway %pM (bandwidth: %u.%u/%u.%u MBit, metric: %u)\n",
>  			   next_gw->orig_node->orig,
>  			   next_gw->bandwidth_down / 10,
>  			   next_gw->bandwidth_down % 10,
>  			   next_gw->bandwidth_up / 10,
> -			   next_gw->bandwidth_up % 10, router->bat_iv.tq_avg);
> +			   next_gw->bandwidth_up % 10,
> +			   bat_priv->bat_algo_ops->bat_metric_get(router));
>  		batadv_throw_uevent(bat_priv, BATADV_UEV_GW, BATADV_UEV_CHANGE,
>  				    gw_addr);
>  	}
> @@ -263,9 +266,10 @@ out:
>  void batadv_gw_check_election(struct batadv_priv *bat_priv,
>  			      struct batadv_orig_node *orig_node)
>  {
> -	struct batadv_orig_node *curr_gw_orig;
>  	struct batadv_neigh_node *router_gw = NULL, *router_orig = NULL;
> -	uint8_t gw_tq_avg, orig_tq_avg;
> +	struct batadv_algo_ops *bao = bat_priv->bat_algo_ops;
> +	struct batadv_orig_node *curr_gw_orig;
> +	uint16_t gw_metric, orig_metric;
>  
>  	curr_gw_orig = batadv_gw_get_selected_orig(bat_priv);
>  	if (!curr_gw_orig)
> @@ -283,23 +287,25 @@ void batadv_gw_check_election(struct batadv_priv *bat_priv,
>  	if (!router_orig)
>  		goto out;
>  
> -	gw_tq_avg = router_gw->bat_iv.tq_avg;
> -	orig_tq_avg = router_orig->bat_iv.tq_avg;
> +	gw_metric = bao->bat_metric_get(router_gw);
> +	orig_metric = bao->bat_metric_get(router_orig);
>  
> -	/* the TQ value has to be better */
> -	if (orig_tq_avg < gw_tq_avg)
> +	/* the metric has to be better */
> +	if (bao->bat_metric_compare(orig_metric, gw_metric) > 0)
>  		goto out;
>  
>  	/* if the routing class is greater than 3 the value tells us how much
> -	 * greater the TQ value of the new gateway must be
> +	 * greater the metric of the new gateway must be.
> +	 *
> +	 * FIXME: This comparison is strictly TQ related.
>  	 */
>  	if ((atomic_read(&bat_priv->gw_sel_class) > 3) &&
> -	    (orig_tq_avg - gw_tq_avg < atomic_read(&bat_priv->gw_sel_class)))
> +	    (orig_metric - gw_metric < atomic_read(&bat_priv->gw_sel_class)))
>  		goto out;
>  
>  	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> -		   "Restarting gateway selection: better gateway found (tq curr: %i, tq new: %i)\n",
> -		   gw_tq_avg, orig_tq_avg);
> +		   "Restarting gateway selection: better gateway found (metric curr: %u, metric new: %u)\n",
> +		   gw_metric, orig_metric);
>  
>  deselect:
>  	batadv_gw_deselect(bat_priv);
> @@ -503,11 +509,11 @@ static int batadv_write_buffer_text(struct batadv_priv *bat_priv,
>  
>  	curr_gw = batadv_gw_get_selected_gw_node(bat_priv);
>  
> -	ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %u.%u/%u.%u MBit\n",
> +	ret = seq_printf(seq, "%s %pM (%10u) %pM [%10s]: %u.%u/%u.%u MBit\n",
>  			 (curr_gw == gw_node ? "=>" : "  "),
>  			 gw_node->orig_node->orig,
> -			 router->bat_iv.tq_avg, router->addr,
> -			 router->if_incoming->net_dev->name,
> +			 bat_priv->bat_algo_ops->bat_metric_get(router),
> +			 router->addr, router->if_incoming->net_dev->name,
>  			 gw_node->bandwidth_down / 10,
>  			 gw_node->bandwidth_down % 10,
>  			 gw_node->bandwidth_up / 10,
> @@ -533,8 +539,8 @@ int batadv_gw_client_seq_print_text(struct seq_file *seq, void *offset)
>  		goto out;
>  
>  	seq_printf(seq,
> -		   "      %-12s (%s/%i) %17s [%10s]: advertised uplink bandwidth ... [B.A.T.M.A.N. adv %s, MainIF/MAC: %s/%pM (%s)]\n",
> -		   "Gateway", "#", BATADV_TQ_MAX_VALUE, "Nexthop", "outgoingIF",
> +		   "      %-12s (%-4s) %17s [%10s]: advertised uplink bandwidth ... [B.A.T.M.A.N. adv %s, MainIF/MAC: %s/%pM (%s)]\n",
> +		   "Gateway", "#", "Nexthop", "outgoingIF",
>  		   BATADV_SOURCE_VERSION, primary_if->net_dev->name,
>  		   primary_if->net_dev->dev_addr, net_dev->name);
>  
> @@ -707,12 +713,13 @@ bool batadv_gw_is_dhcp_target(struct sk_buff *skb, unsigned int *header_len)
>  bool batadv_gw_out_of_range(struct batadv_priv *bat_priv,
>  			    struct sk_buff *skb, struct ethhdr *ethhdr)
>  {
> +	struct batadv_algo_ops *bao = bat_priv->bat_algo_ops;
>  	struct batadv_neigh_node *neigh_curr = NULL, *neigh_old = NULL;
>  	struct batadv_orig_node *orig_dst_node = NULL;
>  	struct batadv_gw_node *gw_node = NULL, *curr_gw = NULL;
>  	bool ret, out_of_range = false;
>  	unsigned int header_len = 0;
> -	uint8_t curr_tq_avg;
> +	uint32_t curr_metric;
>  	unsigned short vid;
>  
>  	vid = batadv_get_vid(skb, 0);
> @@ -739,7 +746,7 @@ bool batadv_gw_out_of_range(struct batadv_priv *bat_priv,
>  		/* If we are a GW then we are our best GW. We can artificially
>  		 * set the tq towards ourself as the maximum value
>  		 */
> -		curr_tq_avg = BATADV_TQ_MAX_VALUE;
> +		curr_metric = BATADV_MAX_METRIC;
>  		break;
>  	case BATADV_GW_MODE_CLIENT:
>  		curr_gw = batadv_gw_get_selected_gw_node(bat_priv);
> @@ -759,7 +766,7 @@ bool batadv_gw_out_of_range(struct batadv_priv *bat_priv,
>  		if (!neigh_curr)
>  			goto out;
>  
> -		curr_tq_avg = neigh_curr->bat_iv.tq_avg;
> +		curr_metric = bao->bat_metric_get(neigh_curr);
>  		break;
>  	case BATADV_GW_MODE_OFF:
>  	default:
> @@ -770,7 +777,8 @@ bool batadv_gw_out_of_range(struct batadv_priv *bat_priv,
>  	if (!neigh_old)
>  		goto out;
>  
> -	if (curr_tq_avg - neigh_old->bat_iv.tq_avg > BATADV_GW_THRESHOLD)
> +	if (bao->bat_metric_is_similar(bao->bat_metric_get(neigh_old),
> +				       curr_metric))
>  		out_of_range = true;

Hmm ... if you add the abs for metric_is_similar as suggested for the patch introducing
the function, this one would fail. For BATMAN IV, curr_metric would be 0xffffffff and
the neigh_old would be something < 256, making this function fail for the BATADV_GW_MODE_SERVER
case. 

Actually BATADV_GW_MODE_SERVER could just set out_of_range and goto out, I don't understand
the purpose of this "artificially setting the tq towards ourself" is good for.

>  
>  out:
> diff --git a/main.h b/main.h
> index 1d3611f..8d69641 100644
> --- a/main.h
> +++ b/main.h
> @@ -31,6 +31,7 @@
>  
>  /* B.A.T.M.A.N. parameters */
>  
> +#define BATADV_MAX_METRIC 0xffffffff
>  #define BATADV_TQ_MAX_VALUE 255
>  #define BATADV_JITTER 20
>  
> -- 
> 1.8.1.5
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]