Message-ID: <c0fc512a-5bee-48da-9dfb-2b8101f3dec6@tu-dortmund.de>
Date: Wed, 26 Nov 2025 17:04:25 +0100
From: Simon Schippers <simon.schippers@...dortmund.de>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: willemdebruijn.kernel@...il.com, jasowang@...hat.com,
        andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, eperezma@...hat.com,
        jon@...anix.com, tim.gebauer@...dortmund.de, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        virtualization@...ts.linux.dev
Subject: Re: [PATCH net-next v6 3/8] tun/tap: add synchronized ring
 produce/consume with queue management

On 11/26/25 16:25, Michael S. Tsirkin wrote:
> 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.

I can not judge it, but yes, it would have made this patchset way
simpler.

> 
> 
> 
> 
> 
>> 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?

tun does not but vhost-net does.

Since vhost-net also uses tun_net_xmit() as its ndo_start_xmit in a
tap+vhost-net setup, its consumer must also be changed. Else
tun_net_xmit() would stop the queue, but it would never be woken again.

> 
> 
>> 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);
>>>
>>>
>>>
>>>
>>>
>>>
>>>
> 

Powered by blists - more mailing lists