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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8265c592-a51f-4b26-9e6d-df69c16aebf4@gmail.com>
Date: Fri, 18 Apr 2025 21:38:07 +0900
From: Toshiaki Makita <toshiaki.makita1@...il.com>
To: Jesper Dangaard Brouer <hawk@...nel.org>
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 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)" ?

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.

Toshiaki Makita