batman-adv: Fixing wrap-around bug in vis

  When the seqno for a vis packet had a wrap around from i.e. 255 to 0,
add_packet() would falsely claim the older packet with the seqno 255 as
newer as the one with the seqno of 0 and would therefore ignore the new
packet. This happens with all following vis packets until the old vis
packet expires after 180 seconds timeout. This patch fixes this issue
and gets rid of these highly undesired 3min. breaks for the vis-server.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 batman-adv-kernelland/packet.h |    3 ++-
 batman-adv-kernelland/vis.c    |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)
  

> +		if ((vis_packet->seqno - old_info->packet.seqno) > 127) {
If anyone might have a more generic solution instead of using the
fixed of 127 without having to utilise math.h, that'd be great.

Ok, and no some more words to the tests and discussions with Marek
yesterday and their results.

> What I also just noticed is, that when I refresh the vis-output
> every second only the TQ values of _2_ entries at a time  get updated,
> the other ones are static for quite a while. However, looking at
> wireshark tells me, that I'm still getting a vis-packet of all
> other 4 nodes                                                                                 
> every second. So it looks like the vis-server might not be always
> updating its vis_hash to me, but not sure about that.
The issue seemed to be the thing I just sent a patch for. Pfeh,
then I didn't see any ghost bug which was sometimes there and
sometimes not. I started to doubt about the setup a lot first :).

> I'd expect all 5 nodes to have a direct connection to all other 4
> nodes. However, for instance 06:22:b0:98:87:f9 is just having
> three. A couple of seconds later, the missing entry is there
> again, but other ones can be missing.
The line which is causing this behavior is in generate_vis_packet():
------
398                 if (orig_node->router != NULL
399                         && compare_orig(orig_node->router->addr,
400                                         orig_node->orig)
------
It turned out that this is not a bug and that it was intended to
leave out those vis entries. Looks like I had slightly different
expectations at the vis-server. I wanted to see all possible links
and alternative next-hops in the vis-graph while Marek enlightened
me, that this would increase the throughput usage a lot as for
instance a far distant node, from which we'd receive a batman-ogm
up to every 120 seconds would result into a vis-entry then.
So including alternative/non-used next-hops in the vis-output
might need a new discussion.

Cheers, Linus
  

Linus Lüssing wrote:
> -               if (vis_packet->seqno - old_info->packet.seqno <= 0) {
> +               if (vis_packet->seqno - old_info->packet.seqno
> +                   <= 1 << 7 * sizeof(vis_packet->seqno)) {

Shouldn't that be 
 1 << 7 + 8 * (sizeof(vis_packet->seqno) - 1))?

Otherwise you would have left the the sizeof(vis_packet->seqno) most 
significant bits 0 and the rest one. And maybe a seq_before/seq_after static 
inline function could be created to make this thing a lot more readable. You 
could also do that in a macro if you don't want to assume the type of 
vis_packet->seqno... which is probably what you tried by using sizeof

#define seq_before(x,y) ((x - y) >= 1 << 7 + 8 * (sizeof(x) - 1))
#define seq_after(x,y) ((y - x) >= 1 << 7 + 8 * (sizeof(x) - 1))
....

And maybe you see here that the equal part which is needed bellow that line 
isn't possible. So it must be changed to 

if (!seq_after(vis_packet->seqno, old_info->packet.seqno))

I hope that this is right - please check twice.


Best regards,
	Sven
  

Sven Eckelmann wrote:
> #define seq_before(x,y) ((x - y) >= 1 << 7 + 8 * (sizeof(x) - 1))
> #define seq_after(x,y) ((y - x) >= 1 << 7 + 8 * (sizeof(x) - 1))
> ....

That must be 
#define seq_before(x,y) ((x - y) > 1 << 7 + 8 * (sizeof(x) - 1))
#define seq_after(x,y) ((y - x) > 1 << 7 + 8 * (sizeof(x) - 1))

Or otherwise it would be wrong balanced between equal, after and before. In 
other words: For every x in N0 and y in N0 is only one true of (x == y) or 
seq_before(x, y) or seq_after(x, y) after the change of >= to >

Best regards,
	Sven
  

Sven Eckelmann wrote:
> That must be
> #define seq_before(x,y) ((x - y) > 1 << 7 + 8 * (sizeof(x) - 1))
> #define seq_after(x,y) ((y - x) > 1 << 7 + 8 * (sizeof(x) - 1))

Nearly correct but accidentally  deleted a =

#define seq_before(x,y) ((x - y) > 1 << 7 + 8 * (sizeof(x) - 1))
#define seq_after(x,y) ((y - x) >= 1 << 7 + 8 * (sizeof(x) - 1))

Best regards,
	Sven
  

Sorry still does not seem to work :). Despite testing it on a
setup in batman itself I also tested it with a small c program
(which does not work as expected either):
-----
#include <stdio.h>
#include <stdint.h>

#define seq_before(x,y) ((x - y) > 1 << 7 + 8 * (sizeof(x) - 1))
#define seq_after(x,y) ((y - x) >= 1 << 7 + 8 * (sizeof(x) - 1))

int main() {
        uint8_t old = 0;
        uint8_t new = 1;
        if (!seq_after(new,old))
                printf("foobar\n");

        return 0;
}
-----
$ ./test
foobar
$

On Fri, Mar 12, 2010 at 12:04:47AM +0100, Sven Eckelmann wrote:
> Sven Eckelmann wrote:
> > That must be
> > #define seq_before(x,y) ((x - y) > 1 << 7 + 8 * (sizeof(x) - 1))
> > #define seq_after(x,y) ((y - x) > 1 << 7 + 8 * (sizeof(x) - 1))
> 
> Nearly correct but accidentally  deleted a =
> 
> #define seq_before(x,y) ((x - y) > 1 << 7 + 8 * (sizeof(x) - 1))
> #define seq_after(x,y) ((y - x) >= 1 << 7 + 8 * (sizeof(x) - 1))
> 
> Best regards,
> 	Sven
  

On Fri, Mar 12, 2010 at 01:09:31AM +0100, Linus L??ssing wrote:
> Sorry still does not seem to work :). Despite testing it on a
> setup in batman itself I also tested it with a small c program
> (which does not work as expected either):
> -----
> #include <stdio.h>
> #include <stdint.h>
> 
> #define seq_before(x,y) ((x - y) > 1 << 7 + 8 * (sizeof(x) - 1))
> #define seq_after(x,y) ((y - x) >= 1 << 7 + 8 * (sizeof(x) - 1))
> 
> int main() {
>         uint8_t old = 0;
>         uint8_t new = 1;
>         if (!seq_after(new,old))
>                 printf("foobar\n");
> 
>         return 0;
> }

