On 28/06/16(Tue) 20:03, Jonathan Matthew wrote:
> Currently, art explicitly relies on the kernel lock to serialise updates. We
> need to allow route insertion during packet processing, which means avoiding
> the kernel lock is desirable. The diff below achieves that, adding a mutex
> per art root. While that doesn't seem particularly fine grained, this is only
> used to serialise updates and table walks, and lookups aren't affected at all.
>
> Most of the locking actually happens in rtable.c as the mutex covers the list
> of routes attached to each node in the tree, as well as the structure of the
> tree itself.
Looks good, two comments below.
> @@ -905,7 +915,7 @@ rtable_mpath_reprio(unsigned int rtablei
> struct srp_ref sr;
> uint8_t *addr;
> int plen;
> - struct rtentry *mrt, *prt = NULL;
> + int error = 0;
>
> ar = rtable_get(rtableid, dst->sa_family);
> if (ar == NULL)
> @@ -914,8 +924,7 @@ rtable_mpath_reprio(unsigned int rtablei
> addr = satoaddr(ar, dst);
> plen = rtable_satoplen(dst->sa_family, mask);
>
> - KERNEL_ASSERT_LOCKED();
> -
> + mtx_enter(&ar->ar_lock);
> an = art_lookup(ar, addr, plen, &sr);
> srp_leave(&sr); /* an can't go away while we have the lock */
>
> @@ -923,10 +932,24 @@ rtable_mpath_reprio(unsigned int rtablei
> if (an == NULL || an->an_plen != plen ||
> memcmp(an->an_dst, dst, dst->sa_len))
> return (ESRCH);
Here you want "error = ESRCH", no?
> + else {
> + rtref(rt); /* keep rt alive in between remove and insert */
> + SRPL_REMOVE_LOCKED(&rt_rc, &an->an_rtlist,
> + rt, rtentry, rt_next);
> + rt->rt_priority = prio;
> + art_mpath_insert(an, rt);
> + rtfree(rt);
> + }
> + mtx_leave(&ar->ar_lock);
>
> - rtref(rt); /* keep rt alive in between remove and add */
> - SRPL_REMOVE_LOCKED(&rt_rc, &an->an_rtlist, rt, rtentry, rt_next);
> - rt->rt_priority = prio;
> + return (error);
> +}
> +
> +void
> +art_mpath_insert(struct art_node *an, struct rtentry *rt)
Could you prefix the function name by "rtable_" ?
> +{
> + struct rtentry *mrt, *prt = NULL;
> + uint8_t prio = rt->rt_priority;
>
> if ((mrt = SRPL_FIRST_LOCKED(&an->an_rtlist)) != NULL) {
> /*
