Re: [PATCH net v2 1/2] xsk: defer software checksum until the skb is complete

[Magnus Karlsson]

On Mon, 26 Oct 2026 at 13:32, Maciej Fijalkowski
<maciej.fijalkowski@xxxxxxxxx> wrote:
>
> On Sat, Oct 24, 2026 at 10:11:56PM +0800, Rongzheng Wang wrote:
> > When XDP_UMEM_TX_SW_CSUM is used with multi-buffer TX, xsk_skb_metadata()
> > calls skb_checksum_help() while only the first descriptor has been added to
> > the skb. The checksum therefore covers just the first fragment, and
> > skb_checksum_help() sets ip_summed to CHECKSUM_NONE, so the remaining
> > fragments are never included in the checksum.
> >
> > Move the software checksum calculation to the point where the last
> > descriptor has been added to the skb.
> >
> > On a checksum failure the packet is now dropped instead of being returned
> > to the application for a retry. This path is not reachable with the
> > existing metadata validation, but the behavior change is intentional.
> >
> > Fixes: 11614723af26 ("xsk: Add option to calculate TX checksum in SW")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Rongzheng Wang <testwrzwrz@xxxxxxxxx>
> > ---
> >  net/xdp/xsk.c | 22 ++++++++++++++--------
> >  1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 33475b180ea6..65be2a368325 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -1011,14 +1011,6 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
> >               skb->csum_start = hr + csum_start;
> >               skb->csum_offset = csum_offset;
> >               skb->ip_summed = CHECKSUM_PARTIAL;
> > -
> > -             if (unlikely(pool->tx_sw_csum)) {
> > -                     int err;
> > -
> > -                     err = skb_checksum_help(skb);
> > -                     if (err)
> > -                             return err;
> > -             }
> >       }
> >
> >       if (flags & XDP_TXMD_FLAGS_LAUNCH_TIME)
> > @@ -1030,6 +1022,14 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
> >       return 0;
> >  }
> >
> > +static int xsk_skb_sw_csum(struct sk_buff *skb, struct xsk_buff_pool *pool)
> > +{
> > +     if (!pool->tx_sw_csum || skb->ip_summed != CHECKSUM_PARTIAL)
>
> If i am reading this right check against ip_summed is a way to tell that
> first frag had XDP_TXMD_FLAGS_CHECKSUM flag set.
>
> Agreed with Jason to preserve unlikely() when checking pool->tx_sw_csum.
>
> Regarding skb drop, I am leaning towards this, I don't see much sense in
> retry when packet is malformed? Not sure how this could be communicated up
> to user space.

Do we need a new error statistics counter, e.g. tx_malformed_packet or
tx_invalid_packet? We do have one for invalid descriptors, but that
does not really apply here.

> > +             return 0;
> > +
> > +     return skb_checksum_help(skb);
> > +}
> > +
> >  static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
> >                                             struct xdp_desc *desc)
> >  {
> > @@ -1275,6 +1275,12 @@ static int __xsk_generic_xmit(struct sock *sk)
> >               if (xp_mb_desc(&desc)) {
> >                       xs->skb = skb;
> >                       continue;
> > +             } else {
> > +                     err = xsk_skb_sw_csum(skb, xs->pool);
> > +                     if (unlikely(err)) {
> > +                             xsk_drop_skb(skb);
> > +                             goto out;
> > +                     }
> >               }
> >
> >               err = __dev_direct_xmit(skb, xs->queue_id);
> > --
> > 2.39.5 (Apple Git-154)
> >
>