[src/trunk]: src/sys Make if_timer MP-safe if IFEF_MPSAFE

[ozaki-r <ozaki-r%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/7d31c1c1e364
branches:  trunk
changeset: 828269:7d31c1c1e364
user:      ozaki-r <ozaki-r%NetBSD.org@localhost>
date:      Wed Dec 06 09:03:12 2017 +0000

description:
Make if_timer MP-safe if IFEF_MPSAFE

if_timer, a counter used by if_watchdog (if_slowtimo), can be modified in
if_watchdog and if_start and/or interrupt handlers of some device drivers. All
such accesses were serialized by KERNEL_LOCK. If IFEF_MPSAFE is enabled,
KERNEL_LOCK of if_start (and perhaps interrupt handlers) is omitted and if_timer
becomes racy.

Fix the race condition by protecting if_timer by a spin mutex. if_watchdog_reset
and if_watchdog_stop are introduced to ensure to take the mutex on accessing
if_timer. Interface with IFEF_MPSAFE enabled must use the functions.

In addition, if_watchdog callout is now set CALLOUT_MPSAFE if IFEF_MPSAFE. It
means that if_watchdog implemented by a driver must be MP-safe if the driver is
set IFEF_MPSAFE.

Currenlty interfaces with IFEF_MPSAFE implementing if_watchdog and accessing
if_timer in if_start and interrupt handlers are only wm(4). wm is changed to
use the functions. (Its watchdog handler (wm_watchdog) is already MP-safe.

These contracts will be written somewhere in a further commit.

Note that the spin mutex is now ifp->if_snd.ifq_lock to avoid adding another
spin mutex to each interface. For now reusing it isn't problematic (see the
comment to know why) thought if that does matter in the future, feel free to
replace it with a new spin mutex. It's easy to do.

diffstat:

 sys/dev/pci/if_wm.c |  12 ++++++------
 sys/net/if.c        |  43 ++++++++++++++++++++++++++++++++++++++-----
 sys/net/if.h        |   5 ++++-
 3 files changed, 48 insertions(+), 12 deletions(-)

diffs (166 lines):

diff -r 321869cf9682 -r 7d31c1c1e364 sys/dev/pci/if_wm.c
--- a/sys/dev/pci/if_wm.c       Wed Dec 06 08:38:33 2017 +0000
+++ b/sys/dev/pci/if_wm.c       Wed Dec 06 09:03:12 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: if_wm.c,v 1.546 2017/11/30 09:24:18 msaitoh Exp $      */
+/*     $NetBSD: if_wm.c,v 1.547 2017/12/06 09:03:12 ozaki-r Exp $      */
 
 /*
  * Copyright (c) 2001, 2002, 2003, 2004 Wasabi Systems, Inc.
@@ -83,7 +83,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_wm.c,v 1.546 2017/11/30 09:24:18 msaitoh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_wm.c,v 1.547 2017/12/06 09:03:12 ozaki-r Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_net_mpsafe.h"
@@ -5932,7 +5932,7 @@
 
        /* Mark the interface as down and cancel the watchdog timer. */
        ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE);
-       ifp->if_timer = 0;
+       if_watchdog_stop(ifp);
 
        if (disable) {
                for (i = 0; i < sc->sc_nqueues; i++) {
@@ -7368,7 +7368,7 @@
 
        if (txq->txq_free != ofree) {
                /* Set a watchdog timer in case the chip flakes out. */
-               ifp->if_timer = 5;
+               if_watchdog_reset(ifp, 5);
        }
 }
 
@@ -7940,7 +7940,7 @@
 
        if (sent) {
                /* Set a watchdog timer in case the chip flakes out. */
-               ifp->if_timer = 5;
+               if_watchdog_reset(ifp, 5);
        }
 }
 
@@ -8077,7 +8077,7 @@
         * timer.
         */
        if (txq->txq_sfree == WM_TXQUEUELEN(txq))
-               ifp->if_timer = 0;
+               if_watchdog_stop(ifp);
 
        return processed;
 }
