Re: [PATCH net v4 3/5] xsk: drain continuation descs after overflow in xsk_build_skb()

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Thu, May 21, 2026 at 07:53:27AM +0800, Jason Xing wrote:
> On Thu, May 21, 2026 at 12:10 AM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> wrote:
> >
> > On Wed, May 20, 2026 at 08:42:42AM +0800, Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > When a multi-buffer packet exceeds MAX_SKB_FRAGS and triggers -EOVERFLOW,
> > > only the current descriptor is released from the TX ring. The remaining
> > > continuation descriptors of the same packet stay in the ring. Since
> > > xs->skb is set to NULL after the drop, the TX loop picks up these
> > > leftover frags and misinterprets each one as the beginning of a new
> > > packet, corrupting the packet stream.
> > >
> > > Fix this by adding a drain_cont flag to xdp_sock. When overflow occurs
> > > and the dropped descriptor has XDP_PKT_CONTD set, the flag is raised,
> > > so we have a chance to examine and handle the potential remaining descs
> > > of this big overflow'ed skb.
> > >
> > > When the last fragment (without XDP_PKT_CONTD) is processed, the flag
> > > is cleared and the loop continues to process subsequent descriptors
> > > with the remaining budget. This behavior follows how previous xmit path
> > > treats overflow packets.
> > >
> > > Closes: https://lore.kernel.org/all/20260425041726.85FB3C2BCB2@smtp.kernel.org/
> > > Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")
> > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > ---
> > >  include/net/xdp_sock.h |  1 +
> > >  net/xdp/xsk.c          | 19 +++++++++++++++++++
> > >  2 files changed, 20 insertions(+)
> > >
> > > diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> > > index ebac60a3d8a1..8b51876efbed 100644
> > > --- a/include/net/xdp_sock.h
> > > +++ b/include/net/xdp_sock.h
> > > @@ -80,6 +80,7 @@ struct xdp_sock {
> > >        * call of __xsk_generic_xmit().
> > >        */
> > >       struct sk_buff *skb;
> > > +     bool drain_cont;
> > >
> > >       struct list_head map_list;
> > >       /* Protects map_list */
> > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > index 0a6203c42576..f4add7be8c93 100644
> > > --- a/net/xdp/xsk.c
> > > +++ b/net/xdp/xsk.c
> > > @@ -1062,11 +1062,30 @@ static int __xsk_generic_xmit(struct sock *sk)
> > >                       goto out;
> > >               }
> > >
> > > +             if (unlikely(xs->drain_cont)) {
> >
> > Hi Jason,
> >
> > could we get away from introducing dedicated boolean to xdp_sock for
> > handling this rare case?
> 
> I've tried several ways, but they didn't work. Please see the short
> conversation with Stan:
> https://lore.kernel.org/all/CAL+tcoA6Cu3P-y2wYjJ_R9cSeRkW2tswY1LveqH=uxd2McuR-w@mail.gmail.com/
> 
> That means we always rely on a flag (that is either static or like
> this patch) to recognize how we treat this abnormal descs.
> 
> >
> > > +                     unsigned long flags;
> > > +                     u32 idx;
> > > +
> > > +                     spin_lock_irqsave(&xs->pool->cq_prod_lock, flags);
> > > +                     idx = xskq_get_prod(xs->pool->cq);
> > > +                     xskq_prod_write_addr(xs->pool->cq, idx, desc.addr);
> > > +                     xskq_prod_submit_n(xs->pool->cq, 1);
> > > +                     spin_unlock_irqrestore(&xs->pool->cq_prod_lock, flags);
> >
> > It feels a bit off to do this cleanup one-by-one.
> 
> Actually no, it adheres to the basic send logic in copy mode.

Not really. Here you are producer to cq which we have batched variants for
usage.

But after second thoughts I don't think it would be worth to put any
further effort onto optimizations here. Anyways, even though generic xmit
is a whole bunch of mess, maybe you could pull this out onto a helper
function?

> 
> >
> > I wonder if it could be covered by xsk_drop_skb() where you would walk the
> > tx ring and store addrs at xsk_addrs and then use xsk_cq_submit_addr_locked() ?
> >
> > OTOH, in theory the amount of frags after we hit the overflow case could
> > be well over MAX_SKB_FRAGS again so xsk_addrs wouldn't be able to cover
> > them.
> 
> Right, this is why we now put the logic into the xmit path just like
> other kinds of descriptors. We need to check and send it under a
> series of protections like the check of cq pressure and budget. I
> don't think it's an exception to escape the check logic. The whole
> process should be fast as there is no need to build skb and send it to
> the driver as long as this case is recognized.
> 
> Thanks,
> Jason
> 
> >
> > > +
> > > +                     xs->tx->invalid_descs++;
> > > +                     xskq_cons_release(xs->tx);
> > > +                     if (!xp_mb_desc(&desc))
> > > +                             xs->drain_cont = false;
> > > +                     continue;
> > > +             }
> > > +
> > >               skb = xsk_build_skb(xs, &desc);
> > >               if (IS_ERR(skb)) {
> > >                       err = PTR_ERR(skb);
> > >                       if (err != -EOVERFLOW)
> > >                               goto out;
> > > +                     if (xp_mb_desc(&desc))
> > > +                             xs->drain_cont = true;
> > >                       err = 0;
> > >                       continue;
> > >               }
> > > --
> > > 2.43.7
> > >