[PATCH v2 2/2] Add the Babel protocol.
toke at toke.dk
Sun Jan 17 12:19:08 CET 2016
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
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.
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
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.
>> +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
>> + /* 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.
More information about the Bird-users