[PATCH v2 2/2] Add the Babel protocol.

[Toke Høiland-Jørgensen]

Juliusz Chroboczek <jch at pps.univ-paris-diderot.fr> writes:

> Please don't make this configurable, it only breaks interoperability.
> Section 4 of RFC 6126 is very clear about the maximum size of a Babel
> packet:

Noted. Snatched that from the RIP implementation; thought it might be
nice to be configurable in the same way. Guess I was wrong... ;)

>> +      <tag>hello interval <m/num/</tag>
>> +      Interval at which periodic "hello" messages are sent on this interface,
>> +      in seconds. Default: 20 seconds for wired interfaces, 4 seconds for wireless. 
>
> Note that recent versions of babeld use 4s in all cases by default.  20s
> turned out to be too long for our users.

Noted. Will change it to 4.

>> +      These options specify the ToS/DiffServ/Traffic class/Priority of the
>> +      outgoing RIP packets.
>
> RIP?

Only one copy/paste error from all the stuff I lifted from the RIP
implementation? Not too shabby I'd say... ;)

>> +/* Is one number larger than another mod 65535? Since diff_mod64k is always >=
>> +   0, just use a simple cutoff value to determine if the difference is small
>> +   enough that one is really larger. Since these comparisons are only made for
>> +   values that should not differ by more than a few numbers, this should be
>> +   safe. */
>> +static inline u16 ge_mod64k(u16 a, u16 b)
>> +{
>> +  return ((u16) a-b) < 0xfff0;
>> +}
>
> The comment is obsolete -- it applies to the previous version.

Well the first sentence, yeah; fixed.


>> +static struct babel_source *
>> +babel_find_source(struct babel_entry *e, u64 router_id)
>> +{
>> +  struct babel_source *s;
>> +  WALK_LIST(s, e->sources)
>> +    if (s->router_id == router_id)
>> +      return s;
>> +  return NULL;
>> +}
>
> We've recently found out that in large networks, a large amount of CPU
> time (40%) in babeld was taken by walking the source table -- I recommend
> using a more efficient data structure here.  (Source creation/flushing is
> rare, but searching for a source is very common.)

Yeah, saw that thread. Punted on doing something about it here; guess
I'll see if I can figure out how the Bird hash table works... Bah! Who
needs such big networks anyway?

>> +static void
>> +babel_iface_timer(timer *t)
>> +{
>
> The triggered update logic here is not obvious to me.  Perhaps a comment
> is suitable?

The idea is that when an update is (first) triggered, the current
timestamp (in 'now') is saved, and when the update is sent, only routes
which are updated at times >= that timestamp are actually transmitted.
This works because the code triggering the update will simultaneously
set the 'updated' time of the route(s) being modified.

Now, going over this I discovered a bug where single-interface triggered
updates could override the triggered time of each other. Fixed this, and
will add a comment.


>> +static void
>> +babel_iface_update_buffers(struct babel_iface *ifa)
>> +{
>> +  if (!ifa->sk)
>> +    return;
>> +
>> +  uint rbsize = ifa->cf->rx_buffer ?: ifa->iface->mtu;
>> +  uint tbsize = ifa->cf->tx_length ?: ifa->iface->mtu;
>
> Shouldn't you be accounting for IPv6 and UDP headers, at least for
> tbsize?

It already does; the max packet length is set on the line below:

>> +static enum parse_result
>> +babel_read_next_hop(struct babel_pkt_tlv_header *hdr,
>> +                    union babel_tlv *tlv,
>> +                    struct babel_parse_state *state)
>
> AE 2 is perfectly legal in NH TLVs.  (Babeld only ever sends AE 3, but
> non-LL NHs are perfectly legal.)  See Section 4.4.8 of RFC 6126.

Noted, and fixed.

>> +void
>> +babel_send_unicast(union babel_tlv *tlv, struct babel_iface *ifa, ip_addr dest)
>
> Note that buffering unicasts can be useful in case you're forwarding
> multiple requests to the same neighbour.  (I babeld, we buffer at most one
> unicast packet per interface -- per-neighbour queueing is probably
> overkill.)

Noted.

>> +  /* A router_id may be 0, so we need a separate variable to track whether we
>> +     have seen a router_id */
>
> Interesting observation.  This suggests making 0 router-id forbidden (and
> all-1 too, for good measure).

Yup, would make sense :)


Thanks for your comments; fixed most of them in my tree already, and
will see what I can do about the rest.

Cheers,

-Toke