Module Name: src
Committed By: dyoung
Date: Mon Feb 8 17:59:06 UTC 2010
Modified Files:
src/sys/net/agr: if_agr.c if_agrtimer.c if_agrvar_impl.h
Log Message:
Take another stab at fixing the LOCKDEBUG panic reported in PR
kern/39940 and by Martti Kuparinen on current-users@: replace the
ioctl lock with finer-grained locking. Lock the ports list and
wait to if_clone_destroy() until all threads are out of the softc.
Thanks to Martti Kuparinen for testing these changes.
To generate a diff of this commit:
cvs rdiff -u -r1.25 -r1.26 src/sys/net/agr/if_agr.c
cvs rdiff -u -r1.5 -r1.6 src/sys/net/agr/if_agrtimer.c
cvs rdiff -u -r1.8 -r1.9 src/sys/net/agr/if_agrvar_impl.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/net/agr/if_agr.c
diff -u src/sys/net/agr/if_agr.c:1.25 src/sys/net/agr/if_agr.c:1.26
--- src/sys/net/agr/if_agr.c:1.25 Tue Jan 19 22:08:16 2010
+++ src/sys/net/agr/if_agr.c Mon Feb 8 17:59:06 2010
@@ -1,4 +1,4 @@
-/* $NetBSD: if_agr.c,v 1.25 2010/01/19 22:08:16 pooka Exp $ */
+/* $NetBSD: if_agr.c,v 1.26 2010/02/08 17:59:06 dyoung Exp $ */
/*-
* Copyright (c)2005 YAMAMOTO Takashi,
@@ -27,7 +27,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_agr.c,v 1.25 2010/01/19 22:08:16 pooka Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_agr.c,v 1.26 2010/02/08 17:59:06 dyoung Exp $");
#include "opt_inet.h"
@@ -41,6 +41,7 @@
#include <sys/sockio.h>
#include <sys/proc.h> /* XXX for curproc */
#include <sys/kauth.h>
+#include <sys/xcall.h>
#include <net/bpf.h>
#include <net/if.h>
@@ -64,9 +65,9 @@
static int agr_clone_create(struct if_clone *, int);
static int agr_clone_destroy(struct ifnet *);
static void agr_start(struct ifnet *);
-static int agr_setconfig(struct ifnet *, const struct agrreq *);
-static int agr_getconfig(struct ifnet *, struct agrreq *);
-static int agr_getportlist(struct ifnet *, struct agrreq *);
+static int agr_setconfig(struct agr_softc *, const struct agrreq *);
+static int agr_getconfig(struct agr_softc *, struct agrreq *);
+static int agr_getportlist(struct agr_softc *, struct agrreq *);
static int agr_addport(struct ifnet *, struct ifnet *);
static int agr_remport(struct ifnet *, struct ifnet *);
static int agrreq_copyin(const void *, struct agrreq *);
@@ -80,6 +81,16 @@
static int agrport_config_promisc(struct agr_port *, bool);
static int agrport_cleanup(struct agr_softc *, struct agr_port *);
+static int agr_enter(struct agr_softc *);
+static void agr_exit(struct agr_softc *);
+static int agr_pause(struct agr_softc *);
+static void agr_evacuate(struct agr_softc *);
+static void agr_sync(void);
+static void agr_ports_lock(struct agr_softc *);
+static void agr_ports_unlock(struct agr_softc *);
+static bool agr_ports_enter(struct agr_softc *);
+static void agr_ports_exit(struct agr_softc *);
+
static struct if_clone agr_cloner =
IF_CLONE_INITIALIZER("agr", agr_clone_create, agr_clone_destroy);
@@ -169,20 +180,6 @@
mutex_exit(&sc->sc_lock);
}
-void
-agr_ioctl_lock(struct agr_softc *sc)
-{
-
- mutex_enter(&sc->sc_ioctl_lock);
-}
-
-void
-agr_ioctl_unlock(struct agr_softc *sc)
-{
-
- mutex_exit(&sc->sc_ioctl_lock);
-}
-
/*
* agr_xmit_frame: transmit a pre-built frame.
*/
@@ -330,8 +327,10 @@
sc = agr_alloc_softc();
TAILQ_INIT(&sc->sc_ports);
- mutex_init(&sc->sc_ioctl_lock, MUTEX_DRIVER, IPL_NONE);
- mutex_init(&sc->sc_lock, MUTEX_DRIVER, IPL_NET);
+ mutex_init(&sc->sc_lock, MUTEX_DEFAULT, IPL_NET);
+ mutex_init(&sc->sc_entry_mtx, MUTEX_DEFAULT, IPL_NONE);
+ cv_init(&sc->sc_insc_cv, "agr_softc");
+ cv_init(&sc->sc_ports_cv, "agrports");
agrtimer_init(sc);
ifp = &sc->sc_if;
snprintf(ifp->if_xname, sizeof(ifp->if_xname), "%s%d",
@@ -366,26 +365,24 @@
struct agr_softc *sc = ifp->if_softc;
int error;
- agr_ioctl_lock(sc);
-
- AGR_LOCK(sc);
- if (sc->sc_nports > 0) {
- error = EBUSY;
- } else {
- error = 0;
- }
- AGR_UNLOCK(sc);
-
- agr_ioctl_unlock(sc);
+ if ((error = agr_pause(sc)) != 0)
+ return error;
- if (error == 0) {
- if_detach(ifp);
- mutex_destroy(&sc->sc_ioctl_lock);
- mutex_destroy(&sc->sc_lock);
- agr_free_softc(sc);
- }
+ if_detach(ifp);
+ agrtimer_destroy(sc);
+ /* Now that the ifnet has been detached, and our
+ * component ifnets are disconnected, there can be
+ * no new threads in the softc. Wait for every
+ * thread to get out of the softc.
+ */
+ agr_evacuate(sc);
+ mutex_destroy(&sc->sc_lock);
+ mutex_destroy(&sc->sc_entry_mtx);
+ cv_destroy(&sc->sc_insc_cv);
+ cv_destroy(&sc->sc_ports_cv);
+ agr_free_softc(sc);
- return error;
+ return 0;
}
static struct agr_port *
@@ -457,8 +454,9 @@
}
static int
-agr_setconfig(struct ifnet *ifp, const struct agrreq *ar)
+agr_setconfig(struct agr_softc *sc, const struct agrreq *ar)
{
+ struct ifnet *ifp = &sc->sc_if;
int cmd = ar->ar_cmd;
struct ifnet *ifp_port;
int error = 0;
@@ -475,6 +473,7 @@
return ENOENT;
}
+ agr_ports_lock(sc);
switch (cmd) {
case AGRCMD_ADDPORT:
error = agr_addport(ifp, ifp_port);
@@ -488,14 +487,14 @@
error = EINVAL;
break;
}
+ agr_ports_unlock(sc);
return error;
}
static int
-agr_getportlist(struct ifnet *ifp, struct agrreq *ar)
+agr_getportlist(struct agr_softc *sc, struct agrreq *ar)
{
- struct agr_softc *sc = ifp->if_softc;
struct agr_port *port;
struct agrportlist apl;
struct agrportinfo api;
@@ -550,20 +549,22 @@
}
static int
-agr_getconfig(struct ifnet *ifp, struct agrreq *ar)
+agr_getconfig(struct agr_softc *sc, struct agrreq *ar)
{
int cmd = ar->ar_cmd;
int error;
+ (void)agr_ports_enter(sc);
switch (cmd) {
case AGRCMD_PORTLIST:
- error = agr_getportlist(ifp, ar);
+ error = agr_getportlist(sc, ar);
break;
default:
error = EINVAL;
break;
}
+ agr_ports_exit(sc);
return error;
}
@@ -926,22 +927,142 @@
return 0;
}
+/* Make sure that if any interrupt handlers are out of the softc. */
+static void
+agr_sync(void)
+{
+ uint64_t h;
+
+ if (!mp_online)
+ return;
+
+ h = xc_broadcast(0, (xcfunc_t)nullop, NULL, NULL);
+ xc_wait(h);
+}
+
+static int
+agr_pause(struct agr_softc *sc)
+{
+ int error;
+
+ mutex_enter(&sc->sc_entry_mtx);
+ if ((error = sc->sc_noentry) != 0)
+ goto out;
+
+ sc->sc_noentry = EBUSY;
+
+ while (sc->sc_insc != 0)
+ cv_wait(&sc->sc_insc_cv, &sc->sc_entry_mtx);
+
+ if (sc->sc_nports == 0) {
+ sc->sc_noentry = ENXIO;
+ } else {
+ sc->sc_noentry = 0;
+ error = EBUSY;
+ }
+ cv_broadcast(&sc->sc_insc_cv);
+out:
+ mutex_exit(&sc->sc_entry_mtx);
+ return error;
+}
+
+static void
+agr_evacuate(struct agr_softc *sc)
+{
+ mutex_enter(&sc->sc_entry_mtx);
+ cv_broadcast(&sc->sc_insc_cv);
+ while (sc->sc_insc != 0 || sc->sc_paused != 0)
+ cv_wait(&sc->sc_insc_cv, &sc->sc_entry_mtx);
+ mutex_exit(&sc->sc_entry_mtx);
+
+ agr_sync();
+}
+
+static int
+agr_enter(struct agr_softc *sc)
+{
+ int error;
+
+ mutex_enter(&sc->sc_entry_mtx);
+ sc->sc_paused++;
+ while ((error = sc->sc_noentry) == EBUSY)
+ cv_wait(&sc->sc_insc_cv, &sc->sc_entry_mtx);
+ sc->sc_paused--;
+ if (error == 0)
+ sc->sc_insc++;
+ mutex_exit(&sc->sc_entry_mtx);
+
+ return error;
+}
+
+static void
+agr_exit(struct agr_softc *sc)
+{
+ mutex_enter(&sc->sc_entry_mtx);
+ if (--sc->sc_insc == 0)
+ cv_signal(&sc->sc_insc_cv);
+ mutex_exit(&sc->sc_entry_mtx);
+}
+
+static bool
+agr_ports_enter(struct agr_softc *sc)
+{
+ mutex_enter(&sc->sc_entry_mtx);
+ while (sc->sc_wrports != 0)
+ cv_wait(&sc->sc_ports_cv, &sc->sc_entry_mtx);
+ sc->sc_rdports++;
+ mutex_exit(&sc->sc_entry_mtx);
+
+ return true;
+}
+
+static void
+agr_ports_exit(struct agr_softc *sc)
+{
+ mutex_enter(&sc->sc_entry_mtx);
+ if (--sc->sc_rdports == 0)
+ cv_signal(&sc->sc_ports_cv);
+ mutex_exit(&sc->sc_entry_mtx);
+}
+
+static void
+agr_ports_lock(struct agr_softc *sc)
+{
+ mutex_enter(&sc->sc_entry_mtx);
+ while (sc->sc_rdports != 0)
+ cv_wait(&sc->sc_ports_cv, &sc->sc_entry_mtx);
+ sc->sc_wrports = true;
+ mutex_exit(&sc->sc_entry_mtx);
+}
+
+static void
+agr_ports_unlock(struct agr_softc *sc)
+{
+ mutex_enter(&sc->sc_entry_mtx);
+ sc->sc_wrports = false;
+ cv_signal(&sc->sc_ports_cv);
+ mutex_exit(&sc->sc_entry_mtx);
+}
+
static int
-agr_ioctl(struct ifnet *ifp, u_long cmd, void *data)
+agr_ioctl(struct ifnet *ifp, const u_long cmd, void *data)
{
struct agr_softc *sc = ifp->if_softc;
struct ifreq *ifr = (struct ifreq *)data;
struct ifaddr *ifa = (struct ifaddr *)data;
struct agrreq ar;
- int error = 0;
+ int error;
+ bool in_ports = false;
int s;
- agr_ioctl_lock(sc);
+ if ((error = agr_enter(sc)) != 0)
+ return error;
s = splnet();
switch (cmd) {
case SIOCINITIFADDR:
+ in_ports = agr_ports_enter(sc);
if (sc->sc_nports == 0) {
error = EINVAL;
break;
@@ -985,7 +1106,7 @@
error = agrreq_copyin(ifr->ifr_data, &ar);
}
if (!error) {
- error = agr_setconfig(ifp, &ar);
+ error = agr_setconfig(sc, &ar);
}
s = splnet();
break;
@@ -994,7 +1115,7 @@
splx(s);
error = agrreq_copyin(ifr->ifr_data, &ar);
if (!error) {
- error = agr_getconfig(ifp, &ar);
+ error = agr_getconfig(sc, &ar);
}
if (!error) {
error = agrreq_copyout(ifr->ifr_data, &ar);
@@ -1004,11 +1125,11 @@
case SIOCADDMULTI:
case SIOCDELMULTI:
- if (sc->sc_nports == 0) {
+ in_ports = agr_ports_enter(sc);
+ if (sc->sc_nports == 0)
error = EINVAL;
- break;
- }
- error = agr_ioctl_multi(ifp, cmd, ifr);
+ else
+ error = agr_ioctl_multi(ifp, cmd, ifr);
break;
default:
@@ -1016,9 +1137,12 @@
break;
}
+ if (in_ports)
+ agr_ports_exit(sc);
+
splx(s);
- agr_ioctl_unlock(sc);
+ agr_exit(sc);
return error;
}
Index: src/sys/net/agr/if_agrtimer.c
diff -u src/sys/net/agr/if_agrtimer.c:1.5 src/sys/net/agr/if_agrtimer.c:1.6
--- src/sys/net/agr/if_agrtimer.c:1.5 Mon Jul 9 21:11:01 2007
+++ src/sys/net/agr/if_agrtimer.c Mon Feb 8 17:59:06 2010
@@ -1,4 +1,4 @@
-/* $NetBSD: if_agrtimer.c,v 1.5 2007/07/09 21:11:01 ad Exp $ */
+/* $NetBSD: if_agrtimer.c,v 1.6 2010/02/08 17:59:06 dyoung Exp $ */
/*-
* Copyright (c)2005 YAMAMOTO Takashi,
@@ -27,7 +27,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_agrtimer.c,v 1.5 2007/07/09 21:11:01 ad Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_agrtimer.c,v 1.6 2010/02/08 17:59:06 dyoung Exp $");
#include <sys/param.h>
#include <sys/callout.h>
@@ -51,6 +51,13 @@
}
void
+agrtimer_destroy(struct agr_softc *sc)
+{
+
+ callout_destroy(&sc->sc_callout);
+}
+
+void
agrtimer_start(struct agr_softc *sc)
{
Index: src/sys/net/agr/if_agrvar_impl.h
diff -u src/sys/net/agr/if_agrvar_impl.h:1.8 src/sys/net/agr/if_agrvar_impl.h:1.9
--- src/sys/net/agr/if_agrvar_impl.h:1.8 Fri May 29 04:57:05 2009
+++ src/sys/net/agr/if_agrvar_impl.h Mon Feb 8 17:59:06 2010
@@ -1,4 +1,4 @@
-/* $NetBSD: if_agrvar_impl.h,v 1.8 2009/05/29 04:57:05 darran Exp $ */
+/* $NetBSD: if_agrvar_impl.h,v 1.9 2010/02/08 17:59:06 dyoung Exp $ */
/*-
* Copyright (c)2005 YAMAMOTO Takashi,
@@ -103,8 +103,15 @@
};
struct agr_softc {
- kmutex_t sc_ioctl_lock;
+ kmutex_t sc_entry_mtx;
kmutex_t sc_lock;
+ kcondvar_t sc_ports_cv;
+ kcondvar_t sc_insc_cv;
+ int sc_noentry;
+ int sc_insc;
+ int sc_wrports;
+ int sc_rdports;
+ int sc_paused;
struct callout sc_callout;
int sc_nports;
TAILQ_HEAD(, agr_port) sc_ports;
@@ -125,9 +132,6 @@
void agr_lock(struct agr_softc *);
void agr_unlock(struct agr_softc *);
-void agr_ioctl_lock(struct agr_softc *);
-void agr_ioctl_unlock(struct agr_softc *);
-
int agrport_ioctl(struct agr_port *, u_long, void *);
struct agr_softc *agr_alloc_softc(void);
@@ -139,6 +143,7 @@
#define AGR_STATIC(sc) (((sc)->sc_if.if_flags & IFF_LINK1) != 0)
void agrtimer_init(struct agr_softc *);
+void agrtimer_destroy(struct agr_softc *);
void agrtimer_start(struct agr_softc *);
void agrtimer_stop(struct agr_softc *);
