Re: [PATCH 2/4] netdev queue flow control for TUN

[Simon Schippers]

Michael S. Tsirkin wrote:
> On Tue, Sep 02, 2025 at 10:09:55AM +0200, Simon Schippers wrote:
>> The netdev queue is stopped in tun_net_xmit after inserting an SKB into
>> the ring buffer if the ring buffer became full because of that. If the
>> insertion into the ptr_ring fails, the netdev queue is also stopped and
>> the SKB is dropped. However, this never happened in my testing. To ensure
>> that the ptr_ring change is available to the consumer before the netdev
>> queue stop, an smp_wmb() is used.
>>
>> Then in tun_ring_recv, the new helper wake_netdev_queue is called in the
>> blocking wait queue and after consuming an SKB from the ptr_ring. This
>> helper first checks if the netdev queue has stopped. Then with the paired
>> smp_rmb() it is known that tun_net_xmit will not produce SKBs anymore.
>> With that knowledge, the helper can then wake the netdev queue if there is
>> at least a single spare slot in the ptr_ring by calling ptr_ring_spare
>> with cnt=1.
>>
>> Co-developed-by: Tim Gebauer <tim.gebauer@xxxxxxxxxxxxxx>
>> Signed-off-by: Tim Gebauer <tim.gebauer@xxxxxxxxxxxxxx>
>> Signed-off-by: Simon Schippers <simon.schippers@xxxxxxxxxxxxxx>
> 
> 
> Oh you just want to know if produce will succeed?
> Kind of a version of peek but for producer?
> 
> So all this cuteness of looking at the consumer is actually not necessary,
> and bad for cache.
> 
> You just want this:
> 
> 
> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> 
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index 551329220e4f..de25fe81dd4e 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -96,6 +96,14 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r)
>  	return ret;
>  }
>  
> +static inline int __ptr_ring_produce_peek(struct ptr_ring *r)
> +{
> +	if (unlikely(!r->size) || r->queue[r->producer])
> +		return -ENOSPC;
> +
> +	return 0;
> +}
> +
>  /* Note: callers invoking this in a loop must use a compiler barrier,
>   * for example cpu_relax(). Callers must hold producer_lock.
>   * Callers are responsible for making sure pointer that is being queued
> @@ -103,8 +111,10 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r)
>   */
>  static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr)
>  {
> -	if (unlikely(!r->size) || r->queue[r->producer])
> -		return -ENOSPC;
> +	int r = __ptr_ring_produce_peek(r);
> +
> +	if (r)
> +		return r;
>  
>  	/* Make sure the pointer we are storing points to a valid data. */
>  	/* Pairs with the dependency ordering in __ptr_ring_consume. */
> 
> 
> 
> Add some docs, and call this, then wake.  No?
>

Yes, this looks great! I like that it does not need any further logic :)
I will just call this method instead of my approach in wake_netdev_queue
without taking any locks. It should be just fine since at this moment it
is known that the producer stopped due to the stopped netdev queue.