From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: bse: mp-safe genet_qstart()
To: tech@openbsd.org, bluhm@openbsd.org
Date: Sun, 11 Jan 2026 13:18:47 +0300

On Tue, Jan 06, 2026 at 12:21:28PM +0300, Vitaliy Makkoveev wrote:
> On Tue, Jan 06, 2026 at 01:04:11AM +0300, Vitaliy Makkoveev wrote:
> > This diff follows the instructions from net/ifq.h The interface queues
> > layer takes care about serialization. To keep qenet_qstart() path
> > lockless I do atomic operations on sc_tx.queued shared between
> > genet_qstart() and genet_intr() to avoid mutex(9). It also rely on
> > atomic operations, but exclude parallel run of these handlers.
> > 
> > I do re-lock dances around barriers within genet_stop(). In one hand
> > they are not necessary because the netlock will not be taken in both
> > genet_qstart() and genet_intr() paths. In other hand, barriers under
> > exclusive netlock completely stop packets processing. Some mp-safe
> > drivers follow the first way, some follow the second.
> > 
> > Tested for two days on couple RPI4 under load and continuous interface
> > up/down sequence.
> > 
> 
> Al little update. Against previous I pushed sc_tx.queued decrement back
> to the loop. Previous one looks too fragile to me. If we want to
> optimise this path, is better to do subtraction after loop.
> 

bluhm@ doesn't like re-locking in the ioctl path. So this diff follows
the ixgbe(4) way and avoids re-locking. This is not the problem, because
concurrent genet_qstart() thread never acquire netlock.

Diff is stable more than 36 hours.

ok?

Index: sys/dev/ic/bcmgenet.c
===================================================================
RCS file: /cvs/src/sys/dev/ic/bcmgenet.c,v
retrieving revision 1.10
diff -u -p -r1.10 bcmgenet.c
--- sys/dev/ic/bcmgenet.c	5 Jan 2026 11:49:01 -0000	1.10
+++ sys/dev/ic/bcmgenet.c	9 Jan 2026 23:28:12 -0000
@@ -195,7 +195,7 @@ genet_setup_txdesc(struct genet_softc *s
 	uint32_t status;
 
 	status = flags | __SHIFTIN(len, GENET_TX_DESC_STATUS_BUFLEN);
-	++sc->sc_tx.queued;
+	atomic_inc_int(&sc->sc_tx.queued);
 
 	WR4(sc, GENET_TX_DESC_ADDRESS_LO(index), (uint32_t)paddr);
 	WR4(sc, GENET_TX_DESC_ADDRESS_HI(index), (uint32_t)(paddr >> 32));
@@ -234,7 +234,8 @@ genet_setup_txbuf(struct genet_softc *sc
 	segs = sc->sc_tx.buf_map[index].map->dm_segs;
 	nsegs = sc->sc_tx.buf_map[index].map->dm_nsegs;
 
-	if (sc->sc_tx.queued >= GENET_DMA_DESC_COUNT - nsegs) {
+	if (atomic_load_int(&sc->sc_tx.queued) >=
+	    GENET_DMA_DESC_COUNT - nsegs) {
 		bus_dmamap_unload(sc->sc_tx.buf_tag,
 		    sc->sc_tx.buf_map[index].map);
 		return -1;
@@ -657,6 +658,12 @@ genet_stop(struct genet_softc *sc)
 	ifp->if_flags &= ~IFF_RUNNING;
 	ifq_clr_oactive(&ifp->if_snd);
 
+	/*
+	 * XXXSMP: We holding the exclusive netlock, but it is safe.
+	 * Concurrent genet_qstart() thread could be called only from
+	 * softnet thread which does not acquire netlock. 
+	 */
+	ifq_barrier(&ifp->if_snd);
 	intr_barrier(sc->sc_ih);
 
 	/* Clean RX ring. */
@@ -749,14 +756,16 @@ genet_txintr(struct genet_softc *sc, int
 {
 	struct ifnet *ifp = &sc->sc_ac.ac_if;
 	struct genet_bufmap *bmap;
-	uint32_t cidx, total;
+	uint32_t cidx, total, queued;
 	int i;
 
 	cidx = RD4(sc, GENET_TX_DMA_CONS_INDEX(qid)) & 0xffff;
 	total = (cidx - sc->sc_tx.cidx) & 0xffff;
 
-	for (i = sc->sc_tx.next; sc->sc_tx.queued > 0 && total > 0;
-	     i = TX_NEXT(i), total--) {
+	if ((queued = atomic_load_int(&sc->sc_tx.queued)) < total)
+		total = queued;
+
+	for (i = sc->sc_tx.next; total > 0; i = TX_NEXT(i), total--) {
 		/* XXX check for errors */
 
 		bmap = &sc->sc_tx.buf_map[i];
@@ -769,7 +778,7 @@ genet_txintr(struct genet_softc *sc, int
 			bmap->mbuf = NULL;
 		}
 
-		--sc->sc_tx.queued;
+		atomic_dec_int(&sc->sc_tx.queued);
 	}
 
 	if (sc->sc_tx.cidx != cidx) {
@@ -782,13 +791,19 @@ genet_txintr(struct genet_softc *sc, int
 }
 
 void
-genet_start(struct ifnet *ifp)
+genet_qstart(struct ifqueue *ifq)
 {
+	struct ifnet *ifp = ifq->ifq_if;
 	struct genet_softc *sc = ifp->if_softc;
 	struct mbuf *m;
 	const int qid = GENET_DMA_DEFAULT_QUEUE;
 	int nsegs, index, cnt;
 
+	if (!LINK_STATE_IS_UP(ifp->if_link_state)) {
+		ifq_purge(ifq);
+		return;
+	}
+
 	index = sc->sc_tx.pidx & (TX_DESC_COUNT - 1);
 	cnt = 0;
 
@@ -971,7 +986,8 @@ genet_attach(struct genet_softc *sc)
 	ifp->if_softc = sc;
 	snprintf(ifp->if_xname, IFNAMSIZ, "%s", sc->sc_dev.dv_xname);
 	ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
-	ifp->if_start = genet_start;
+	ifp->if_xflags = IFXF_MPSAFE;
+	ifp->if_qstart = genet_qstart;
 	ifp->if_ioctl = genet_ioctl;
 	ifq_init_maxlen(&ifp->if_snd, IFQ_MAXLEN);