On Wed, Nov 26, 2025 at 10:23:50AM +0100, Simon Schippers wrote:
> On 11/25/25 17:54, Michael S. Tsirkin wrote:
> > On Thu, Nov 20, 2025 at 04:29:08PM +0100, Simon Schippers wrote:
> >> Implement new ring buffer produce and consume functions for tun and tap
> >> drivers that provide lockless producer-consumer synchronization and
> >> netdev queue management to prevent ptr_ring tail drop and permanent
> >> starvation.
> >>
> >> - tun_ring_produce(): Produces packets to the ptr_ring with proper memory
> >> barriers and proactively stops the netdev queue when the ring is about
> >> to become full.
> >>
> >> - __tun_ring_consume() / __tap_ring_consume(): Internal consume functions
> >> that check if the netdev queue was stopped due to a full ring, and wake
> >> it when space becomes available. Uses memory barriers to ensure proper
> >> ordering between producer and consumer.
> >>
> >> - tun_ring_consume() / tap_ring_consume(): Wrapper functions that acquire
> >> the consumer lock before calling the internal consume functions.
> >>
> >> Key features:
> >> - Proactive queue stopping using __ptr_ring_full_next() to stop the queue
> >> before it becomes completely full.
> >> - Not stopping the queue when the ptr_ring is full already, because if
> >> the consumer empties all entries in the meantime, stopping the queue
> >> would cause permanent starvation.
> >
> > what is permanent starvation? this comment seems to answer this
> > question:
> >
> >
> > /* Do not stop the netdev queue if the ptr_ring is full already.
> > * The consumer could empty out the ptr_ring in the meantime
> > * without noticing the stopped netdev queue, resulting in a
> > * stopped netdev queue and an empty ptr_ring. In this case the
> > * netdev queue would stay stopped forever.
> > */
> >
> >
> > why having a single entry in
> > the ring we never use helpful to address this?
> >
> >
> >
> >
> > In fact, all your patch does to solve it, is check
> > netif_tx_queue_stopped on every consumed packet.
> >
> >
> > I already proposed:
> >
> > static inline int __ptr_ring_peek_producer(struct ptr_ring *r)
> > {
> > if (unlikely(!r->size) || r->queue[r->producer])
> > return -ENOSPC;
> > return 0;
> > }
> >
> > And with that, why isn't avoiding the race as simple as
> > just rechecking after stopping the queue?
>
> I think you are right and that is quite similar to what veth [1] does.
> However, there are two differences:
>
> - Your approach avoids returning NETDEV_TX_BUSY by already stopping
> when the ring becomes full (and not when the ring is full already)
> - ...and the recheck of the producer wakes on !full instead of empty.
>
> I like both aspects better than the veth implementation.
Right.
Though frankly, someone should just fix NETDEV_TX_BUSY already
at least with the most popular qdiscs.
It is a common situation and it is just annoying that every driver has
to come up with its own scheme.
> Just one thing: like the veth implementation, we probably need a
> smp_mb__after_atomic() after netif_tx_stop_queue() as they also discussed
> in their v6 [2].
yea makes sense.
>
> On the consumer side, I would then just do:
>
> __ptr_ring_consume();
> if (unlikely(__ptr_ring_consume_created_space()))
> netif_tx_wake_queue(txq);
>
> Right?
>
> And for the batched consume method, I would just call this in a loop.
Well tun does not use batched consume does it?
> Thank you!
>
> [1] Link:
> https://lore.kernel.org/netdev/174559288731.827981.8748257839971869213.stgit@firesoul/T/#m2582fcc48901e2e845b20b89e0e7196951484e5f
> [2] Link:
> https://lore.kernel.org/all/174549933665.608169.392044991754158047.stgit@firesoul/T/#m63f2deb86ffbd9ff3a27e1232077a3775606c14d
>
> >
> > __ptr_ring_produce();
> > if (__ptr_ring_peek_producer())
> > netif_tx_stop_queue
>
> smp_mb__after_atomic(); // Right here
>
> > if (!__ptr_ring_peek_producer())
> > netif_tx_wake_queue(txq);
> >
> >
> >
> >
> >
> >
> >
