From: Jonathan Matthew Subject: Re: bse: mp-safe genet_qstart() To: Vitaliy Makkoveev Cc: tech@openbsd.org, bluhm@openbsd.org Date: Mon, 12 Jan 2026 11:40:47 +1000 On Sun, Jan 11, 2026 at 01:18:47PM +0300, Vitaliy Makkoveev wrote: > 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? In most mpsafe drivers we avoid using atomic operations by calculating the number of free tx slots from the producer and consumer indexes, where the producer is only updated by qstart and the consumer is only updated by the interrupt handler, rather than having a field storing the difference between them that has to be updated in both. It looks like that approach should work here too, using the cidx and pidx fields in sc->sc_tx. > > 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); I don't think this comment is necessary. We do the same thing in many other drivers. > 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); > >