[src/trunk]: src/sys/net/agr Take another stab at fixing the LOCKDEBUG panic ...

[dyoung <dyoung%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/e28865f37355
branches:  trunk
changeset: 751914:e28865f37355
user:      dyoung <dyoung%NetBSD.org@localhost>
date:      Mon Feb 08 17:59:06 2010 +0000

description:
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.

diffstat:

 sys/net/agr/if_agr.c         |  232 ++++++++++++++++++++++++++++++++----------
 sys/net/agr/if_agrtimer.c    |   11 +-
 sys/net/agr/if_agrvar_impl.h |   15 +-
 3 files changed, 197 insertions(+), 61 deletions(-)

diffs (truncated from 464 to 300 lines):

diff -r f6b6181146ba -r e28865f37355 sys/net/agr/if_agr.c
--- a/sys/net/agr/if_agr.c      Mon Feb 08 17:19:11 2010 +0000
+++ b/sys/net/agr/if_agr.c      Mon Feb 08 17:59:06 2010 +0000
@@ -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);
+       if ((error = agr_pause(sc)) != 0)
+               return error;
 
-       agr_ioctl_unlock(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);
 
-       if (error == 0) {
-               if_detach(ifp);
-               mutex_destroy(&sc->sc_ioctl_lock);
-               mutex_destroy(&sc->sc_lock);
-               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_ioctl(struct ifnet *ifp, u_long cmd, void *data)
+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)
+{