Date: Thu, 2 Aug 2012 14:20:39 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Andrey Zonov <andrey@zonov.org> Cc: freebsd-net@FreeBSD.org, Ryan Stone <rysto32@gmail.com> Subject: Re: kern/165863 Message-ID: <20120802102039.GC70185@glebius.int.ru> In-Reply-To: <5018275E.6090901@zonov.org> References: <201203090930.q299UCPX057950@freefall.freebsd.org> <CAFMmRNwWZFzbqG7ejdEqsMTKzxxZv-ZbGGJGr98mYcE_ku7xMQ@mail.gmail.com> <20120410065432.GJ9391@glebius.int.ru> <CAFMmRNzqG_AwyvPnFid4XapWTmJYNU-50WUDdPsJpsRdo1F0bw@mail.gmail.com> <5018275E.6090901@zonov.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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.
[-- Attachment #2 --]
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120802102039.GC70185>