Re: [B.A.T.M.A.N.] [RFC 05/10] batman-adv: add bat_metric_is_similar API function

[Antonio Quartulli <ordex@autistici.org>]

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

On Wed, May 29, 2013 at 04:55:19PM +0200, Simon Wunderlich wrote:
> On Wed, May 29, 2013 at 04:28:51PM +0200, Antonio Quartulli wrote:
> > On Wed, May 29, 2013 at 04:16:57PM +0200, Simon Wunderlich wrote:
> > > On Wed, May 29, 2013 at 12:20:45AM +0200, Antonio Quartulli wrote:
> > > > From: Antonio Quartulli <antonio@open-mesh.com>
> > > > 
> > > > Each routing protocol has its own metric semantic and
> > > > therefore is the protocol itself the only component able to
> > > > compare two metrics to check similarity similarity.
> > > > 
> > > > This new API allows each routing protocol to implement its
> > > > own logic and make the external code protocol agnostic.
> > > > 
> > > > Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> > > > ---
> > > >  bat_iv_ogm.c | 7 +++++++
> > > >  main.c       | 3 ++-
> > > >  main.h       | 6 ++++++
> > > >  types.h      | 3 +++
> > > >  4 files changed, 18 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/bat_iv_ogm.c b/bat_iv_ogm.c
> > > > index abf4cd3..18c9ae8 100644
> > > > --- a/bat_iv_ogm.c
> > > > +++ b/bat_iv_ogm.c
> > > > @@ -1441,6 +1441,12 @@ static uint32_t batadv_iv_ogm_metric_get(struct batadv_neigh_node *neigh_node)
> > > >  	return neigh_node->bat_iv.tq_avg;
> > > >  }
> > > >  
> > > > +static bool batadv_iv_ogm_metric_is_similar(uint32_t metric,
> > > > +					    uint32_t new_metric)
> > > > +{
> > > > +	return (metric - new_metric < BATADV_TQ_SIMILARITY_THRESHOLD);
> > > 
> > > You might want to use abs(metric - new_metric) here, otherwise is_similar(a, b) output
> > > might differ from is_similar(b, a).
> > 
> > Mh..imho the name of the function is bad because this has been done on purpose.
> 
> agreed, the function name is not really good. You could rename it to something like
> "is_almost_or_better()" if you keep the current semantics, although this is not an
> "easy name" either. Or rename it to "similar_or_greater()". Something like that
> 
> Although ...
> > 
> > The idea is that we want to see if 'b' is at least
> > as good as 'a', therefore what we want to check if is b is greater than
> > 'a - threshold' only.
> > 
> > Imagine that 'b' is greater than (a + threshold), for me the function has to
> > return true, because the metric b is at least as good as a, but if I introduce
> > the abs() the function would return false.
> > 
> > Example:
> > 
> > a=190
> > b=240
> > threshold=20
> > 
> > a - b = -50 < 20 => b is at least as good as a!
> > 
> > using abs:
> > 
> > abs(a - b) = 50 < 20 => NO! b is worse than a....and this is not true.
> > 
> > 
> > this situation can happen in the code because usually 'a' will represents some
> > kind of current metric and this is not supposed to be the best ever (maybe we
> > still have to switch to a new best).
> 
> ... we actually compare to the biggest value (e.g. highest gateway rank, highest
> bonding candidate), so I wonder if this can really happen. So adding abs() would
> just make the semantics more clear without breaking the current code when we simply
> replace. Although we need to check all occurences again, I'm not completely sure
> that it's always compared to the greatest member.

well the current code uses the semantic that I implemented in the API (IIRC) and
this is why I've done so: I wanted to keep the very same behaviour.

I'm also not entirely sure that we always compare to the greatest member.

What you are thinking about is the compare() function taking a threshold as
parameter. We can do that with the compare() API if you want, but I think we
should keep this "~is_similar()" API in order to avoid behavioural changes in
the code.


Cheers,

-- 
Antonio Quartulli

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

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