Re: [PATCH RFC net-next v4 06/14] xsk: support dynamic xmit.more control for batch xmit

On Wed, Apr 15, 2026 at 5:35 PM <[email protected]> wrote:
>
> Sashiko AI review found 1 potential issue(s):
> - [High] Stalled TX queue due to missing hardware doorbell ring on early loop exit
> --
>
> commit 6d7c8f2ad056eb4cb2eec8d360e632f9ef9044f5
> Author: Jason Xing <[email protected]>
>
> xsk: support dynamic xmit.more control for batch xmit
>
> This commit updates xsk batch transmission to only set xmit.more to false for
> the last skb in the send queue, aiming to reduce unnecessary IRQs and improve
> batching performance.
>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -4873,6 +4873,7 @@ int xsk_direct_xmit_batch(struct xdp_sock *xs, struct net_device *dev)
> >       struct sk_buff_head *send_queue = &xs->batch.send_queue;
> >       int ret = NETDEV_TX_BUSY;
> >       struct sk_buff *skb;
> > +     bool more = true;
> >
> >       local_bh_disable();
> >       HARD_TX_LOCK(dev, txq, smp_processor_id());
>
> [ ... ]
>
> > @@ -4892,8 +4893,12 @@ int xsk_direct_xmit_batch(struct xdp_sock *xs, struct net_device *dev)
> >                       __skb_queue_head(send_queue, skb);
> >                       break;
> >               }
> > +
> > +             if (!skb_peek(send_queue))
> > +                     more = false;
> > +
> >               skb_set_queue_mapping(skb, queue_id);
> > -             ret = netdev_start_xmit(skb, dev, txq, false);
> > +             ret = netdev_start_xmit(skb, dev, txq, more);
> >               if (ret != NETDEV_TX_OK) {
> >                       if (ret == NETDEV_TX_BUSY)
> >                               __skb_queue_head(send_queue, skb);
> >                       break;
> >               }
> >       }
>
> If an error occurs and the loop breaks early, does this leave previously
> transmitted packets stalled in the driver's TX ring?
>
> Since more is passed as true for all packets prior to the failure, the
> network driver defers ringing the hardware TX doorbell.
>
> If the code breaks out of the loop due to validate_xmit_skb_list() failing,
> netif_xmit_frozen_or_drv_stopped(), or netdev_start_xmit() returning
> NETDEV_TX_BUSY, the loop terminates without ever passing more = false to the
> driver.
>
> Could this omission prevent the hardware from being notified to transmit
> the successfully processed packets that preceded the error?

The quite similar code snippet was added in dev_hard_start_xmit()
introduced by the following commit in 2014:
commit 95f6b3dda2a4a052f7dabe9998e4ffac491b7bc2
Author: David S. Miller <[email protected]>
Date:   Fri Aug 29 21:57:30 2014 -0700

    net: Have xmit_list() signal more==true when appropriate.

    Signed-off-by: David S. Miller <[email protected]>

Look at the doorbell of drivers, like ixgbe:
if (netif_xmit_stopped(txring_txq(tx_ring)) || !netdev_xmit_more()) {
        writel(i, tx_ring->tail);
}

It has already taken care of it.

If we really want to cover all the possible drivers in the above two
cases, maybe we need a separate patchset to add a common interface of
devices to actively kick the hardware. But now I choose to defer it
unless someone insists on this.

Thanks,
Jason

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6