Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys/dev/usb more smp cleanup for ure(4)/axen(4)/cdce(4):



details:   https://anonhg.NetBSD.org/src/rev/2816baf46f5d
branches:  trunk
changeset: 842368:2816baf46f5d
user:      mrg <mrg%NetBSD.org@localhost>
date:      Fri Jun 28 01:57:43 2019 +0000

description:
more smp cleanup for ure(4)/axen(4)/cdce(4):

- convert IFF_ALLMULTI to ETHER_F_ALLMULTI, using ETHER_LOCK()
- remove IFF_OACTIVE use, and simply check the ring count in start
- assert/take more locks
- XXX: IFF_RUNNING is not properly protected (all driver problem)
- fix axen_timer setting so it actually runs
- document a locking issue in stop callback:
  stop is called with the softc lock held, but the lock order
  in all other places is ifnet -> softc -> rx -> tx, so taking
  ifnet lock when softc lock is held would be problematic
- in rxeof check for stopping/dying more often.  i managed to
  trigger a pagefault in cdce_rxeof() when yanking an active
  device as it attempted to usbd_setup_xfer() on closed pipes.
- add missing USBD_MPSAFE and cdce_stopping resetting for cdce(4)

between this and other recent clean ups increase performance of
these drivers mostly.  some numbers (in mbit/sec):

        old:                            new:
driver  in      out     in+out          in      out     in+out
----    --      ---     ------          --      ---     ------
cdce    39      32      44              38      33      54
axen    44      34      45              48      37      42
ure     36      34      35              36      38      38

i'm not sure why axen drops a little with in+out.  cdce is
helped quite a lot, and ure a little.  it is disappointing that
ure does not outperform cdce -- it's the same actual hardware,
and the device-specific (ure) should outperform the generic
cdce driver...

diffstat:

 sys/dev/usb/if_axen.c |  47 +++++++++++++++++++++------------
 sys/dev/usb/if_cdce.c |  70 +++++++++++++++++++++++++++++++++++++-------------
 sys/dev/usb/if_ure.c  |  45 ++++++++++++++++++++++----------
 3 files changed, 112 insertions(+), 50 deletions(-)

diffs (truncated from 519 to 300 lines):

