From: Vitaliy Makkoveev Subject: Re: bse: mp-safe genet_qstart() To: tech@openbsd.org, kettenis@openbsd.org Date: Tue, 6 Jan 2026 12:21:28 +0300 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. 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 6 Jan 2026 09:04:00 -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,8 +658,13 @@ genet_stop(struct genet_softc *sc) ifp->if_flags &= ~IFF_RUNNING; ifq_clr_oactive(&ifp->if_snd); + NET_UNLOCK(); + + ifq_barrier(&ifp->if_snd); intr_barrier(sc->sc_ih); + NET_LOCK(); + /* Clean RX ring. */ for (i = 0; i < RX_DESC_COUNT; i++) { bmap = &sc->sc_rx.buf_map[i]; @@ -749,14 +755,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 +777,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 +790,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; @@ -860,6 +874,9 @@ genet_ioctl(struct ifnet *ifp, u_long cm ifp->if_flags |= IFF_UP; /* FALLTHROUGH */ case SIOCSIFFLAGS: + NET_UNLOCK(); + rw_enter_write(&sc->sc_lock); + NET_LOCK(); if (ifp->if_flags & IFF_UP) { if (ifp->if_flags & IFF_RUNNING) error = ENETRESET; @@ -869,6 +886,7 @@ genet_ioctl(struct ifnet *ifp, u_long cm if (ifp->if_flags & IFF_RUNNING) genet_stop(sc); } + rw_exit_write(&sc->sc_lock); break; case SIOCGIFMEDIA: @@ -964,6 +982,8 @@ genet_attach(struct genet_softc *sc) return EINVAL; } + rw_init(&sc->sc_lock, "genetlk"); + timeout_set(&sc->sc_stat_ch, genet_tick, sc); timeout_set(&sc->sc_rxto, genet_rxtick, sc); @@ -971,7 +991,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); Index: sys/dev/ic/bcmgenetvar.h =================================================================== RCS file: /cvs/src/sys/dev/ic/bcmgenetvar.h,v retrieving revision 1.1 diff -u -p -r1.1 bcmgenetvar.h --- sys/dev/ic/bcmgenetvar.h 14 Apr 2020 21:02:39 -0000 1.1 +++ sys/dev/ic/bcmgenetvar.h 6 Jan 2026 09:04:00 -0000 @@ -69,6 +69,7 @@ struct genet_softc { #define sc_lladdr sc_ac.ac_enaddr struct mii_data sc_mii; struct timeout sc_stat_ch; + struct rwlock sc_lock; struct genet_ring sc_tx; struct genet_ring sc_rx;