On Thu, Jan 15, 2026 at 11:53:36AM +1000, Jonathan Matthew wrote:
> On Wed, Jan 14, 2026 at 11:01:49PM +0000, Vitaliy Makkoveev wrote:
> > On Tue, Jan 13, 2026 at 09:20:24AM +0000, Vitaliy Makkoveev wrote:
> > > On Mon, Jan 12, 2026 at 11:40:47AM +1000, Jonathan Matthew wrote:
> > > > 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.
> > > > 
> > > 
> > > Thanks for feedback. The diff was reworked to follow this way.
> > > 
> > 
> > No issues for more than two days.
> 
> I don't have hardware to test this, but it looks good to me.
> One suggestion for a future change below, but ok jmatthew@ as is.
>

Thanks!

> 
> Instead of checking if there's space on the ring here, after loading
> the mbuf into a DMA map, what we typically do is check if there's
> space for the largest possible packet in *_start before dequeuing,
> so we don't have to then roll back if the packet we got won't fit.
> The big comment towards the top of src/sys/net/ifq.h shows what
> this should look like.
> 
> In this driver, the largest possible packet is apparently 128
> segments, and the ring only has 256 slots, so this would leave half
> the ring unused. 128 segments is 'a lot' for a gigabit interface
> that only supports 1500 byte packets, so maybe reducing this to 8 or
> so would be better? I think we only get up to 30 segments on 10GbE+
> nics with TSO, so we won't get anywhere near that here.
> 
> 

Will do with the next diff.