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

[Antonio Quartulli <ordex@autistici.org>]

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

On Wed, May 29, 2013 at 04:32:21PM +0200, Simon Wunderlich wrote:
> 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.

I totally agree. Thanks for your feedback, but I don't really know how I could
abstract this behaviour. This is totally GW related, but hardcoded around the
metric semantic. here we would need a routing API that should be implemented by
the GW component....mhhh..suggestions? :D

[...]

> >  
> > -	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. 

mh I see. the problem is in the max value being wrong for the TQ metric....mh we
have to think about that.

> 
> 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.

This behaviour has been introduced to avoid killing client connections while
they are roaming from node to node. As I explained on IRC this is the wanted
behaviour.

Cheers,



-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

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