Index | Thread | Search

From:
Jonathan Matthew <jonathan@d14n.org>
Subject:
Re: bse: do the TX ring space check before packet dequeue
To:
Vitaliy Makkoveev <mvs@openbsd.org>
Cc:
tech@openbsd.org
Date:
Thu, 22 Jan 2026 08:12:31 +1000

Download raw body.

Thread
On Wed, Jan 21, 2026 at 12:30:00PM +0000, Vitaliy Makkoveev wrote:
> On Sat, Jan 17, 2026 at 07:10:41PM +0000, Vitaliy Makkoveev wrote:
> > Subj. jmatthew@ pointed this while doing review of my previous bse(4)
> > diff. This early check removes possible bus_dmamap_unload() and dequeue
> > rollback in the output path.
> > 
> > Also jmatthew@  pointed that the maximum TX segments count of 128 is too
> > big and proposed to limit limit it with 8. I checked others and found,
> > that NetBSD uses 128 as we do and FreeBSD uses 20, so I used 20. May be
> > this value is also too big, but it could be decreased at any moment.
> > 
> > I'm running this diff without any issue more than 24 hours.
> > 
> 
> No issues after 5 days.

Looks good to me.
ok jmatthew@

> 
> > Index: sys/dev/ic/bcmgenet.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/ic/bcmgenet.c,v
> > retrieving revision 1.11
> > diff -u -p -r1.11 bcmgenet.c
> > --- sys/dev/ic/bcmgenet.c	15 Jan 2026 03:12:49 -0000	1.11
> > +++ sys/dev/ic/bcmgenet.c	17 Jan 2026 13:18:42 -0000
> > @@ -70,7 +70,7 @@ CTASSERT(MCLBYTES == 2048);
> >  #define	TX_NEXT(n)		TX_SKIP(n, 1)
> >  #define	RX_NEXT(n)		(((n) + 1) & (GENET_DMA_DESC_COUNT - 1))
> >  
> > -#define	TX_MAX_SEGS		128
> > +#define	TX_MAX_SEGS		20
> >  #define	TX_DESC_COUNT		GENET_DMA_DESC_COUNT
> >  #define	RX_DESC_COUNT		GENET_DMA_DESC_COUNT
> >  #define	MII_BUSY_RETRY		1000
> > @@ -205,7 +205,7 @@ int
> >  genet_setup_txbuf(struct genet_softc *sc, int index, struct mbuf *m)
> >  {
> >  	bus_dma_segment_t *segs;
> > -	int error, queued, nsegs, cur, i;
> > +	int error, nsegs, cur, i;
> >  	uint32_t flags;
> >  
> >  	/*
> > @@ -232,13 +232,6 @@ 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;
> > -	queued = (sc->sc_tx.pidx - sc->sc_tx.cidx) & 0xffff;
> > -
> > -	if (queued >= GENET_DMA_DESC_COUNT - nsegs) {
> > -		bus_dmamap_unload(sc->sc_tx.buf_tag,
> > -		    sc->sc_tx.buf_map[index].map);
> > -		return -1;
> > -	}
> >  
> >  	flags = GENET_TX_DESC_STATUS_SOP |
> >  		GENET_TX_DESC_STATUS_CRC |
> > @@ -784,29 +777,27 @@ genet_qstart(struct ifqueue *ifq)
> >  	struct genet_softc *sc = ifp->if_softc;
> >  	struct mbuf *m;
> >  	const int qid = GENET_DMA_DEFAULT_QUEUE;
> > -	int nsegs, index, cnt;
> > +	int queued, nsegs, index, cnt;
> >  
> >  	index = sc->sc_tx.pidx & (TX_DESC_COUNT - 1);
> >  	cnt = 0;
> >  
> >  	for (;;) {
> > -		m = ifq_deq_begin(&ifp->if_snd);
> > +		queued = (sc->sc_tx.pidx - sc->sc_tx.cidx) & 0xffff;
> > +		if (queued >= TX_DESC_COUNT - TX_MAX_SEGS)
> > +			break;
> > +
> > +		m = ifq_dequeue(&ifp->if_snd);
> >  		if (m == NULL)
> >  			break;
> >  
> >  		nsegs = genet_setup_txbuf(sc, index, m);
> > -		if (nsegs == -1) {
> > -			ifq_deq_rollback(&ifp->if_snd, m);
> > -			ifq_set_oactive(&ifp->if_snd);
> > -			break;
> > -		}
> >  		if (nsegs == 0) {
> > -			ifq_deq_commit(&ifp->if_snd, m);
> >  			m_freem(m);
> >  			ifp->if_oerrors++;
> >  			continue;
> >  		}
> > -		ifq_deq_commit(&ifp->if_snd, m);
> > +
> >  		if (ifp->if_bpf)
> >  			bpf_mtap(ifp->if_bpf, m, BPF_DIRECTION_OUT);
> >