Message-ID: <6878655ca06c7_9aa0c294c5@willemb.c.googlers.com.notmuch>
Date: Wed, 16 Jul 2025 22:52:12 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, 
 edumazet@...gle.com, 
 pabeni@...hat.com, 
 bjorn@...nel.org, 
 magnus.karlsson@...el.com, 
 maciej.fijalkowski@...el.com, 
 jonathan.lemon@...il.com, 
 sdf@...ichev.me, 
 ast@...nel.org, 
 daniel@...earbox.net, 
 hawk@...nel.org, 
 john.fastabend@...il.com, 
 joe@...a.to, 
 willemdebruijn.kernel@...il.com, 
 bpf@...r.kernel.org, 
 netdev@...r.kernel.org, 
 Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v2] xsk: skip validating skb list in xmit path

Jason Xing wrote:
> On Thu, Jul 17, 2025 at 8:52 AM Jakub Kicinski <kuba@...nel.org> wrote:
> >
> > On Thu, 17 Jul 2025 08:06:48 +0800 Jason Xing wrote:
> > > To be honest, this patch really only does one thing as the commit
> > > says. It might look very complex, but if readers take a deep look they
> > > will find only one removal of that validation for xsk in the hot path.
> > > Nothing more and nothing less. So IMHO, it doesn't bring more complex
> > > codes here.
> > >
> > > And removal of one validation indeed contributes to the transmission.
> > > I believe there remain a number of applications using copy mode
> > > currently. And maintainers of xsk don't regard copy mode as orphaned,
> > > right?
> >
> > First of all, I'm not sure the patch is correct. The XSK skbs can have
> > frags, if device doesn't support or clears _SG we should linearize,
> > right?
> 
> But note that there is one more function __skb_linearize() after
> skb_needs_linearize() in the validate_xmit_skb(). __skb_linearize()
> tests many members of skbs, which are not used to check the skbs from
> xsk. For xsk, it's very simple (please see xsk_build_skb())

For single frame xsk skb_needs_linearize will be false and thus
__skb_linearize is not called?

More generally, I would also think that the cost of the
validate_xmit_skb checks are quite cheap in the xsk case where they
are all false. On the assumption that the touched cachelines are
likely warm.
 
> >
> > Second, we don't understand where the win is coming from, the numbers
> > you share are a bit vague. What's so expensive about a few skbs
> 
> To be more accurate, it's not "a few" but "so many" because of the
> high pps reaching more than 1,000,000. So if people run the xdpsock to
> test it, it's not hard to see most of time is spent during the skb
> allocation process.

Right, the alloc or memcpy more than the validate?

> > accesses? Maybe there's an optimization possible to the validation,
> > which would apply more broadly, instead of skipping it for one trivial
> > case.
> >
> > Third, I asked you to compare with AF_PACKET, because IIUC it should
> > have similar properties as AF_XDP in copy mode. So why not use that?
> 
> I haven't run into AF_PACKET so far. At least, I can confirm that xsk
> doesn't need it from my side. The whole logic of validation apparently
> is not designed for xsk case...
> 
> >
> > Lastly, the patch is not all that bad, sure. But the experience of
> > supporting generic XDP is a very mixed. All the paths that pretend
> > to do XDP on skbs have a bunch of quirks and bugs. I'd prefer that
> > we push back more broadly on any sort of pretend XDP.
> 
> Well, sorry, I feel a bit upset when reading this because as I
> insisted before not everyone can use the advanced zerocopy mode.
> 
> Thanks,
> Jason

Powered by blists - more mailing lists