Re: [PATCH net-next V5 2/2] veth: apply qdisc backpressure on full ptr_ring to reduce TX drops

[Jesper Dangaard Brouer <hawk@kernel.org>]

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