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
