netdev - Re: [PATCH net-next V5 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <e49b96fa-6b2c-4722-adcc-7639f1a9a66a@kernel.org>
Date: Fri, 18 Apr 2025 22:08:05 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Toshiaki Makita <toshiaki.makita1@...il.com>
Cc: 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, netdev@...r.kernel.org,
 Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next V5 2/2] veth: apply qdisc backpressure on full
 ptr_ring to reduce TX drops

On 18/04/2025 14.38, Toshiaki Makita wrote:
> On 2025/04/17 22:55, Jesper Dangaard Brouer wrote:
> ...
>> +    case NETDEV_TX_BUSY:
>> +        /* If a qdisc is attached to our virtual device, returning
>> +         * NETDEV_TX_BUSY is allowed.
>> +         */
>> +        txq = netdev_get_tx_queue(dev, rxq);
>> +
>> +        if (qdisc_txq_has_no_queue(txq)) {
>> +            dev_kfree_skb_any(skb);
>> +            goto drop;
>> +        }
>> +        netif_tx_stop_queue(txq);
>> +        /* Restore Eth hdr pulled by dev_forward_skb/eth_type_trans */
>> +        __skb_push(skb, ETH_HLEN);
>> +        if (use_napi)
>> +            __veth_xdp_flush(rq);
>> +        /* Cancel TXQ stop for very unlikely race */
>> +        if (unlikely(__ptr_ring_empty(&rq->xdp_ring)))
>> +            netif_tx_wake_queue(txq);
> 
> xdp_ring is only initialized when use_napi is not NULL.
> Should add "if (use_napi)" ?
> 

We actually don't need the "if (use_napi)" check, because this code path
cannot be invoked without use_name set.  This also means the check
before __veth_xdp_flush() is unnecessary.  I still added it, because it
is subtle that this isn't needed and if code change slightly is will be
needed.

Regarding xdp_ring is only initialized when use_napi is not NULL, I'm
considering not adding a if(use_napi) check, because this code path
cannot be called without use_napi is true, and if that change in the
future, then it's better that the code crash.  Different opinions are
welcomed...

> BTW, you added a check for the ring_empty here. so
> 
> if empty:
>    this function starts the queue by itself
> else:
>    it is guaranteed that veth_xdp_rcv() consumes the ring after this point.
>    so the rcv side definitely starts the queue.
> 
> With that, __veth_xdp_flush invocation seems to be unnecessary,
> if your concern is starting the queue.

That is actually correct. I'm trying to catch the race in two different
ways. The __ptr_ring_empty() will be sufficient, to cover both cases.
I'll try to think of a good comment that explains, the parring with the
!__ptr_ring_empty() check in veth_poll().

--Jesper