Re: [PATCH net-next V6 2/2] veth: apply qdisc backpressure on full ptr_ring to reduce TX drops

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 24 Apr 2025 17:24:51 +0200 Jesper Dangaard Brouer wrote:
> > Looks like I wrote a reply to v5 but didn't hit send. But I may have
> > set v5 to Changes Requested because of it :S Here is my comment:
> > 
> >   I think this is missing a memory barrier. When drivers do this dance
> >   there's usually a barrier between stop and recheck, to make sure the
> >   stop is visible before we check. And vice versa veth_xdp_rcv() needs
> >   to make sure other side sees the "empty" indication before it checks
> >   if the queue is stopped.  
> 
> The call netif_tx_stop_queue(txq); already contains a memory barrier
> smp_mb__before_atomic() plus an atomic set_bit operation.  That should
> be sufficient.

That barrier is _before_ stopping the queue. I'm saying we need a
barrier between stop and emptiness re-check. Note that:
 - smp_mb__after_atomic() is enough, and it 'compiles' to nothing
   on x86
 - all of this is the unlikely path :) You restart the qdisc
   when the ptr ring is completely full so the stopping in absolute
   worst case will happen once or twice per full ptr_ring ?

> And the other side veth_poll(), have a smp_store_mb() before reading
> ptr_ring.
> 
> --Jesper
> 
> p.s.
> I actually had an alternative implementation of this, that only calls
> stop when it is needed.  See below, it kind of looks prettier, but it
> adds an extra memory barrier in the likely path. (And I'm not sure if 
> read memory barrier is strong enough).

Not sure that works either :S