diff -r 321869cf9682 -r 7d31c1c1e364 sys/net/if.c
--- a/sys/net/if.c      Wed Dec 06 08:38:33 2017 +0000
+++ b/sys/net/if.c      Wed Dec 06 09:03:12 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: if.c,v 1.404 2017/12/06 08:23:17 knakahara Exp $       */
+/*     $NetBSD: if.c,v 1.405 2017/12/06 09:03:12 ozaki-r Exp $ */
 
 /*-
  * Copyright (c) 1999, 2000, 2001, 2008 The NetBSD Foundation, Inc.
@@ -90,7 +90,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if.c,v 1.404 2017/12/06 08:23:17 knakahara Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if.c,v 1.405 2017/12/06 09:03:12 ozaki-r Exp $");
 
 #if defined(_KERNEL_OPT)
 #include "opt_inet.h"
@@ -768,9 +768,11 @@
        rt_ifannouncemsg(ifp, IFAN_ARRIVAL);
 
        if (ifp->if_slowtimo != NULL) {
+               int flags = ISSET(ifp->if_extflags, IFEF_MPSAFE) ?
+                   CALLOUT_MPSAFE : 0;
                ifp->if_slowtimo_ch =
                    kmem_zalloc(sizeof(*ifp->if_slowtimo_ch), KM_SLEEP);
-               callout_init(ifp->if_slowtimo_ch, 0);
+               callout_init(ifp->if_slowtimo_ch, flags);
                callout_setfunc(ifp->if_slowtimo_ch, if_slowtimo, ifp);
                if_slowtimo(ifp);
        }
@@ -2503,6 +2505,18 @@
 }
 
 /*
+ * XXX reusing (ifp)->if_snd->ifq_lock rather than having another spin mutex
+ * for each ifnet.  It doesn't matter because:
+ * - if IFEF_MPSAFE is enabled, if_snd isn't used and lock contention on
+ *   ifq_lock don't happen
+ * - if IFEF_MPSAFE is disabled, there is no lock contention on ifq_lock
+ *   because if_snd and if_watchdog_reset is used with KERNEL_LOCK on packet
+ *   transmissions and if_slowtimo is also called with KERNEL_LOCK
+ */
+#define IF_WATCHDOG_LOCK(ifp)  mutex_enter((ifp)->if_snd.ifq_lock)
+#define IF_WATCHDOG_UNLOCK(ifp)        mutex_exit((ifp)->if_snd.ifq_lock)
+
+/*
  * Handle interface slowtimo timer routine.  Called
  * from softclock, we decrement timer (if set) and
  * call the appropriate interface routine on expiration.
@@ -2513,21 +2527,40 @@
        void (*slowtimo)(struct ifnet *);
        struct ifnet *ifp = arg;
        int s;
+       bool fire;
 
        slowtimo = ifp->if_slowtimo;
        if (__predict_false(slowtimo == NULL))
                return;
 
        s = splnet();
-       if (ifp->if_timer != 0 && --ifp->if_timer == 0)
+       IF_WATCHDOG_LOCK(ifp);
+       fire = (ifp->if_timer != 0 && --ifp->if_timer == 0);
+       IF_WATCHDOG_UNLOCK(ifp);
+       if (fire)
                (*slowtimo)(ifp);
-
        splx(s);
 
        if (__predict_true(ifp->if_slowtimo != NULL))
                callout_schedule(ifp->if_slowtimo_ch, hz / IFNET_SLOWHZ);
 }
 
+void
+if_watchdog_reset(struct ifnet *ifp, short v)
+{
+
+       IF_WATCHDOG_LOCK(ifp);
+       ifp->if_timer = v;
+       IF_WATCHDOG_UNLOCK(ifp);
+}
+
+void
+if_watchdog_stop(struct ifnet *ifp)
+{
+
+       if_watchdog_reset(ifp, 0);
+}
+
 /*
  * Mark an interface up and notify protocols of
  * the transition.
diff -r 321869cf9682 -r 7d31c1c1e364 sys/net/if.h
--- a/sys/net/if.h      Wed Dec 06 08:38:33 2017 +0000
+++ b/sys/net/if.h      Wed Dec 06 09:03:12 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: if.h,v 1.248 2017/12/06 08:23:17 knakahara Exp $       */
+/*     $NetBSD: if.h,v 1.249 2017/12/06 09:03:12 ozaki-r Exp $ */
 
 /*-
  * Copyright (c) 1999, 2000, 2001 The NetBSD Foundation, Inc.
@@ -1047,6 +1047,9 @@
 
 void   if_input(struct ifnet *, struct mbuf *);
 
+void   if_watchdog_reset(struct ifnet *, short);
+void   if_watchdog_stop(struct ifnet *);
+
 struct if_percpuq *
        if_percpuq_create(struct ifnet *);
 void   if_percpuq_destroy(struct if_percpuq *);