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.geba...@tu-dortmund.de>
>> Signed-off-by: Tim Gebauer <tim.geba...@tu-dortmund.de>
>> Signed-off-by: Simon Schippers <simon.schipp...@tu-dortmund.de>
>
>
> 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 <m...@redhat.com>
>
> 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.
