On Thu, Oct 22, 2015 at 07:42:24PM +0200, Martin Pieuchot wrote:
> Now that we have a single refcounting mechanism for route entries, I'd
> like to use atomic operations and grab the KERNEL_LOCK only if a CPU is
> dropping the last reference on an entry.
>
> Currently this only matters for MPLS. I intentionally use atomic_* ops
> because I'd like to see be able to see if a counter goes negative.
>
> For symmetry reasons I'm also moving the KERNEL_LOCK() inside rtalloc().
> These two functions are my current targets.
>
> Comments, oks?
One comment inline...
>
> Index: sys/net/route.c
> ===================================================================
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.258
> diff -u -p -r1.258 route.c
> --- sys/net/route.c 22 Oct 2015 17:19:38 -0000 1.258
> +++ sys/net/route.c 22 Oct 2015 17:21:52 -0000
> @@ -215,6 +215,7 @@ rtalloc(struct sockaddr *dst, int flags,
> info.rti_info[RTAX_DST] = dst;
>
> s = splsoftnet();
> + KERNEL_LOCK();
> rt = rtable_match(tableid, dst);
> if (rt != NULL) {
> if ((rt->rt_flags & RTF_CLONING) && ISSET(flags, RT_RESOLVE)) {
> @@ -236,6 +237,7 @@ miss:
> if (ISSET(flags, RT_REPORT))
> rt_missmsg(RTM_MISS, &info, 0, NULL, error, tableid);
> }
> + KERNEL_UNLOCK();
> splx(s);
> return (rt);
> }
> @@ -337,7 +339,7 @@ rtalloc_mpath(struct sockaddr *dst, uint
> void
> rtref(struct rtentry *rt)
> {
> - rt->rt_refcnt++;
> + atomic_inc_int(&rt->rt_refcnt);
> }
>
> void
> @@ -348,14 +350,16 @@ rtfree(struct rtentry *rt)
> if (rt == NULL)
> return;
>
> - if (--rt->rt_refcnt <= 0) {
> + if (atomic_dec_int_nv(&rt->rt_refcnt) <= 0) {
> KASSERT(!ISSET(rt->rt_flags, RTF_UP));
> KASSERT(!RT_ROOT(rt));
> - rttrash--;
> + atomic_dec_int(&rttrash);
Are you using rttrash for debugging? It's unused anywhere else,
and if it's just incrementing and decrementing a counter only
used for debugging (or possibly not at all!), it might be
better to put it in DEBUG kernels, or just remove it entirely.
> if (rt->rt_refcnt < 0) {
> printf("rtfree: %p not freed (neg refs)\n", rt);
> return;
> }
> +
> + KERNEL_LOCK();
> rt_timer_remove_all(rt);
> ifa = rt->rt_ifa;
> if (ifa)
> @@ -368,6 +372,8 @@ rtfree(struct rtentry *rt)
> if (rt->rt_gateway)
> free(rt->rt_gateway, M_RTABLE, 0);
> free(rt_key(rt), M_RTABLE, 0);
> + KERNEL_UNLOCK();
> +
> pool_put(&rtentry_pool, rt);
> }
> }
> @@ -773,7 +779,7 @@ rtrequest1(int req, struct rt_addrinfo *
> rt->rt_flags &= ~RTF_UP;
> if ((ifa = rt->rt_ifa) && ifa->ifa_rtrequest)
> ifa->ifa_rtrequest(RTM_DELETE, rt);
> - rttrash++;
> + atomic_inc_int(&rttrash);
>
> if (ret_nrt != NULL)
> *ret_nrt = rt;
> Index: sys/netmpls/mpls_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netmpls/mpls_input.c,v
> retrieving revision 1.50
> diff -u -p -r1.50 mpls_input.c
> --- sys/netmpls/mpls_input.c 23 Sep 2015 08:49:46 -0000 1.50
> +++ sys/netmpls/mpls_input.c 22 Oct 2015 17:21:52 -0000
> @@ -170,9 +170,7 @@ do_v6:
> }
> }
>
> - KERNEL_LOCK();
> rt = rtalloc(smplstosa(smpls), RT_REPORT|RT_RESOLVE, 0);
> - KERNEL_UNLOCK();
> if (rt == NULL) {
> /* no entry for this label */
> #ifdef MPLS_DEBUG
> @@ -290,9 +288,7 @@ do_v6:
> if (ifp != NULL && rt_mpls->mpls_operation != MPLS_OP_LOCAL)
> break;
>
> - KERNEL_LOCK();
> rtfree(rt);
> - KERNEL_UNLOCK();
> rt = NULL;
> }
>
> @@ -323,11 +319,7 @@ do_v6:
> (*ifp->if_ll_output)(ifp, m, smplstosa(smpls), rt);
> KERNEL_UNLOCK();
> done:
> - if (rt) {
> - KERNEL_LOCK();
> - rtfree(rt);
> - KERNEL_UNLOCK();
> - }
> + rtfree(rt);
> }
>
> int
> @@ -394,7 +386,7 @@ mpls_do_error(struct mbuf *m, int type,
> struct in_ifaddr *ia;
> struct icmp *icp;
> struct ip *ip;
> - int nstk;
> + int nstk, error;
>
> for (nstk = 0; nstk < MPLS_INKERNEL_LOOP_MAX; nstk++) {
> if (m->m_len < sizeof(*shim) &&
> @@ -427,9 +419,7 @@ mpls_do_error(struct mbuf *m, int type,
> smpls->smpls_len = sizeof(*smpls);
> smpls->smpls_label = shim->shim_label & MPLS_LABEL_MASK;
>
> - KERNEL_LOCK();
> rt = rtalloc(smplstosa(smpls), RT_REPORT|RT_RESOLVE, 0);
> - KERNEL_UNLOCK();
> if (rt == NULL) {
> /* no entry for this label */
> m_freem(m);
> @@ -442,19 +432,16 @@ mpls_do_error(struct mbuf *m, int type,
> * less interface we need to find some other IP to
> * use as source.
> */
> - KERNEL_LOCK();
> rtfree(rt);
> - KERNEL_UNLOCK();
> m_freem(m);
> return (NULL);
> }
> - KERNEL_LOCK();
> rtfree(rt);
> - if (icmp_reflect(m, NULL, ia)) {
> - KERNEL_UNLOCK();
> - return (NULL);
> - }
> + KERNEL_LOCK();
> + error = icmp_reflect(m, NULL, ia);
> KERNEL_UNLOCK();
> + if (error)
> + return (NULL);
>
> ip = mtod(m, struct ip *);
> /* stuff to fix up which is normaly done in ip_output */
>