diff -r 8e831221a337 -r 2816baf46f5d sys/dev/usb/if_axen.c
--- a/sys/dev/usb/if_axen.c     Thu Jun 27 19:56:10 2019 +0000
+++ b/sys/dev/usb/if_axen.c     Fri Jun 28 01:57:43 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: if_axen.c,v 1.48 2019/06/22 10:58:39 mrg Exp $ */
+/*     $NetBSD: if_axen.c,v 1.49 2019/06/28 01:57:43 mrg Exp $ */
 /*     $OpenBSD: if_axen.c,v 1.3 2013/10/21 10:10:22 yuo Exp $ */
 
 /*
@@ -23,7 +23,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_axen.c,v 1.48 2019/06/22 10:58:39 mrg Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_axen.c,v 1.49 2019/06/28 01:57:43 mrg Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_inet.h"
@@ -455,12 +455,14 @@
        rxmode = le16toh(wval);
        rxmode &= ~(AXEN_RXCTL_ACPT_ALL_MCAST | AXEN_RXCTL_PROMISC |
            AXEN_RXCTL_ACPT_MCAST);
-       ifp->if_flags &= ~IFF_ALLMULTI;
 
        if (ifp->if_flags & IFF_PROMISC) {
                DPRINTF(("%s: promisc\n", device_xname(sc->axen_dev)));
                rxmode |= AXEN_RXCTL_PROMISC;
-allmulti:      ifp->if_flags |= IFF_ALLMULTI;
+allmulti:
+               ETHER_LOCK(ec);
+               ec->ec_flags |= ETHER_F_ALLMULTI;
+               ETHER_UNLOCK(ec);
                rxmode |= AXEN_RXCTL_ACPT_ALL_MCAST
                /* | AXEN_RXCTL_ACPT_PHY_MCAST */;
        } else {
@@ -468,6 +470,8 @@
                DPRINTF(("%s: initializing hash table\n",
                    device_xname(sc->axen_dev)));
                ETHER_LOCK(ec);
+               ec->ec_flags &= ~ETHER_F_ALLMULTI;
+
                ETHER_FIRST_MULTI(step, ec, enm);
                while (enm != NULL) {
                        if (memcmp(enm->enm_addrlo, enm->enm_addrhi,
@@ -979,8 +983,11 @@
        usb_rem_task_wait(sc->axen_udev, &sc->axen_tick_task,
            USB_TASKQ_DRIVER, NULL);
 
-       if (ifp->if_flags & IFF_RUNNING)
+       if (ifp->if_flags & IFF_RUNNING) {
+               IFNET_LOCK(ifp);
                axen_stop(ifp, 1);
+               IFNET_UNLOCK(ifp);
+       }
 
        mutex_enter(&sc->axen_lock);
        sc->axen_refcnt--;
@@ -1256,7 +1263,7 @@
                if_percpuq_enqueue((ifp)->if_percpuq, (m));
 
                mutex_enter(&sc->axen_rxlock);
-               if (sc->axen_stopping) {
+               if (sc->axen_dying || sc->axen_stopping) {
                        mutex_exit(&sc->axen_rxlock);
                        return;
                }
@@ -1274,6 +1281,11 @@
        } while (pkt_count > 0);
 
 done:
+       if (sc->axen_dying || sc->axen_stopping) {
+               mutex_exit(&sc->axen_rxlock);
+               return;
+       }
+
        mutex_exit(&sc->axen_rxlock);
 
        /* Setup new transfer. */
@@ -1351,7 +1363,6 @@
        cd->axen_tx_cnt--;
 
        sc->axen_timer = 0;
-       ifp->if_flags &= ~IFF_OACTIVE;
 
        switch (status) {
        case USBD_NOT_STARTED:
@@ -1489,6 +1500,7 @@
        /* Transmit */
        err = usbd_transfer(c->axen_xfer);
        if (err != USBD_IN_PROGRESS) {
+               /* XXXSMP IFNET_LOCK */
                axen_stop(ifp, 0);
                return EIO;
        }
@@ -1505,11 +1517,9 @@
        int idx;
 
        KASSERT(mutex_owned(&sc->axen_txlock));
+       KASSERT(cd->axen_tx_cnt <= AXEN_TX_LIST_CNT);
 
-       if (sc->axen_link == 0)
-               return;
-
-       if ((ifp->if_flags & (IFF_OACTIVE | IFF_RUNNING)) != IFF_RUNNING)
+       if (sc->axen_link == 0 || (ifp->if_flags & IFF_RUNNING) == 0)
                return;
 
        idx = cd->axen_tx_prod;
@@ -1519,7 +1529,6 @@
                        break;
 
                if (axen_encap(sc, m, idx)) {
-                       ifp->if_flags |= IFF_OACTIVE; /* XXX */
                        ifp->if_oerrors++;
                        break;
                }
@@ -1537,9 +1546,6 @@
        }
        cd->axen_tx_prod = idx;
 
-       if (cd->axen_tx_cnt >= AXEN_TX_LIST_CNT)
-               ifp->if_flags |= IFF_OACTIVE;
-
        /*
         * Set a timeout in case the chip goes out to lunch.
         */
@@ -1646,8 +1652,8 @@
        mutex_exit(&sc->axen_rxlock);
 
        /* Indicate we are up and running. */
+       KASSERT(IFNET_LOCKED(ifp));
        ifp->if_flags |= IFF_RUNNING;
-       ifp->if_flags &= ~IFF_OACTIVE;
 
        callout_schedule(&sc->axen_stat_ch, hz);
        return 0;
@@ -1771,8 +1777,15 @@
        axen_cmd(sc, AXEN_CMD_MAC_WRITE2, 2, AXEN_MAC_RXCTL, &wval);
        axen_unlock_mii_sc_locked(sc);
 
+       /*
+        * XXXSMP Would like to
+        *      KASSERT(IFNET_LOCKED(ifp))
+        * here but the locking order is:
+        *      ifnet -> sc lock -> rxlock -> txlock
+        * and sc lock is already held.
+        */
+       ifp->if_flags &= ~IFF_RUNNING;
        sc->axen_timer = 0;
-       ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE);
 
        callout_stop(&sc->axen_stat_ch);
        sc->axen_link = 0;
diff -r 8e831221a337 -r 2816baf46f5d sys/dev/usb/if_cdce.c
--- a/sys/dev/usb/if_cdce.c     Thu Jun 27 19:56:10 2019 +0000
+++ b/sys/dev/usb/if_cdce.c     Fri Jun 28 01:57:43 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: if_cdce.c,v 1.50 2019/06/23 02:14:14 mrg Exp $ */
+/*     $NetBSD: if_cdce.c,v 1.51 2019/06/28 01:57:43 mrg Exp $ */
 
 /*
  * Copyright (c) 1997, 1998, 1999, 2000-2003 Bill Paul <wpaul%windriver.com@localhost>
@@ -41,7 +41,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_cdce.c,v 1.50 2019/06/23 02:14:14 mrg Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_cdce.c,v 1.51 2019/06/28 01:57:43 mrg Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_inet.h"
@@ -146,7 +146,6 @@
 static void     cdce_start(struct ifnet *);
 static int      cdce_ioctl(struct ifnet *, u_long, void *);
 static void     cdce_init(void *);
-static void     cdce_watchdog(struct ifnet *);
 static void     cdce_stop(struct cdce_softc *);
 static void     cdce_tick(void *);
 static void     cdce_tick_task(void *);
@@ -384,8 +383,11 @@
        usb_rem_task_wait(sc->cdce_udev, &sc->cdce_tick_task,
            USB_TASKQ_DRIVER, NULL);
 
-       if (ifp->if_flags & IFF_RUNNING)
+       if (ifp->if_flags & IFF_RUNNING) {
+               IFNET_LOCK(ifp);
                cdce_stop(sc);
+               IFNET_UNLOCK(ifp);
+       }
 
        callout_destroy(&sc->cdce_stat_ch);
        ether_ifdetach(ifp);
@@ -408,8 +410,8 @@
        struct mbuf             *m_head = NULL;
 
        KASSERT(mutex_owned(&sc->cdce_txlock));
-       if (sc->cdce_dying || sc->cdce_stopping ||
-           (ifp->if_flags & IFF_OACTIVE))
+
+       if (sc->cdce_dying || sc->cdce_cdata.cdce_tx_cnt == CDCE_TX_LIST_CNT)
                return;
 
        IFQ_POLL(&ifp->if_snd, m_head);
@@ -425,12 +427,10 @@
 
        bpf_mtap(ifp, m_head, BPF_D_OUT);
 
-       ifp->if_flags |= IFF_OACTIVE;
-
        /*
         * Set a timeout in case the chip goes out to lunch.
         */
-       ifp->if_timer = 6;
+       sc->cdce_timer = 6;
 }
 
 static void
@@ -469,30 +469,42 @@
            USBD_FORCE_SHORT_XFER, 10000, cdce_txeof);
        err = usbd_transfer(c->cdce_xfer);
        if (err != USBD_IN_PROGRESS) {
+               /* XXXSMP IFNET_LOCK */
                cdce_stop(sc);
                return EIO;
        }
 
        sc->cdce_cdata.cdce_tx_cnt++;
+       KASSERT(sc->cdce_cdata.cdce_tx_cnt <= CDCE_TX_LIST_CNT);
 
        return 0;
 }
 
 static void
