Multi-protocol core
Alexander V. Chernikov
melifaro at yandex-team.ru
Mon Oct 3 11:52:06 CEST 2011
On 03.10.2011 12:43, Martin Mares wrote:
> Ahoj!
>
>> commit ee3256fe1cb3e429de22e7c50a0826aaa4dc7a73
>> Author: Ondrej Zajicek<santiago at crfreenet.org>
>> Date: Sat Oct 1 10:28:34 2011 +0200
>>
>> Multiprotocol core support.
>>
>> The patch from Alexander V. Chernikov.
>
> Overall, the multiprotocol core looks good, there is a couple of minor
> nit-picking comments:
>
> | diff --git a/configure.in b/configure.in
> | index 46a6ecd..9af70b7 100644
> | --- a/configure.in
> | +++ b/configure.in
> | @@ -10,6 +10,7 @@ AC_ARG_ENABLE(debug,[ --enable-debug enable internal debugging routine
> | AC_ARG_ENABLE(memcheck,[ --enable-memcheck check memory allocations when debugging (default: enabled)],,enable_memcheck=yes)
> | AC_ARG_ENABLE(client,[ --enable-client enable building of BIRD client (default: enabled)],,enable_client=yes)
> | AC_ARG_ENABLE(ipv6,[ --enable-ipv6 enable building of IPv6 version (default: disabled)],,enable_ipv6=no)
> | +AC_ARG_ENABLE(mpls,[ --enable-mpls enable building MPLS version (default: disabled)],,enable_mpls=no)
> | AC_ARG_WITH(sysconfig,[ --with-sysconfig=FILE use specified BIRD system configuration file])
> | AC_ARG_WITH(protocols,[ --with-protocols=LIST include specified routing protocols (default: all)],,[with_protocols="all"])
> | AC_ARG_WITH(sysinclude,[ --with-sysinclude=PATH search for system includes on specified place])
> | @@ -50,6 +51,12 @@ else
> | all_protocols=bgp,ospf,pipe,rip,static
> | fi
> |
> | +if test "$enable_mpls" = yes ; then
> | + AC_DEFINE(MPLS)
> | + AC_DEFINE(MPLS_VPN)
> | +# all_protocols="$all_protocols",ldp
> | +fi
> | +
>
> Why is it called MPLS, when we call all other configuration switches CONFIG_something?
I thought CONFIG_XXX relates to protocol support and general features is
defined like IPV6, without any prefix.
>
> | diff --git a/lib/addrs.c b/lib/addrs.c
> | new file mode 100644
> | index 0000000..6553bba
> | --- /dev/null
> | +++ b/lib/addrs.c
> | @@ -0,0 +1,293 @@
> | +/*
> | + * BIRD Library -- Address Manipulation Functions
> | + *
> | + * (c) 2011 Alexander V. Chernikov<melifaro at ipfw.ru>
> | + *
> | + * Can be freely distributed and used under the terms of the GNU GPL.
> | + */
> | +
> | +#include<stdio.h>
> | +#include<stdlib.h>
> | +
> | +#include<sys/types.h>
> | +#include<sys/socket.h>
> | +#include<netinet/in.h>
> | +#include<arpa/inet.h>
>
> <arpa/inet.h> definitely belongs to sysdep.
>
> | +#define PBUFS 4
> | +#define PSIZE 50
>
> This deserves a comment.
>
> | +/**
> | + * fn_print - prints a FIB node
> | + * @n: pointer to fib_node structure
> | + *
> | + * This function prints fib node address to static buffer and
> | + * returns it to the caller. Up to PBUFS(4) different buffers are
> | + * available.
> | + */
>
> Recycling of a fixed number of static buffers is a rather dirty trick,
> which will likely lead to hard-to-catch bugs. (And break multithreading,
> if we ever use it in BIRD.)
This was used in pre-bprintf version and should be changed, yes
>
> Better let the caller supply an appropriate buffer. The number of direct
> callers is going to be low anyway, since almost everybody will use bprintf().
>
> | --- /dev/null
> | +++ b/lib/addrs.h
> [...]
> | +typedef uint64_t u64;
>
> Huh? It should be already defined in sysdep/config.h.
Yup. This should be removed (used in pre-extcommunity version)
>
> | +#ifndef IPV6
> | +typedef vpn4_addr vpn_addr;
> | +#else
> | +typedef vpn6_addr vpn_addr;
> | +#endif
>
> Calling this beast "vpn" without further clarification is going to be
> confusing -- people (even those experienced in networking) usually do not
> associate "vpn" with "MPLS" implicitly. Better choose a more descriptive name.
Well, VPN prefix is heavily used in rfc 4364, for example. I'm a bit out
of ideas. "mplsvpnv4" is more specific, of course, but a bit long.
>
> | diff --git a/nest/route.h b/nest/route.h
> | index 641b924..c3d5e2b 100644
> | --- a/nest/route.h
> | +++ b/nest/route.h
> | @@ -36,9 +36,10 @@ struct fib_node {
> | struct fib_iterator *readers; /* List of readers of this node */
> | byte pxlen;
> | byte flags; /* User-defined */
> | - byte x0, x1; /* User-defined */
> | + byte addr_type; /* User-defined */
> | + byte addr_off; /* address data offset */
>
> Please use consistent capitalization (everywhere).
>
> | + unsigned int addr_type; /* Type of adresses stored in fib (IPv46, VPNv46, MPLS, etc..)*/
> | + unsigned int addr_size; /* size of address specified in entry */
> | + unsigned int addr_off; /* value of data offset to be set in fib_node */
>
> I did not understand anything from the latter two comments.
>
> What is specified in an entry? An address? Or the size itself?
It specifies size of address data used in particular fib (e.g. addr_size
= sizeof(ip_addr)).
s/size of address specified in entry/Size of address data/ ?
>
> Offset of which data?
Address data from the previous comment.
s/value of data offset to be set in fib_node/Address data offset in
fib_node/ ?
>
> | @@ -116,6 +129,7 @@ void fit_put(struct fib_iterator *, struct fib_node *);
> | struct rtable_config {
> | node n;
> | char *name;
> | + int rtype; /* Type of the table (IPv46, VPNv46, MPLS, etc..)*/
>
> Why is it called "rtype" here and "addr_type" elsewhere?
rtype is short for 'routing table type' but this should really be
changed for consistency.
>
> | +/* Types of routing tables/entries */
> | +#define RT_IPV4 1
> | +#define RT_IPV6 2
> | +#define RT_VPNV4 3
> | +#define RT_VPNV6 4
> | +#define RT_MPLS 5
> | +
> | +#define RT_MAX 6
>
> Deserves comments.
>
> | +/* Same tables using bit positions. Used for appropriate table linking on protocol init */
> | +#define RT_IPV4_BIT 0x001
> | +#define RT_IPV6_BIT 0x002
> | +#define RT_VPNV4_BIT 0x004
> | +#define RT_VPNV6_BIT 0x008
> | +#define RT_MPLS_BIT 0x010
>
> Better use
>
> #define RT_IPV4_BIT (1U<< RT_IPV4)
>
> | diff --git a/nest/rt-fib.c b/nest/rt-fib.c
> | index 510aa76..bf2b562 100644
> | --- a/nest/rt-fib.c
> | +++ b/nest/rt-fib.c
> | @@ -73,9 +73,22 @@ fib_ht_free(struct fib_node **h)
> | }
> |
> | static inline unsigned
> | -fib_hash(struct fib *f, ip_addr *a)
> | +fib_hash(struct fib *f, void *a)
> | {
> | - return ipa_hash(*a)>> f->hash_shift;
> | + u32 *v, x;
> | + int i;
> | +
> | + if (f->hash_f)
> | + return f->hash_f(a);
> | +
> | + if (f->addr_type == RT_IP)
> | + return ipa_hash(*((ip_addr *)a))>> f->hash_shift;
> | +
> | + x = 0;
> | + for (i = 0, v = (u32 *)a; i< f->addr_size / 4; i++, v++)
> | + x = x ^ *v;
> | +
> | + return ((x ^ (x>> 16) ^ (x>> 8))& 0xffff)>> f->hash_shift;
> | }
>
> Is it guaranteed that addr_size will be always divisible by 4? If it is
> a requirement, document it.
>
> If I understand the code properly, fib_hash() is a pretty hot place, but you
> do several pointless tests there -- e.g., f->addr_type is constant for the whole FIB,
> but you test it during each calculation of a hash value.
>
> Instead of that, set f->hash_f during initialization of the FIB according to
> the type of addresses used.
Thanks, as a result:
If we do hash_f setting in FIB init and assume non-IP users to set their
own hashing function fib_hash can be changed to simple:
#define fib_hash(f, a) (f)->hash_f(a)
>
> Martin
>
--
Alexander V. Chernikov
Yandex NOC
More information about the Bird-users
mailing list