On 06/06/16(Mon) 17:14, Jonathan Matthew wrote:
> We've finally got srp and art to the point where we can use srp to manage the
> internal links in the art data structures. This allows us to do route lookups
> without holding any locks, which is kind of nice.
It's awesome!
> As we're not doing unlocked insert, delete or walk (yet), those art functions
> use the locked variants of the srp functions. Currently the lock that
> protects those operations is the kernel lock.
>
> Callers to art_lookup and art_match now pass in a pointer to an srp_ref, which
> is always active on return, even if no route is found. In some situations we
> use these functions while modifying the routing table, in which case the
> kernel lock already protects against concurrent modifications so the srp_ref
> is unnecessary, so we srp_leave it immediately.
>
> I'd also like to draw attention to the comment in rtable_match. Is it OK that
> we might choose a multipath route at random if the set of available routes
> changes while we're choosing?
I believe it's ok. If for a short period of time the multi-path route
becomes single-path then the answer is obvious.
Now every time there's a change in a list of multipath route, it might
happen that for a given source, paths selected before and after the
change are different. Simply because the order of the list mater and
there's no way to distinguish two routes with the same gateway and same
priority.
I have two nitpicks, but your diff is ok mpi@
> Index: art.h
> ===================================================================
> RCS file: /cvs/src/sys/net/art.h,v
> retrieving revision 1.13
> diff -u -p -u -p -r1.13 art.h
> --- art.h 3 Jun 2016 03:59:43 -0000 1.13
> +++ art.h 6 Jun 2016 00:32:31 -0000
> @@ -19,13 +19,15 @@
> #ifndef _NET_ART_H_
> #define _NET_ART_H_
>
> +#include <sys/srp.h>
Could you keep the include in the .c file instead?
> #define ART_MAXLVL 32 /* We currently use 32 levels for IPv6. */
>
> /*
> * Root of the ART tables, equivalent to the radix head.
> */
> struct art_root {
> - struct art_table *ar_root; /* First table */
> + struct srp ar_root; /* First table */
> uint8_t ar_bits[ART_MAXLVL]; /* Per level stride */
> uint8_t ar_nlvl; /* Number of levels */
> uint8_t ar_alen; /* Address length in bits */
> while (1) {
> - /*
> - * Rember the default route of this table in case
> - * we do not find a better matching route.
> - */
> - if (at->at_default != NULL)
> - dflt = at->at_default;
> -
> /* Do a single level route lookup. */
> j = art_findex(at, addr);
> + entry = srp_follow(nsr, &at->at_heap[j].node);
>
> - /* If this is a leaf we're done. */
> - if (ISLEAF(at->at_heap[j]))
> + /* If this is a leaf (NULL is a leaf) we're done. */
> + if (ISLEAF(entry))
> break;
I like the extended comments "(NULL is a leaf)", could you also put it
in art_lookup()?
