Re: [PATCH RFC net-next v4 05/14] xsk: add direct xmit in batch function

On Wed, Apr 15, 2026 at 5:12 PM <[email protected]> wrote:
>
> Sashiko AI review found 4 potential issue(s):
> - [High] Packet validation executed inside hardware transmit lock, causing severe lock contention
> - [High] Missing transmit recursion protection introduces deadlock vectors
> - [High] Missing device state validation allows transmission to stopped or suspended devices
> - [Medium] Hardware doorbell batching is defeated by hardcoded 'more' parameter
> --
>
> commit b98984346f64d24e3ecd521b219464ccbe9fba91
> Author: Jason Xing <[email protected]>
>
> xsk: add direct xmit in batch function
>
> This commit adds a batch transmission logic for AF_XDP. It introduces a
> new direct transmit function that disables bottom halves, grabs the lock
> once, and sends all aggregated packets in a loop.
>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
>
> [ ... ]
>
> > +int xsk_direct_xmit_batch(struct xdp_sock *xs, struct net_device *dev)
> > +{
> > +     u16 queue_id = xs->queue_id;
> > +     struct netdev_queue *txq = netdev_get_tx_queue(dev, queue_id);
> > +     struct sk_buff_head *send_queue = &xs->batch.send_queue;
> > +     int ret = NETDEV_TX_BUSY;
> > +     struct sk_buff *skb;
> > +
>
> Should there be a check for the device state before attempting to transmit?
> Direct transmit paths that bypass the Qdisc layer, like __dev_direct_xmit(),
> typically verify that the device is running and the carrier is up:
>
>         if (unlikely(!netif_running(dev) ||
>                      !netif_carrier_ok(dev)))
>                 goto drop;

Actually I did it at the very beginning of the sendmsg function, but
with the respect to the original code I will move it.

>
> Could invoking transmission on a stopped or suspended device cause issues?
>
> > +     local_bh_disable();
>
> Are we missing transmit recursion protection here? Standard paths wrap the
> hardware transmit lock with recursion tracking to protect against stack
> overflows and deadlocks when routing through virtual devices:
>
>         dev_xmit_recursion_inc();
>         HARD_TX_LOCK(dev, txq, smp_processor_id());

This is intended because I think it's not necessary for XSK that is
only triggered by sendmsg only in copy mode. No such terrible thing is
likely to happen as no one binds xsk to macvlan type of devices.

I will reconsider it, fearing some over-engineering work impacts the
performance.

>
> > +     HARD_TX_LOCK(dev, txq, smp_processor_id());
> > +     while ((skb = __skb_dequeue(send_queue)) != NULL) {
> > +             struct sk_buff *orig_skb = skb;
> > +             bool again = false;
> > +
> > +             skb = validate_xmit_skb_list(skb, dev, &again);
>
> Will executing validate_xmit_skb_list() inside the hardware transmit lock
> cause lock contention? This function can perform computationally expensive
> operations such as GSO segmentation, software checksum fallbacks, and
> subsystem hooks. Other transmit paths execute this validation strictly
> before acquiring the hardware lock.

The next patch adds a fast path check for this so it has been improved.

>
> > +             if (skb != orig_skb) {
> > +                     dev_core_stats_tx_dropped_inc(dev);
> > +                     kfree_skb_list(skb);
> > +                     ret = NET_XMIT_DROP;
> > +                     break;
> > +             }
> > +
> > +             if (netif_xmit_frozen_or_drv_stopped(txq)) {
> > +                     __skb_queue_head(send_queue, skb);
> > +                     break;
> > +             }
> > +             skb_set_queue_mapping(skb, queue_id);
> > +             ret = netdev_start_xmit(skb, dev, txq, false);
>
> Does passing false for the more parameter defeat the purpose of hardware
> doorbell batching? It instructs the driver to ring the hardware doorbell for
> every single packet instead of delaying it until the end of the batch.
>
> It looks like this might be fixed later in the patch series by commit
> 6d7c8f2ad056e ("xsk: support dynamic xmit.more control for batch xmit").

Yep, it was suggested by Jesper already.

Thanks,
Jason

>
> > +             if (ret != NETDEV_TX_OK) {
> > +                     if (ret == NETDEV_TX_BUSY)
> > +                             __skb_queue_head(send_queue, skb);
> > +                     break;
> > +             }
> > +     }
> > +     HARD_TX_UNLOCK(dev, txq);
>
> Also, we would need to pair any added recursion increment with a decrement
> here:
>
>         dev_xmit_recursion_dec();
>
> > +     local_bh_enable();
> > +
> > +     return ret;
> > +}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5