Re: [PATCH net 1/4] Revert "tun/tap & vhost-net: avoid ptr_ring tail-drop when a qdisc is present"

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incomplete Revert / Orphaned Memory Barrier
--

--- Patch [1]: [PATCH net 1/4] Revert "tun/tap & vhost-net: avoid ptr_ring tail-drop when a qdisc is present" ---
commit 18e68b71511b6b4155def5921c5f149c1b6e74e9
Author: Simon Schippers <simon.schippers@tu-dortmund.de>

Revert "tun/tap & vhost-net: avoid ptr_ring tail-drop when a qdisc is present"

This commit reverts a previous change that stopped the netdev queue when the
ptr_ring is full. It goes back to dropping packets instead, resolving a reported
performance regression in a multicast testcase.

Link: https://lore.kernel.org/netdev/akVnoOYQOrt8k-Gu@karahi.librecast.net/

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index ffbe6f13fb1fa..ec5573f545af6 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
[ ... ]
> @@ -1073,33 +1072,13 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	nf_reset_ct(skb);
>  
> -	queue = netdev_get_tx_queue(dev, txq);
> -
> -	spin_lock(&tfile->tx_ring.producer_lock);
> -	ret = __ptr_ring_produce(&tfile->tx_ring, skb);
> -	if (!qdisc_txq_has_no_queue(queue) &&
> -	    __ptr_ring_check_produce(&tfile->tx_ring) == -ENOSPC) {
> -		netif_tx_stop_queue(queue);
> -		/* Paired with smp_mb() in __tun_wake_queue() */
> -		smp_mb__after_atomic();

[Severity: Medium]
Does this revert leave an orphaned memory barrier in __tun_wake_queue()?

Since the producer-side queue stopping logic and smp_mb__after_atomic() were
removed here in tun_net_xmit(), the consumer-side logic in __tun_wake_queue()
now executes an unconditional full memory barrier on every consumed packet:

drivers/net/tun.c:__tun_wake_queue() {
    ...
    /* Paired with smp_mb__after_atomic() in tun_net_xmit() */
    smp_mb();
    if (netif_tx_queue_stopped(txq)) {
    ...
}

Should the corresponding wakeup logic and smp_mb() in __tun_wake_queue() also
be removed to complete the revert and avoid unnecessary overhead?

> -		if (!__ptr_ring_check_produce(&tfile->tx_ring))
> -			netif_tx_wake_queue(queue);
> -	}
> -	spin_unlock(&tfile->tx_ring.producer_lock);
> -
> -	if (ret) {
> -		/* This should be a rare case if a qdisc is present, but
> -		 * can happen due to lltx.
> -		 * Since skb_tx_timestamp(), skb_orphan(),
> -		 * run_ebpf_filter() and pskb_trim() could have tinkered
> -		 * with the SKB, returning NETDEV_TX_BUSY is unsafe and
> -		 * we must drop instead.
> -		 */
> +	if (ptr_ring_produce(&tfile->tx_ring, skb)) {
>  		drop_reason = SKB_DROP_REASON_FULL_RING;
>  		goto drop;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728092240.250257-1-simon.schippers@tu-dortmund.de?part=1