Date: Thu, 6 Nov 2008 22:11:57 +0000 (UTC) From: Julian Elischer <julian@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-7@freebsd.org Subject: svn commit: r184739 - in stable/7/sys: . modules/cxgb net netinet Message-ID: <200811062211.mA6MBvdU066984@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: julian Date: Thu Nov 6 22:11:57 2008 New Revision: 184739 URL: http://svn.freebsd.org/changeset/base/184739 Log: MFC a rewrite of rt_check(). also revert the addition of rt_check_fib() which we discovered is not needed. fixes some hangs people have seen Approved by: re (ken) Modified: stable/7/sys/ (props changed) stable/7/sys/modules/cxgb/ (props changed) stable/7/sys/net/if_atmsubr.c stable/7/sys/net/if_fwsubr.c stable/7/sys/net/if_iso88025subr.c stable/7/sys/net/route.c stable/7/sys/net/route.h stable/7/sys/netinet/if_ether.c stable/7/sys/netinet/in_rmx.c stable/7/sys/netinet/in_var.h Modified: stable/7/sys/net/if_atmsubr.c ============================================================================== --- stable/7/sys/net/if_atmsubr.c Thu Nov 6 21:47:02 2008 (r184738) +++ stable/7/sys/net/if_atmsubr.c Thu Nov 6 22:11:57 2008 (r184739) @@ -158,8 +158,7 @@ atm_output(struct ifnet *ifp, struct mbu * check route */ if (rt0 != NULL) { - error = rt_check_fib(&rt, &rt0, - dst, rt0->rt_fibnum); + error = rt_check(&rt, &rt0, dst); if (error) goto bad; RT_UNLOCK(rt); Modified: stable/7/sys/net/if_fwsubr.c ============================================================================== --- stable/7/sys/net/if_fwsubr.c Thu Nov 6 21:47:02 2008 (r184738) +++ stable/7/sys/net/if_fwsubr.c Thu Nov 6 22:11:57 2008 (r184739) @@ -103,7 +103,7 @@ firewire_output(struct ifnet *ifp, struc } if (rt0 != NULL) { - error = rt_check_fib(&rt, &rt0, dst, rt0->rt_fibnum); + error = rt_check(&rt, &rt0, dst); if (error) goto bad; RT_UNLOCK(rt); Modified: stable/7/sys/net/if_iso88025subr.c ============================================================================== --- stable/7/sys/net/if_iso88025subr.c Thu Nov 6 21:47:02 2008 (r184738) +++ stable/7/sys/net/if_iso88025subr.c Thu Nov 6 22:11:57 2008 (r184739) @@ -259,8 +259,7 @@ iso88025_output(ifp, m, dst, rt0) /* Calculate routing info length based on arp table entry */ /* XXX any better way to do this ? */ if (rt0 != NULL) { -/* XXX MRT *//* Guess only */ - error = rt_check_fib(&rt, &rt0, dst, rt0->rt_fibnum); + error = rt_check(&rt, &rt0, dst); if (error) goto bad; RT_UNLOCK(rt); Modified: stable/7/sys/net/route.c ============================================================================== --- stable/7/sys/net/route.c Thu Nov 6 21:47:02 2008 (r184738) +++ stable/7/sys/net/route.c Thu Nov 6 22:11:57 2008 (r184739) @@ -1549,84 +1549,120 @@ rtinit(struct ifaddr *ifa, int cmd, int * final destination if directly reachable); * *lrt0 points to the cached route to the final destination; * *lrt is not meaningful; - * fibnum is the index to the correct network fib for this packet + * (*lrt0 has no ref held on it by us so REMREF is not needed. + * Refs only account for major structural references and not usages, + * which is actually a bit of a problem.) * * === Operation === * If the route is marked down try to find a new route. If the route * to the gateway is gone, try to setup a new route. Otherwise, * if the route is marked for packets to be rejected, enforce that. + * Note that rtalloc returns an rtentry with an extra REF that we may + * need to lose. * * === On return === * *dst is unchanged; * *lrt0 points to the (possibly new) route to the final destination - * *lrt points to the route to the next hop + * *lrt points to the route to the next hop [LOCKED] * * Their values are meaningful ONLY if no error is returned. + * + * To follow this you have to remember that: + * RT_REMREF reduces the reference count by 1 but doesn't check it for 0 (!) + * RTFREE_LOCKED includes an RT_REMREF (or an rtfree if refs == 1) + * and an RT_UNLOCK + * RTFREE does an RT_LOCK and an RTFREE_LOCKED + * The gwroute pointer counts as a reference on the rtentry to which it points. + * so when we add it we use the ref that rtalloc gives us and when we lose it + * we need to remove the reference. + * RT_TEMP_UNLOCK does an RT_ADDREF before freeing the lock, and + * RT_RELOCK locks it (it can't have gone away due to the ref) and + * drops the ref, possibly freeing it and zeroing the pointer if + * the ref goes to 0 (unlocking in the process). */ int rt_check(struct rtentry **lrt, struct rtentry **lrt0, struct sockaddr *dst) { - return (rt_check_fib(lrt, lrt0, dst, 0)); -} - -int -rt_check_fib(struct rtentry **lrt, struct rtentry **lrt0, struct sockaddr *dst, - u_int fibnum) -{ struct rtentry *rt; struct rtentry *rt0; - int error; + u_int fibnum; KASSERT(*lrt0 != NULL, ("rt_check")); - rt = rt0 = *lrt0; + rt0 = *lrt0; + rt = NULL; + fibnum = rt0->rt_fibnum; /* NB: the locking here is tortuous... */ - RT_LOCK(rt); - if ((rt->rt_flags & RTF_UP) == 0) { - RT_UNLOCK(rt); - rt = rtalloc1_fib(dst, 1, 0UL, fibnum); - if (rt != NULL) { - RT_REMREF(rt); - /* XXX what about if change? */ - } else + RT_LOCK(rt0); +retry: + if (rt0 && (rt0->rt_flags & RTF_UP) == 0) { + /* Current rt0 is useless, try get a replacement. */ + RT_UNLOCK(rt0); + rt0 = NULL; + } + if (rt0 == NULL) { + rt0 = rtalloc1_fib(dst, 1, 0UL, fibnum); + if (rt0 == NULL) { return (EHOSTUNREACH); - rt0 = rt; + } + RT_REMREF(rt0); /* don't need the reference. */ } - /* XXX BSD/OS checks dst->sa_family != AF_NS */ - if (rt->rt_flags & RTF_GATEWAY) { - if (rt->rt_gwroute == NULL) - goto lookup; - rt = rt->rt_gwroute; - RT_LOCK(rt); /* NB: gwroute */ - if ((rt->rt_flags & RTF_UP) == 0) { - RTFREE_LOCKED(rt); /* unlock gwroute */ - rt = rt0; - rt0->rt_gwroute = NULL; - lookup: - RT_UNLOCK(rt0); -/* XXX MRT link level looked up in table 0 */ - rt = rtalloc1_fib(rt->rt_gateway, 1, 0UL, 0); - if (rt == rt0) { - RT_REMREF(rt0); - RT_UNLOCK(rt0); + + if (rt0->rt_flags & RTF_GATEWAY) { + if ((rt = rt0->rt_gwroute) != NULL) { + RT_LOCK(rt); /* NB: gwroute */ + if ((rt->rt_flags & RTF_UP) == 0) { + /* gw route is dud. ignore/lose it */ + RTFREE_LOCKED(rt); /* unref (&unlock) gwroute */ + rt = rt0->rt_gwroute = NULL; + } + } + + if (rt == NULL) { /* NOT AN ELSE CLAUSE */ + RT_TEMP_UNLOCK(rt0); /* MUST return to undo this */ + rt = rtalloc1_fib(rt0->rt_gateway, 1, 0UL, fibnum); + if ((rt == rt0) || (rt == NULL)) { + /* the best we can do is not good enough */ + if (rt) { + RT_REMREF(rt); /* assumes ref > 0 */ + RT_UNLOCK(rt); + } + RTFREE(rt0); /* lock, unref, (unlock) */ return (ENETUNREACH); } - RT_LOCK(rt0); - if (rt0->rt_gwroute != NULL) - RTFREE(rt0->rt_gwroute); - rt0->rt_gwroute = rt; - if (rt == NULL) { - RT_UNLOCK(rt0); - return (EHOSTUNREACH); + /* + * Relock it and lose the added reference. + * All sorts of things could have happenned while we + * had no lock on it, so check for them. + */ + RT_RELOCK(rt0); + if (rt0 == NULL || ((rt0->rt_flags & RTF_UP) == 0)) + /* Ru-roh.. what we had is no longer any good */ + goto retry; + /* + * While we were away, someone replaced the gateway. + * Since a reference count is involved we can't just + * overwrite it. + */ + if (rt0->rt_gwroute) { + if (rt0->rt_gwroute != rt) { + RTFREE_LOCKED(rt); + goto retry; + } + } else { + rt0->rt_gwroute = rt; } } + RT_LOCK_ASSERT(rt); RT_UNLOCK(rt0); + } else { + /* think of rt as having the lock from now on.. */ + rt = rt0; } /* XXX why are we inspecting rmx_expire? */ - error = (rt->rt_flags & RTF_REJECT) && - (rt->rt_rmx.rmx_expire == 0 || - time_uptime < rt->rt_rmx.rmx_expire); - if (error) { + if ((rt->rt_flags & RTF_REJECT) && + (rt->rt_rmx.rmx_expire == 0 || + time_uptime < rt->rt_rmx.rmx_expire)) { RT_UNLOCK(rt); return (rt == rt0 ? EHOSTDOWN : EHOSTUNREACH); } Modified: stable/7/sys/net/route.h ============================================================================== --- stable/7/sys/net/route.h Thu Nov 6 21:47:02 2008 (r184738) +++ stable/7/sys/net/route.h Thu Nov 6 22:11:57 2008 (r184739) @@ -312,19 +312,35 @@ struct rt_addrinfo { } while (0) #define RTFREE_LOCKED(_rt) do { \ - if ((_rt)->rt_refcnt <= 1) \ - rtfree(_rt); \ - else { \ - RT_REMREF(_rt); \ - RT_UNLOCK(_rt); \ - } \ - /* guard against invalid refs */ \ - _rt = 0; \ - } while (0) + if ((_rt)->rt_refcnt <= 1) \ + rtfree(_rt); \ + else { \ + RT_REMREF(_rt); \ + RT_UNLOCK(_rt); \ + } \ + /* guard against invalid refs */ \ + _rt = 0; \ +} while (0) #define RTFREE(_rt) do { \ - RT_LOCK(_rt); \ - RTFREE_LOCKED(_rt); \ - } while (0) + RT_LOCK(_rt); \ + RTFREE_LOCKED(_rt); \ +} while (0) + +#define RT_TEMP_UNLOCK(_rt) do { \ + RT_ADDREF(_rt); \ + RT_UNLOCK(_rt); \ +} while (0) + +#define RT_RELOCK(_rt) do { \ + RT_LOCK(_rt); \ + if ((_rt)->rt_refcnt <= 1) { \ + rtfree(_rt); \ + _rt = 0; /* signal that it went away */ \ + } else { \ + RT_REMREF(_rt); \ + /* note that _rt is still valid */ \ + } \ +} while (0) extern struct radix_node_head *rt_tables[][AF_MAX+1]; @@ -352,6 +368,7 @@ int rt_setgate(struct rtentry *, struct int rtexpunge(struct rtentry *); void rtfree(struct rtentry *); +int rt_check(struct rtentry **, struct rtentry **, struct sockaddr *); /* XXX MRT COMPAT VERSIONS THAT SET UNIVERSE to 0 */ /* Thes are used by old code not yet converted to use multiple FIBS */ @@ -366,7 +383,6 @@ void rtredirect(struct sockaddr *, stru int rtrequest(int, struct sockaddr *, struct sockaddr *, struct sockaddr *, int, struct rtentry **); int rtrequest1(int, struct rt_addrinfo *, struct rtentry **); -int rt_check(struct rtentry **, struct rtentry **, struct sockaddr *); /* defaults to "all" FIBs */ int rtinit_fib(struct ifaddr *, int, int); @@ -385,7 +401,6 @@ void rtredirect_fib(struct sockaddr *, int rtrequest_fib(int, struct sockaddr *, struct sockaddr *, struct sockaddr *, int, struct rtentry **, u_int); int rtrequest1_fib(int, struct rt_addrinfo *, struct rtentry **, u_int); -int rt_check_fib(struct rtentry **, struct rtentry **, struct sockaddr *, u_int); #include <sys/eventhandler.h> typedef void (*rtevent_arp_update_fn)(void *, struct rtentry *, uint8_t *, struct sockaddr *); Modified: stable/7/sys/netinet/if_ether.c ============================================================================== --- stable/7/sys/netinet/if_ether.c Thu Nov 6 21:47:02 2008 (r184738) +++ stable/7/sys/netinet/if_ether.c Thu Nov 6 22:11:57 2008 (r184739) @@ -362,7 +362,7 @@ arpresolve(struct ifnet *ifp, struct rte struct rtentry *rt = NULL; struct sockaddr_dl *sdl; int error; - int fibnum = 0; + int fibnum = -1; if (m) { @@ -379,7 +379,7 @@ arpresolve(struct ifnet *ifp, struct rte if (rt0 != NULL) { /* Look for a cached arp (ll) entry. */ - error = in_rt_check(&rt, &rt0, dst, fibnum); + error = rt_check(&rt, &rt0, dst); if (error) { m_freem(m); return error; @@ -388,14 +388,23 @@ arpresolve(struct ifnet *ifp, struct rte if (la == NULL) RT_UNLOCK(rt); } + + /* + * If we had no mbuf and no route, then hope the caller + * has a fib in mind because we are running out of ideas. + * I think this should not happen in current code. + * (kmacy would know). + */ + if (fibnum == -1) + fibnum = curthread->td_proc->p_fibnum; /* last gasp */ + if (la == NULL) { /* * We enter this block if rt0 was NULL, - * or if rt found by in_rt_check() didn't have llinfo. - * we should get a cloned route, which since it should - * come from the local interface should have a ll entry. - * if may be incoplete but that's ok. - * XXXMRT if we haven't found a fibnum is that OK? + * or if rt found by rt_check() didn't have llinfo. + * We should get a cloned route from the local interface, + * so it should have an ll entry. + * It may be incomplete but that's ok. */ rt = arplookup(SIN(dst)->sin_addr.s_addr, 1, 0, fibnum); if (rt == NULL) { Modified: stable/7/sys/netinet/in_rmx.c ============================================================================== --- stable/7/sys/netinet/in_rmx.c Thu Nov 6 21:47:02 2008 (r184738) +++ stable/7/sys/netinet/in_rmx.c Thu Nov 6 22:11:57 2008 (r184739) @@ -465,13 +465,6 @@ in_rtalloc1(struct sockaddr *dst, int re return (rtalloc1_fib(dst, report, ignflags, fibnum)); } -int -in_rt_check(struct rtentry **lrt, struct rtentry **lrt0, - struct sockaddr *dst, u_int fibnum) -{ - return (rt_check_fib(lrt, lrt0, dst, fibnum)); -} - void in_rtredirect(struct sockaddr *dst, struct sockaddr *gateway, Modified: stable/7/sys/netinet/in_var.h ============================================================================== --- stable/7/sys/netinet/in_var.h Thu Nov 6 21:47:02 2008 (r184738) +++ stable/7/sys/netinet/in_var.h Thu Nov 6 22:11:57 2008 (r184739) @@ -314,7 +314,6 @@ void in_rtredirect(struct sockaddr *, s struct sockaddr *, int, struct sockaddr *, u_int); int in_rtrequest(int, struct sockaddr *, struct sockaddr *, struct sockaddr *, int, struct rtentry **, u_int); -int in_rt_check(struct rtentry **, struct rtentry **, struct sockaddr *, u_int); #if 0 int in_rt_getifa(struct rt_addrinfo *, u_int fibnum);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200811062211.mA6MBvdU066984>