-cdce_stop(struct cdce_softc *sc)
+cdce_stop_locked(struct cdce_softc *sc)
 {
        usbd_status      err;
        struct ifnet    *ifp = GET_IFP(sc);
        int              i;
 
+       /* XXXSMP can't KASSERT(IFNET_LOCKED(ifp)); */
+       KASSERT(mutex_owned(&sc->cdce_lock));
+
        mutex_enter(&sc->cdce_rxlock);
        mutex_enter(&sc->cdce_txlock);
        sc->cdce_stopping = true;
        mutex_exit(&sc->cdce_txlock);
        mutex_exit(&sc->cdce_rxlock);
 
-       ifp->if_timer = 0;
-       ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE);
+       /*
+        * XXXSMP Would like to
+        *      KASSERT(IFNET_LOCKED(ifp))
+        * here but the locking order is:
+        *      ifnet -> sc lock -> rxlock -> txlock
+        * and sc lock is already held.
+        */
+       ifp->if_flags &= ~IFF_RUNNING;
+       sc->cdce_timer = 0;
 
        callout_stop(&sc->cdce_stat_ch);
 
@@ -549,7 +561,15 @@
                            device_xname(sc->cdce_dev), usbd_errstr(err));
                sc->cdce_bulkout_pipe = NULL;
        }
-       mutex_exit(&sc->cdce_txlock);
+}
+
+static void
+cdce_stop(struct cdce_softc *sc)
+{
+
+       mutex_enter(&sc->cdce_lock);
+       cdce_stop_locked(sc);
+       mutex_exit(&sc->cdce_lock);
 }
 
 static int
@@ -613,7 +633,7 @@
 static void
 cdce_watchdog(struct ifnet *ifp)
 {
-       struct cdce_softc       *sc = ifp->if_softc;
+       struct cdce_softc *sc = ifp->if_softc;
        struct cdce_chain *c;
        usbd_status stat;
 
@@ -625,12 +645,16 @@
        ifp->if_oerrors++;
        aprint_error_dev(sc->cdce_dev, "watchdog timeout\n");
 
+       mutex_enter(&sc->cdce_txlock);
+
        c = &sc->cdce_cdata.cdce_rx_chain[0];
        usbd_get_xfer_status(c->cdce_xfer, NULL, NULL, NULL, &stat);
        cdce_txeof(c->cdce_xfer, c, stat);
 
        if (!IFQ_IS_EMPTY(&ifp->if_snd))



Home | Main Index | Thread Index | Old Index