It is worth taking a look at include/linux/jiffies.h. The macros
time_after(), time_after_eq() etc.

	      Andrew
  

Linus Lüssing wrote:
> Sorry still does not seem to work :). Despite testing it on a
> setup in batman itself I also tested it with a small c program
> (which does not work as expected either):

But your version works? I would really doubt that when we do the same. The
macro will only replace the thing and we would have the same formular in that
situation.

> -----
> #include <stdio.h>
> #include <stdint.h>
> 
> #define seq_before(x,y) ((x - y) > 1 << 7 + 8 * (sizeof(x) - 1))
> #define seq_after(x,y) ((y - x) >= 1 << 7 + 8 * (sizeof(x) - 1))
> 
> int main() {
>         uint8_t old = 0;
>         uint8_t new = 1;
>         if (!seq_after(new,old))
>                 printf("foobar\n");
> 
>         return 0;
> }
> -----
> $ ./test
> foobar
> $

Now it gets real cruel, but who cares... C++ wasn't accept in the kernel, so we have
to deal with it using some GNUish stuff.

#define seq_before(x,y) ({typeof(x) _dummy = (x - y); \
                          _dummy > 1u << (7u + 8u * (sizeof(_dummy) - 1u));})
#define seq_after(x,y) ({typeof(x) _dummy = (y - x); \
                         _dummy >= 1u << (7u + 8u * (sizeof(_dummy) - 1u));})

Thanks for testing (I never did it :) ) and please test that one.

Best regards,
	Sven
  

Sven Eckelmann wrote:
> Now it gets real cruel, but who cares... C++ wasn't accept in the kernel,
>  so we have to deal with it using some GNUish stuff.
> 
> #define seq_before(x,y) ({typeof(x) _dummy = (x - y); \
>                           _dummy > 1u << (7u + 8u * (sizeof(_dummy) -
>  1u));}) #define seq_after(x,y) ({typeof(x) _dummy = (y - x); \
>                          _dummy >= 1u << (7u + 8u * (sizeof(_dummy) -
>  1u));})

After a small discussion with Marek I would like to suggest a easier readable 
and symmetric definition

/* Returns the smallest signed integer in two's complement with the sizeof x 
*/
#define smallest_signed_int(x) (1u << (7u + 8u * (sizeof(x) - 1u)))

/* Checks if a sequence number x is a predecessor/successor of y.
   they handle overflows/underflows and can correctly check for a
   predecessor/successor unless the variable sequence number has grown by
   more then 2**(bitwidth(x)-1)-1.
   This means that for an uint8_t with the maximum value 255, it would think:
    * when adding nothing - it is neither a predecessor nor a successor
    * before adding more than 127 to the starting value - it is a predecessor,
    * when adding 128 - it is neither a predecessor nor a successor,
    * after adding more than 127 to the starting value - it is a successor */
#define seq_before(x,y) ({typeof(x) _dummy = (x - y); \
                         _dummy > smallest_signed_int(_dummy); })
#define seq_after(x,y) seq_before(y,x)

Best regards,
	Sven
  

