On Tue, Jul 31, 2012 at 10:43:42PM +0400, Andrey Zonov wrote: A> Please review attached patch. I used Gleb's ideas. He almost fixed the A> issue, but he didn't observe that entry can be safely unlocked in A> arptimer() because it has refcnt incremented and cannot be removed. I A> also fixed entry removing based on refcnt that was totally broken.
Another try. :) Same as Andrey's patch, but I do not remove the lle from single linked list in the LLE_FREE macro. This requires a protection against sequential llentry_free() calls on same lle. diff also includes: - some cleanup of llentry_update and its rename to llentry_alloc(), would be committed sepearately - ktr tracing in macros, won't be commited. -- Totus tuus, Glebius.
Index: netinet/in.c =================================================================== --- netinet/in.c (revision 238967) +++ netinet/in.c (working copy) @@ -1280,7 +1280,6 @@ if (lle == NULL) /* NB: caller generates msg */ return NULL; - callout_init(&lle->base.la_timer, CALLOUT_MPSAFE); /* * For IPv4 this will trigger "arpresolve" to generate * an ARP request. @@ -1290,7 +1289,10 @@ lle->base.lle_refcnt = 1; lle->base.lle_free = in_lltable_free; LLE_LOCK_INIT(&lle->base); - return &lle->base; + callout_init_rw(&lle->base.la_timer, &lle->base.lle_lock, + CALLOUT_RETURNUNLOCKED); + + return (&lle->base); } #define IN_ARE_MASKED_ADDR_EQUAL(d, a, m) ( \ @@ -1306,6 +1308,7 @@ int i; size_t pkts_dropped; + IF_AFDATA_WLOCK(llt->llt_ifp); for (i = 0; i < LLTBL_HASHTBL_SIZE; i++) { LIST_FOREACH_SAFE(lle, &llt->lle_head[i], lle_next, next) { /* @@ -1315,17 +1318,15 @@ if (IN_ARE_MASKED_ADDR_EQUAL(satosin(L3_ADDR(lle)), pfx, msk) && ((flags & LLE_STATIC) || !(lle->la_flags & LLE_STATIC))) { - int canceled; - - canceled = callout_drain(&lle->la_timer); LLE_WLOCK(lle); - if (canceled) + if (callout_stop(&lle->la_timer)) LLE_REMREF(lle); pkts_dropped = llentry_free(lle); ARPSTAT_ADD(dropped, pkts_dropped); } } } + IF_AFDATA_WUNLOCK(llt->llt_ifp); } @@ -1457,11 +1458,12 @@ lle->lle_tbl = llt; lle->lle_head = lleh; + lle->la_flags |= LLE_LINKED; LIST_INSERT_HEAD(lleh, lle, lle_next); } else if (flags & LLE_DELETE) { if (!(lle->la_flags & LLE_IFADDR) || (flags & LLE_IFADDR)) { LLE_WLOCK(lle); - lle->la_flags = LLE_DELETED; + lle->la_flags |= LLE_DELETED; EVENTHANDLER_INVOKE(lle_event, lle, LLENTRY_DELETED); LLE_WUNLOCK(lle); #ifdef DIAGNOSTIC Index: netinet/if_ether.c =================================================================== --- netinet/if_ether.c (revision 238967) +++ netinet/if_ether.c (working copy) @@ -163,49 +163,40 @@ static void arptimer(void *arg) { + struct llentry *lle = (struct llentry *)arg; struct ifnet *ifp; - struct llentry *lle; - int pkts_dropped; + size_t pkts_dropped; - KASSERT(arg != NULL, ("%s: arg NULL", __func__)); - lle = (struct llentry *)arg; + if (lle->la_flags & LLE_STATIC) { + LLE_WUNLOCK(lle); + return; + } + ifp = lle->lle_tbl->llt_ifp; CURVNET_SET(ifp->if_vnet); + + if (lle->la_flags != LLE_DELETED) { + int evt; + + if (lle->la_flags & LLE_VALID) + evt = LLENTRY_EXPIRED; + else + evt = LLENTRY_TIMEDOUT; + EVENTHANDLER_INVOKE(lle_event, lle, evt); + } + + callout_stop(&lle->la_timer); + + /* XXX: LOR avoidance. We still have ref on lle. */ + LLE_WUNLOCK(lle); IF_AFDATA_LOCK(ifp); LLE_WLOCK(lle); - if (lle->la_flags & LLE_STATIC) - LLE_WUNLOCK(lle); - else { - if (!callout_pending(&lle->la_timer) && - callout_active(&lle->la_timer)) { - callout_stop(&lle->la_timer); - LLE_REMREF(lle); - if (lle->la_flags != LLE_DELETED) { - int evt; - - if (lle->la_flags & LLE_VALID) - evt = LLENTRY_EXPIRED; - else - evt = LLENTRY_TIMEDOUT; - EVENTHANDLER_INVOKE(lle_event, lle, evt); - } - - pkts_dropped = llentry_free(lle); - ARPSTAT_ADD(dropped, pkts_dropped); - ARPSTAT_INC(timeouts); - } else { -#ifdef DIAGNOSTIC - struct sockaddr *l3addr = L3_ADDR(lle); - log(LOG_INFO, - "arptimer issue: %p, IPv4 address: \"%s\"\n", lle, - inet_ntoa( - ((const struct sockaddr_in *)l3addr)->sin_addr)); -#endif - LLE_WUNLOCK(lle); - } - } + LLE_REMREF(lle); + pkts_dropped = llentry_free(lle); IF_AFDATA_UNLOCK(ifp); + ARPSTAT_ADD(dropped, pkts_dropped); + ARPSTAT_INC(timeouts); CURVNET_RESTORE(); } Index: netinet6/in6.c =================================================================== --- netinet6/in6.c (revision 238967) +++ netinet6/in6.c (working copy) @@ -2497,23 +2497,22 @@ * (flags & LLE_STATIC) means deleting all entries * including static ND6 entries. */ + IF_AFDATA_WLOCK(llt->llt_ifp); for (i = 0; i < LLTBL_HASHTBL_SIZE; i++) { LIST_FOREACH_SAFE(lle, &llt->lle_head[i], lle_next, next) { if (IN6_ARE_MASKED_ADDR_EQUAL( - &((struct sockaddr_in6 *)L3_ADDR(lle))->sin6_addr, - &pfx->sin6_addr, - &msk->sin6_addr) && - ((flags & LLE_STATIC) || !(lle->la_flags & LLE_STATIC))) { - int canceled; - - canceled = callout_drain(&lle->la_timer); + &satosin6(L3_ADDR(lle))->sin6_addr, + &pfx->sin6_addr, &msk->sin6_addr) && + ((flags & LLE_STATIC) || + !(lle->la_flags & LLE_STATIC))) { LLE_WLOCK(lle); - if (canceled) + if (callout_stop(&lle->la_timer)) LLE_REMREF(lle); llentry_free(lle); } } } + IF_AFDATA_WUNLOCK(llt->llt_ifp); } static int @@ -2605,11 +2604,12 @@ lle->lle_tbl = llt; lle->lle_head = lleh; + lle->la_flags |= LLE_LINKED; LIST_INSERT_HEAD(lleh, lle, lle_next); } else if (flags & LLE_DELETE) { if (!(lle->la_flags & LLE_IFADDR) || (flags & LLE_IFADDR)) { LLE_WLOCK(lle); - lle->la_flags = LLE_DELETED; + lle->la_flags |= LLE_DELETED; LLE_WUNLOCK(lle); #ifdef DIAGNOSTIC log(LOG_INFO, "ifaddr cache = %p is deleted\n", lle); Index: net/if_var.h =================================================================== --- net/if_var.h (revision 238967) +++ net/if_var.h (working copy) @@ -415,6 +415,8 @@ #define IF_AFDATA_DESTROY(ifp) rw_destroy(&(ifp)->if_afdata_lock) #define IF_AFDATA_LOCK_ASSERT(ifp) rw_assert(&(ifp)->if_afdata_lock, RA_LOCKED) +#define IF_AFDATA_RLOCK_ASSERT(ifp) rw_assert(&(ifp)->if_afdata_lock, RA_RLOCKED) +#define IF_AFDATA_WLOCK_ASSERT(ifp) rw_assert(&(ifp)->if_afdata_lock, RA_WLOCKED) #define IF_AFDATA_UNLOCK_ASSERT(ifp) rw_assert(&(ifp)->if_afdata_lock, RA_UNLOCKED) int if_handoff(struct ifqueue *ifq, struct mbuf *m, struct ifnet *ifp, Index: net/flowtable.c =================================================================== --- net/flowtable.c (revision 238967) +++ net/flowtable.c (working copy) @@ -1258,7 +1258,7 @@ else l3addr = (struct sockaddr_storage *)&ro->ro_dst; - llentry_update(&lle, LLTABLE6(ifp), l3addr, ifp); + lle = llentry_alloc(ifp, LLTABLE6(ifp), l3addr); } #endif #ifdef INET @@ -1267,7 +1267,7 @@ l3addr = (struct sockaddr_storage *)rt->rt_gateway; else l3addr = (struct sockaddr_storage *)&ro->ro_dst; - llentry_update(&lle, LLTABLE(ifp), l3addr, ifp); + lle = llentry_alloc(ifp, LLTABLE(ifp), l3addr); } #endif Index: net/if_llatbl.c =================================================================== --- net/if_llatbl.c (revision 238967) +++ net/if_llatbl.c (working copy) @@ -106,10 +106,19 @@ size_t pkts_dropped; struct mbuf *next; - pkts_dropped = 0; + IF_AFDATA_WLOCK_ASSERT(lle->lle_tbl->llt_ifp); LLE_WLOCK_ASSERT(lle); + + /* XXX: guard against race with other llentry_free(). */ + if (!(lle->la_flags & LLE_LINKED)) { + LLE_FREE_LOCKED(lle); + return (0); + } + LIST_REMOVE(lle, lle_next); + lle->la_flags &= ~(LLE_VALID | LLE_LINKED); + pkts_dropped = 0; while ((lle->la_numheld > 0) && (lle->la_hold != NULL)) { next = lle->la_hold->m_nextpkt; m_freem(lle->la_hold); @@ -122,49 +131,39 @@ ("%s: la_numheld %d > 0, pkts_droped %zd", __func__, lle->la_numheld, pkts_dropped)); - lle->la_flags &= ~LLE_VALID; LLE_FREE_LOCKED(lle); return (pkts_dropped); } /* - * Update an llentry for address dst (equivalent to rtalloc for new-arp) - * Caller must pass in a valid struct llentry * (or NULL) + * (al)locate an llentry for address dst (equivalent to rtalloc for new-arp). * - * if found the llentry * is returned referenced and unlocked + * If found the llentry * is returned referenced and unlocked. */ -int -llentry_update(struct llentry **llep, struct lltable *lt, - struct sockaddr_storage *dst, struct ifnet *ifp) +struct llentry * +llentry_alloc(struct ifnet *ifp, struct lltable *lt, + struct sockaddr_storage *dst) { struct llentry *la; IF_AFDATA_RLOCK(ifp); - la = lla_lookup(lt, LLE_EXCLUSIVE, - (struct sockaddr *)dst); + la = lla_lookup(lt, LLE_EXCLUSIVE, (struct sockaddr *)dst); IF_AFDATA_RUNLOCK(ifp); if ((la == NULL) && (ifp->if_flags & (IFF_NOARP | IFF_STATICARP)) == 0) { IF_AFDATA_WLOCK(ifp); - la = lla_lookup(lt, - (LLE_CREATE | LLE_EXCLUSIVE), + la = lla_lookup(lt, (LLE_CREATE | LLE_EXCLUSIVE), (struct sockaddr *)dst); IF_AFDATA_WUNLOCK(ifp); } - if (la != NULL && (*llep != la)) { - if (*llep != NULL) - LLE_FREE(*llep); + + if (la != NULL) { LLE_ADDREF(la); LLE_WUNLOCK(la); - *llep = la; - } else if (la != NULL) - LLE_WUNLOCK(la); + } - if (la == NULL) - return (ENOENT); - - return (0); + return (la); } /* @@ -182,17 +181,16 @@ SLIST_REMOVE(&V_lltables, llt, lltable, llt_link); LLTABLE_WUNLOCK(); + IF_AFDATA_WLOCK(llt->llt_ifp); for (i = 0; i < LLTBL_HASHTBL_SIZE; i++) { LIST_FOREACH_SAFE(lle, &llt->lle_head[i], lle_next, next) { - int canceled; - - canceled = callout_drain(&lle->la_timer); LLE_WLOCK(lle); - if (canceled) + if (callout_stop(&lle->la_timer)) LLE_REMREF(lle); llentry_free(lle); } } + IF_AFDATA_WUNLOCK(llt->llt_ifp); free(llt, M_LLTABLE); } Index: net/if_llatbl.h =================================================================== --- net/if_llatbl.h (revision 238967) +++ net/if_llatbl.h (working copy) @@ -33,6 +33,7 @@ #include "opt_ofed.h" #include <sys/_rwlock.h> +#include <sys/ktr.h> #include <netinet/in.h> struct ifnet; @@ -103,26 +104,31 @@ #define LLE_ADDREF(lle) do { \ LLE_WLOCK_ASSERT(lle); \ KASSERT((lle)->lle_refcnt >= 0, \ - ("negative refcnt %d", (lle)->lle_refcnt)); \ + ("negative refcnt %d on lle %p", \ + (lle)->lle_refcnt, (lle))); \ + CTR2(KTR_SPARE3, "LLE_ADDREF %p %d", (lle), (lle)->lle_refcnt); \ (lle)->lle_refcnt++; \ } while (0) #define LLE_REMREF(lle) do { \ LLE_WLOCK_ASSERT(lle); \ - KASSERT((lle)->lle_refcnt > 1, \ - ("bogus refcnt %d", (lle)->lle_refcnt)); \ + KASSERT((lle)->lle_refcnt > 0, \ + ("bogus refcnt %d on lle %p", \ + (lle)->lle_refcnt, (lle))); \ + CTR2(KTR_SPARE3, "LLE_REMREF %p %d", (lle), (lle)->lle_refcnt); \ (lle)->lle_refcnt--; \ } while (0) #define LLE_FREE_LOCKED(lle) do { \ - if ((lle)->lle_refcnt <= 1) \ - (lle)->lle_free((lle)->lle_tbl, (lle));\ + CTR2(KTR_SPARE3, "LLE_FREE %p %d", (lle), (lle)->lle_refcnt); \ + if ((lle)->lle_refcnt == 1) \ + (lle)->lle_free((lle)->lle_tbl, (lle)); \ else { \ - (lle)->lle_refcnt--; \ + LLE_REMREF(lle); \ LLE_WUNLOCK(lle); \ } \ /* guard against invalid refs */ \ - lle = NULL; \ + (lle) = NULL; \ } while (0) #define LLE_FREE(lle) do { \ @@ -172,6 +178,7 @@ #define LLE_VALID 0x0008 /* ll_addr is valid */ #define LLE_PROXY 0x0010 /* proxy entry ??? */ #define LLE_PUB 0x0020 /* publish entry ??? */ +#define LLE_LINKED 0x0040 /* linked to lookup structure */ #define LLE_EXCLUSIVE 0x2000 /* return lle xlocked */ #define LLE_DELETE 0x4000 /* delete on a lookup - match LLE_IFADDR */ #define LLE_CREATE 0x8000 /* create on a lookup miss */ @@ -189,8 +196,8 @@ int lltable_sysctl_dumparp(int, struct sysctl_req *); size_t llentry_free(struct llentry *); -int llentry_update(struct llentry **, struct lltable *, - struct sockaddr_storage *, struct ifnet *); +struct llentry *llentry_alloc(struct ifnet *, struct lltable *, + struct sockaddr_storage *); /* * Generic link layer address lookup function.
_______________________________________________ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"