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