Sven Eckelmann wrote:
> Sven Eckelmann wrote:
> > Now it gets real cruel, but who cares... C++ wasn't accept in the kernel,
> >  so we have to deal with it using some GNUish stuff.
> >
> > #define seq_before(x,y) ({typeof(x) _dummy = (x - y); \
> >                           _dummy > 1u << (7u + 8u * (sizeof(_dummy) -
> >  1u));}) #define seq_after(x,y) ({typeof(x) _dummy = (y - x); \
> >                          _dummy >= 1u << (7u + 8u * (sizeof(_dummy) -
> >  1u));})
> 
> After a small discussion with Marek I would like to suggest a easier
>  readable and symmetric definition
> 
> /* Returns the smallest signed integer in two's complement with the sizeof
>  x */
> #define smallest_signed_int(x) (1u << (7u + 8u * (sizeof(x) - 1u)))
> 
> /* Checks if a sequence number x is a predecessor/successor of y.
>    they handle overflows/underflows and can correctly check for a
>    predecessor/successor unless the variable sequence number has grown by
>    more then 2**(bitwidth(x)-1)-1.
>    This means that for an uint8_t with the maximum value 255, it would
>  think: * when adding nothing - it is neither a predecessor nor a successor
>  * before adding more than 127 to the starting value - it is a predecessor,
>  * when adding 128 - it is neither a predecessor nor a successor, * after
>  adding more than 127 to the starting value - it is a successor */ #define
>  seq_before(x,y) ({typeof(x) _dummy = (x - y); \
>                          _dummy > smallest_signed_int(_dummy); })
> #define seq_after(x,y) seq_before(y,x)

@Linus: Here a small testcase - just as prove that I tested it this time :D

Best regards,
	Sven
  

Hey,

nice catch!

I'd like to join the party and propose this version:

#define seq_before(x,y) ((int8_t) (x - y) < 0)
#define seq_after(x,y) seq_before(y,x)

Not so much bitshifting and may a little bit easier to 
understand, but also not general and does not pass Svens regression test ... :(

Wrong seq_before(0, 128) == true
Wrong seq_after(0, 128) == true
Wrong seq_before(1, 129) == true
Wrong seq_after(1, 129) == true
Wrong seq_before(2, 130) == true
Wrong seq_after(2, 130) == true
Wrong seq_before(3, 131) == true
Wrong seq_after(3, 131) == true
Wrong seq_before(4, 132) == true
Wrong seq_after(4, 132) == true
Wrong seq_before(5, 133) == true
Wrong seq_after(5, 133) == true

What do you think?  ;)

best regards
	Simon

On Thu, Mar 11, 2010 at 11:06:42PM +0100, Sven Eckelmann wrote:
> Linus Lüssing wrote:
> > -               if (vis_packet->seqno - old_info->packet.seqno <= 0) {
> > +               if (vis_packet->seqno - old_info->packet.seqno
> > +                   <= 1 << 7 * sizeof(vis_packet->seqno)) {
> 
> Shouldn't that be 
>  1 << 7 + 8 * (sizeof(vis_packet->seqno) - 1))?
> 
> Otherwise you would have left the the sizeof(vis_packet->seqno) most 
> significant bits 0 and the rest one. And maybe a seq_before/seq_after static 
> inline function could be created to make this thing a lot more readable. You 
> could also do that in a macro if you don't want to assume the type of 
> vis_packet->seqno... which is probably what you tried by using sizeof
> 
> #define seq_before(x,y) ((x - y) >= 1 << 7 + 8 * (sizeof(x) - 1))
> #define seq_after(x,y) ((y - x) >= 1 << 7 + 8 * (sizeof(x) - 1))
> ....
> 
> And maybe you see here that the equal part which is needed bellow that line 
> isn't possible. So it must be changed to 
> 
> if (!seq_after(vis_packet->seqno, old_info->packet.seqno))
> 
> I hope that this is right - please check twice.
> 
> 
> Best regards,
> 	Sven
  

On Fri, Mar 12, 2010 at 09:57:40PM +0100, Simon Wunderlich wrote:
> #define seq_before(x,y) ((int8_t) (x - y) < 0)
> #define seq_after(x,y) seq_before(y,x)
> 
> Not so much bitshifting and may a little bit easier to 
> understand, but also not general and does not pass Svens regression test ... :(
> 
> Wrong seq_before(0, 128) == true
> Wrong seq_after(0, 128) == true
> Wrong seq_before(1, 129) == true
> Wrong seq_after(1, 129) == true
> Wrong seq_before(2, 130) == true
> Wrong seq_after(2, 130) == true
> Wrong seq_before(3, 131) == true
> Wrong seq_after(3, 131) == true
> Wrong seq_before(4, 132) == true
> Wrong seq_after(4, 132) == true
> Wrong seq_before(5, 133) == true
> Wrong seq_after(5, 133) == true
> 
> What do you think?  ;)

This is the version which also was mentioned by andrew - which has the
advantage that it doesn't have "complicated" code, but isn't general purpose
for unsigned ints. The only reason I started with the more complicated stuff
was that Linus tried to make it easier to change the packet definition
without ruining the code at different places.

That it doesn't pass the test is only the due to my symmetric definition of the
operation. I am not really intrested in changing that code, but to support
Linus.

Best regards,
	Sven