netdev - Re: [PATCH net-next V6 2/2] veth: apply qdisc backpressure on full ptr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250424085358.75d817ae@kernel.org>
Date: Thu, 24 Apr 2025 08:53:58 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Jesper Dangaard Brouer <hawk@...nel.org>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, tom@...bertland.com, Eric
 Dumazet <eric.dumazet@...il.com>, "David S. Miller" <davem@...emloft.net>,
 Paolo Abeni <pabeni@...hat.com>, Toke Høiland-Jørgensen <toke@...e.dk>, dsahern@...nel.org,
 makita.toshiaki@....ntt.co.jp, kernel-team@...udflare.com, phil@....cc
Subject: Re: [PATCH net-next V6 2/2] veth: apply qdisc backpressure on full
 ptr_ring to reduce TX drops

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