Re: CVS commit: src/sys

To: Nick Hudson <skrll%NetBSD.org@localhost>
Subject: Re: CVS commit: src/sys
From: Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>
Date: Sat, 10 Aug 2024 21:37:45 +0000

> Module Name:    src
> Committed By:   skrll
> Date:           Sat Aug 10 12:16:47 UTC 2024
> 
> Modified Files:
>         src/sys/arch/arm/altera: cycv_gmac.c
>         src/sys/arch/arm/amlogic: meson_dwmac.c
>         src/sys/arch/arm/rockchip: rk_gmac.c
>         src/sys/arch/arm/sunxi: sunxi_gmac.c
>         src/sys/dev/ic: dwc_gmac.c dwc_gmac_var.h
> 
> Log Message:
> awge(4): MP improvements
> 
> Remove the non-MP-safe scaffolding and make the locking less
> coarse.

It's great to see more MP-safety and removal of conditional-NET_MPSAFE
scaffolding -- but please, no more all-encompassing `core locks'!

The usage model for a network interface is:

1. (under IFNET_LOCK) if_init:
   (a) resets hardware, while nothing else is touching registers
   (b) programs multicast filter
   (c) starts Tx/Rx and tick for mii/watchdog
   (d) sets IFF_RUNNING

2. concurrently:
   - (mii lock) periodic mii ticks or (with IFNET_LOCK too) ifmedia ioctls
   - (tx lock) Tx submissions
   - (rx lock) Rx notifications
   - (multicast filter lock) SIOCADDMULTI/SIOCDELMULTI
   - (ic lock) 802.11 state machine notifications
   - (IFNET_LOCK) maybe other ioctls (some of which return ENETRESET
     to cause an if_stop/init cycle in thread context)

3. (under IFNET_LOCK) if_stop:
   (a) clears IFF_RUNNING
   (b) halts Tx/Rx/tick and waits for completion
   (c) disables concurrent multicast updates
   (d) calls mii_down to wait for concurrent mii activity to quiesce
   (e) resets hardware, now that nothing is touching registers any more

All of the steps in (1) if_init, and all of the steps in (3) if_stop,
are already serialized by IFNET_LOCK, the heavyweight configuration
lock, in low-priority thread context.  Sometimes, while (2) running,
ifconfig(8) will reconfigure things, also serialized by IFNET_LOCK.

So there's _no need_ for a `core lock' to cover everything in if_init
and if_stop: IFNET_LOCK already takes care of that.

Any high-priority activity like Tx/Rx paths, callouts, &c., MUST NOT
take IFNET_LOCK or wait for the completion of the heavyweight
configuration changes, which often have to wait for concurrent
high-priority activity to cease -- such waits lead to deadlocks like
<https://mail-index.netbsd.org/tech-net/2022/01/10/msg008170.html>.

So it's _harmful_ to have a `core lock' cover everything in if_init
and if_stop.

