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