[quagga-dev 7265] Re: [PATCH 1/3] zebra: RIB improvements (revised)

paul at jakma.org paul at jakma.org
Thu Sep 3 20:07:38 BST 2009


On Wed, 5 Aug 2009, Stephen Hemminger wrote:

> Subject: zebra: RIB improvements
>
> This patch to current quagga git adds:
>  * recursive static that work. For example: given two routes:
>      route 10.1.1.0/24 next-hop 10.2.2.1
>      connected route 10.2.2.0/24 on eth1
>    and eth1 goes down, then recursive route should become inactive
>  * merged code for ipv4/ipv6 active check

I've been going through this patch. The recursive active check is 
quite neat - though I suspect it can still have some niggles (e.g. 
what if a recursive nexthop-active check updates the CHANGED flag? 
maybe we shouldn't clobber that flag in the nexthop-check path?). I 
just worry a bit it'll miss dependencies..

I've ported this to the rib_reform branch, though with some edits. 
I've put the cleanups in a separate, earlier commit (if at all 
possible, please try split cleanups out from functional changes, for 
easier review).

See:

http://code.quagga.net/cgi-bin/gitweb.cgi?p=people/paul/quagga;a=shortlog;h=refs/heads/volatile/rib_reform

I'm kind of nervous of doing big changes to this pre-next-stable, and 
I wonder if we should just target next-unstable for the RIB 
overhauling?

What do you think?

Few further comments in line.

> ---
> Change from last version:
>   node_parent from table.h to zebra_rib.c since only used here

Meh, leave it in lib/table ;)

>
> --- a/zebra/zebra_rib.c	2009-08-05 12:38:41.021412981 -0700
> +++ b/zebra/zebra_rib.c	2009-08-05 12:39:41.725397377 -0700
> @@ -328,142 +328,144 @@ nexthop_blackhole_add (struct rib *rib)
>   return nexthop;
> }
>

> +static int
> +nexthop_same (const struct nexthop *h1,
> +		const struct nexthop *h2)
> +{
> +  if (h1->type != h2->type)
>     return 0;

I've put nexthop_same in lib/nexthop.c - there was already a function 
there that I'd moved from bgpd as part of the rib_reform branch.

> -  while (rn)
> +  switch (h1->type)
>     {

> static int
> -nexthop_active_ipv6 (struct rib *rib, struct nexthop *nexthop, int set,
> -		     struct route_node *top)
> +nexthop_active (struct rib *rib, struct nexthop *nexthop, int set,
> +		struct route_node *top)
> {

>   /* Lookup table.  */
> -  table = vrf_table (AFI_IP6, SAFI_UNICAST, 0);
> +  table = vrf_table (AFI_IP, SAFI_UNICAST, 0);

oops. ;) meant to get the family from the route table or prefix?

>   if (! table)
>     return 0;
>
> -  rn = route_node_match (table, (struct prefix *) &p);
> -  while (rn)
> +  for (rn = route_node_match (table, &p); rn; rn = route_node_parent (rn))

That's in the cleanup commit, thanks.

> +  /* Not active: no usable route found */
> +  if (!match)
> +    return 0;
> +
> +  if (IS_ZEBRA_DEBUG_RIB)
> +    {
> +      char buf[INET6_ADDRSTRLEN];
> +      zlog_debug ("%s: %s match type %d rib flags %#x",
> +		  __func__, inet_ntop (p.family, &p.u.prefix, buf, sizeof buf),
> +		  match->type, rib->flags);
> +      if (match->nexthop)
> +	zlog_debug ("  nexthop %s flags %#x",
> +		    nexthop_isactive(match->nexthop) ? "active" : "inactive",
> +		    match->nexthop->flags);
> +    }
> +
> +  /* If this is a connected route then see if it is alive */
> +  if (match->type == ZEBRA_ROUTE_CONNECT)
> +    {
> +      /* Directly point connected route. */
> +      newhop = match->nexthop;
> +      if (!newhop || !nexthop_isactive (newhop))
> +	return 0;	/* Not active: dead route */

I've changed the logic here slightly. E.g. instead of 
nexthop_isactive, the recursion is back into nexthop_active_update, 
to do a full check on match.

If I understand right: If the metaq can guarantee that RIB nexthops 
are validated in order of any dependencies (from 0-dependency 
connected routes on up), and if we do all the nexthop-checks from a 
scan timer thread, i think we can add code to avoid repeated checks 
of any one nexthop.

Also, that check can be unconditional, prior to looking to see if the 
match is connected or whether we can try for recursion, methinks.

> +	  if (nexthop->ifindex != newhop->ifindex ||
> +	      CHECK_FLAG (nexthop->flags, NEXTHOP_FLAG_RECURSIVE))
> +	    SET_FLAG (rib->flags, ZEBRA_FLAG_CHANGED);

> +
> +	/* Did destination for recursive route change? */
> +	if (!nexthop_same (nexthop, newhop))
> +	  SET_FLAG (rib->flags, ZEBRA_FLAG_CHANGED);
> +	return 1;

Also, I've shoved the responsibility for checking whether a nexthop 
has changed back up to nexthop_active_update.

> @@ -1568,6 +1618,11 @@ rib_add_ipv4 (int type, int flags, struc
> 	distance = 200;
>     }
>
> +  /* All system, kernel, connected or static routes are possible
> +     candidates for recursive routes */
> +  if (type < ZEBRA_ROUTE_RIP)
> +    SET_FLAG (flags, ZEBRA_FLAG_INTERNAL);

I'm not sure about this part. Do we really want recursion to apply to 
connected routes? For the sake of being conservative, I've instead 
made 'static_init' set this flag, so it just applies to statics.


regards,
-- 
Paul Jakma	paul at jakma.org	Key ID: 64A2FF6A
Fortune:
The optimum committee has no members.
 		-- Norman Augustine



More information about the Quagga-dev mailing list