In usbnet(9), I broke the deadlock by eliminating the core lock and
replacing it by a narrowly scoped lock covering only the state needed
by multicast filter updates, which happen from high-priority contexts.
I suggest doing the same here (and in bge(4) and in vmx(4) and
wherever else we have an overbroad `core lock'), with the attached
patch.  But I don't have any awge(4) hardware to test.  Could I
trouble you to test this and commit it if it works?

diff -r f339ab4bf462 sys/dev/ic/dwc_gmac.c
--- a/sys/dev/ic/dwc_gmac.c	Sat Aug 10 15:20:59 2024 +0000
+++ b/sys/dev/ic/dwc_gmac.c	Sat Aug 10 21:19:37 2024 +0000
@@ -87,9 +87,7 @@ static void dwc_gmac_reset_tx_ring(struc
 static void dwc_gmac_free_tx_ring(struct dwc_gmac_softc *, struct dwc_gmac_tx_ring *);
 static void dwc_gmac_txdesc_sync(struct dwc_gmac_softc *, int, int, int);
 static int dwc_gmac_init(struct ifnet *);
-static int dwc_gmac_init_locked(struct ifnet *);
 static void dwc_gmac_stop(struct ifnet *, int);
-static void dwc_gmac_stop_locked(struct ifnet *, int);
 static void dwc_gmac_start(struct ifnet *);
 static void dwc_gmac_start_locked(struct ifnet *);
 static int dwc_gmac_queue(struct dwc_gmac_softc *, struct mbuf *);
@@ -297,7 +295,7 @@ dwc_gmac_attach(struct dwc_gmac_softc *s
 	sc->sc_stopping = false;
 	sc->sc_txbusy = false;
 
-	sc->sc_core_lock = mutex_obj_alloc(MUTEX_DEFAULT, IPL_NONE);
+	sc->sc_mcast_lock = mutex_obj_alloc(MUTEX_DEFAULT, IPL_SOFTNET);
 	sc->sc_intr_lock = mutex_obj_alloc(MUTEX_DEFAULT, IPL_NET);
 	mutex_init(&sc->sc_txq.t_mtx, MUTEX_DEFAULT, IPL_NET);
 	mutex_init(&sc->sc_rxq.r_mtx, MUTEX_DEFAULT, IPL_NET);
@@ -863,28 +861,13 @@ static int
 dwc_gmac_init(struct ifnet *ifp)
 {
 	struct dwc_gmac_softc * const sc = ifp->if_softc;
-
-	KASSERT(IFNET_LOCKED(ifp));
-
-	mutex_enter(sc->sc_core_lock);
-	int ret = dwc_gmac_init_locked(ifp);
-	mutex_exit(sc->sc_core_lock);
-
-	return ret;
-}
-
-static int
-dwc_gmac_init_locked(struct ifnet *ifp)
-{
-	struct dwc_gmac_softc * const sc = ifp->if_softc;
 	uint32_t ffilt;
 
 	ASSERT_SLEEPABLE();
 	KASSERT(IFNET_LOCKED(ifp));
-	KASSERT(mutex_owned(sc->sc_core_lock));
 	KASSERT(ifp == &sc->sc_ec.ec_if);
 
-	dwc_gmac_stop_locked(ifp, 0);
+	dwc_gmac_stop(ifp, 0);
 
 	/*
 	 * Configure DMA burst/transfer mode and RX/TX priorities.
@@ -942,11 +925,11 @@ dwc_gmac_init_locked(struct ifnet *ifp)
 
 	mutex_enter(sc->sc_intr_lock);
 	sc->sc_stopping = false;
+	mutex_exit(sc->sc_intr_lock);
 
 	mutex_enter(&sc->sc_txq.t_mtx);
 	sc->sc_txbusy = false;
 	mutex_exit(&sc->sc_txq.t_mtx);
-	mutex_exit(sc->sc_intr_lock);
 
 	return 0;
 }
@@ -1015,29 +998,23 @@ dwc_gmac_stop(struct ifnet *ifp, int dis
 {
 	struct dwc_gmac_softc * const sc = ifp->if_softc;
 
-	mutex_enter(sc->sc_core_lock);
-	dwc_gmac_stop_locked(ifp, disable);
-	mutex_exit(sc->sc_core_lock);
-}
-
-static void
-dwc_gmac_stop_locked(struct ifnet *ifp, int disable)
-{
-	struct dwc_gmac_softc * const sc = ifp->if_softc;
-
 	ASSERT_SLEEPABLE();
 	KASSERT(IFNET_LOCKED(ifp));
-	KASSERT(mutex_owned(sc->sc_core_lock));
+
+	ifp->if_flags &= ~IFF_RUNNING;
+
+	mutex_enter(sc->sc_mcast_lock);
+	sc->sc_if_flags = ifp->if_flags;
+	mutex_exit(sc->sc_mcast_lock);
 
 	mutex_enter(sc->sc_intr_lock);
 	sc->sc_stopping = true;
+	mutex_exit(sc->sc_intr_lock);
 
 	mutex_enter(&sc->sc_txq.t_mtx);
 	sc->sc_txbusy = false;
 	mutex_exit(&sc->sc_txq.t_mtx);
 
-	mutex_exit(sc->sc_intr_lock);
-
 	bus_space_write_4(sc->sc_bst, sc->sc_bsh,
 	    AWIN_GMAC_DMA_OPMODE,
 	    bus_space_read_4(sc->sc_bst, sc->sc_bsh,
@@ -1051,9 +1028,6 @@ dwc_gmac_stop_locked(struct ifnet *ifp, 
 	mii_down(&sc->sc_mii);
 	dwc_gmac_reset_tx_ring(sc, &sc->sc_txq);
 	dwc_gmac_reset_rx_ring(sc, &sc->sc_rxq);
-
-	ifp->if_flags &= ~IFF_RUNNING;
-	sc->sc_if_flags = ifp->if_flags;
 }
 
 /*
@@ -1150,7 +1124,7 @@ dwc_gmac_ifflags_cb(struct ethercom *ec)
 	int ret = 0;
 
 	KASSERT(IFNET_LOCKED(ifp));
-	mutex_enter(sc->sc_core_lock);
+	mutex_enter(sc->sc_mcast_lock);
 
 	u_short change = ifp->if_flags ^ sc->sc_if_flags;
 	sc->sc_if_flags = ifp->if_flags;
@@ -1161,7 +1135,7 @@ dwc_gmac_ifflags_cb(struct ethercom *ec)
 		dwc_gmac_setmulti(sc);
 	}
 
-	mutex_exit(sc->sc_core_lock);
+	mutex_exit(sc->sc_mcast_lock);
 
 	return ret;
 }
@@ -1187,7 +1161,7 @@ dwc_gmac_ioctl(struct ifnet *ifp, u_long
 	if (error == ENETRESET) {
 		error = 0;
 		if (cmd == SIOCADDMULTI || cmd == SIOCDELMULTI) {
-			mutex_enter(sc->sc_core_lock);
+			mutex_enter(sc->sc_mcast_lock);
 			if (sc->sc_if_flags & IFF_RUNNING) {
 				/*
 				 * Multicast list has changed; set the hardware
@@ -1195,7 +1169,7 @@ dwc_gmac_ioctl(struct ifnet *ifp, u_long
 				 */
 				dwc_gmac_setmulti(sc);
 			}
-			mutex_exit(sc->sc_core_lock);
+			mutex_exit(sc->sc_mcast_lock);
 		}
 	}
 
@@ -1393,7 +1367,6 @@ skip:
 static void
 dwc_gmac_setmulti(struct dwc_gmac_softc *sc)
 {
-	struct ifnet * const ifp = &sc->sc_ec.ec_if;
 	struct ether_multi *enm;
 	struct ether_multistep step;
 	struct ethercom *ec = &sc->sc_ec;
@@ -1401,7 +1374,7 @@ dwc_gmac_setmulti(struct dwc_gmac_softc 
 	uint32_t ffilt, h;
 	int mcnt;
 
-	KASSERT(mutex_owned(sc->sc_core_lock));
+	KASSERT(mutex_owned(sc->sc_mcast_lock));
 
 	ffilt = bus_space_read_4(sc->sc_bst, sc->sc_bsh, AWIN_GMAC_MAC_FFILT);
 
@@ -1463,7 +1436,6 @@ special_filter:
 	    0xffffffff);
 	bus_space_write_4(sc->sc_bst, sc->sc_bsh, AWIN_GMAC_MAC_HTHIGH,
 	    0xffffffff);
-	sc->sc_if_flags = ifp->if_flags;
 }
 
 int
diff -r f339ab4bf462 sys/dev/ic/dwc_gmac_var.h
--- a/sys/dev/ic/dwc_gmac_var.h	Sat Aug 10 15:20:59 2024 +0000
+++ b/sys/dev/ic/dwc_gmac_var.h	Sat Aug 10 21:19:37 2024 +0000
@@ -102,12 +102,12 @@ struct dwc_gmac_softc {
 	struct dwc_gmac_rx_ring sc_rxq;
 	struct dwc_gmac_tx_ring sc_txq;
 	const struct dwc_gmac_desc_methods *sc_descm;
-	u_short sc_if_flags;			/* shadow of ether flags */
+	u_short sc_if_flags;			/* if_flags copy (for mcast) */
 	uint16_t sc_mii_clk;
 	bool sc_txbusy;
 	bool sc_stopping;
 	krndsource_t rnd_source;
-	kmutex_t *sc_core_lock;			/* lock for softc operations */
+	kmutex_t *sc_mcast_lock;		/* lock for SIOCADD/DELMULTI */
 	kmutex_t *sc_intr_lock;			/* lock for interrupt operations */
 
 	struct if_percpuq *sc_ipq;		/* softint-based input queues */

Follow-Ups:
- Re: CVS commit: src/sys
  - From: Nick Hudson
- Re: CVS commit: src/sys
  - From: Taylor R Campbell

Prev by Date: Re: CVS commit: src/share/man/man5
Next by Date: Re: CVS commit: src/sys
Previous by Thread: Re: CVS commit: src/share/man/man5
Next by Thread: Re: CVS commit: src/sys
Indexes:

Home | Main Index | Thread Index | Old Index