[PATCH net-next v2] xsk: skip validating skb list in xmit path

[PATCH net-next v2] xsk: skip validating skb list in xmit path

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

This patch only does one thing that removes validate_xmit_skb_list()
for xsk.

For xsk, it's not needed to validate and check the skb in
validate_xmit_skb_list() in copy mode because xsk_build_skb() doesn't
and doesn't need to prepare those requisites to validate. Xsk is just
responsible for delivering raw data from userspace to the driver.

The __dev_direct_xmit was taken out of af_packet in commit 865b03f21162
("dev: packet: make packet_direct_xmit a common function"). And a call
to validate_xmit_skb_list was added in commit 104ba78c9880 ("packet: on
direct_xmit, limit tso and csum to supported devices") to support TSO.
Since we don't support tso/vlan offloads in xsk_build_skb, we can remove
validate_xmit_skb_list for xsk. Skipping numerous checks somehow
contributes to the transmission especially in the extremely hot path.

Performance-wise, I used './xdpsock -i enp2s0f0np0 -t  -S -s 64' to verify
the guess and then measured on the machine with ixgbe driver. It stably
goes up by 5.48%, which can be seen in the shown below:
Before:
 sock0@enp2s0f0np0:0 txonly xdp-skb
                   pps            pkts           1.00
rx                 0              0
tx                 1,187,410      3,513,536
After:
 sock0@enp2s0f0np0:0 txonly xdp-skb
                   pps            pkts           1.00
rx                 0              0
tx                 1,252,590      2,459,456

This patch also removes total ~4% consumption which can be observed
by perf:
|--2.97%--validate_xmit_skb
|          |
|           --1.76%--netif_skb_features
|                     |
|                      --0.65%--skb_network_protocol
|
|--1.06%--validate_xmit_xfrm

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
V2
Link: https://lore.kernel.org/all/20250713025756.24601-1-kerneljasonxing@gmail.com/
1. avoid adding a new flag
2. add more descriptions from Stan
---
 include/linux/netdevice.h | 30 ++++++++++++++++++++----------
 net/core/dev.c            |  6 ------
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a80d21a14612..8e05c99928e1 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3364,16 +3364,6 @@ static inline int dev_queue_xmit_accel(struct sk_buff *skb,
 	return __dev_queue_xmit(skb, sb_dev);
 }
 
-static inline int dev_direct_xmit(struct sk_buff *skb, u16 queue_id)
-{
-	int ret;
-
-	ret = __dev_direct_xmit(skb, queue_id);
-	if (!dev_xmit_complete(ret))
-		kfree_skb(skb);
-	return ret;
-}
-
 int register_netdevice(struct net_device *dev);
 void unregister_netdevice_queue(struct net_device *dev, struct list_head *head);
 void unregister_netdevice_many(struct list_head *head);
@@ -4301,6 +4291,26 @@ static __always_inline int ____dev_forward_skb(struct net_device *dev,
 	return 0;
 }
 
+static inline int dev_direct_xmit(struct sk_buff *skb, u16 queue_id)
+{
+	struct net_device *dev = skb->dev;
+	struct sk_buff *orig_skb = skb;
+	bool again = false;
+	int ret;
+
+	skb = validate_xmit_skb_list(skb, dev, &again);
+	if (skb != orig_skb) {
+		dev_core_stats_tx_dropped_inc(dev);
+		kfree_skb_list(skb);
+		return NET_XMIT_DROP;
+	}
+
+	ret = __dev_direct_xmit(skb, queue_id);
+	if (!dev_xmit_complete(ret))
+		kfree_skb(skb);
+	return ret;
+}
+
 bool dev_nit_active_rcu(const struct net_device *dev);
 static inline bool dev_nit_active(const struct net_device *dev)
 {
diff --git a/net/core/dev.c b/net/core/dev.c
index e365b099484e..793f5d45c6b2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4744,19 +4744,13 @@ EXPORT_SYMBOL(__dev_queue_xmit);
 int __dev_direct_xmit(struct sk_buff *skb, u16 queue_id)
 {
 	struct net_device *dev = skb->dev;
-	struct sk_buff *orig_skb = skb;
 	struct netdev_queue *txq;
 	int ret = NETDEV_TX_BUSY;
-	bool again = false;
 
 	if (unlikely(!netif_running(dev) ||
 		     !netif_carrier_ok(dev)))
 		goto drop;
 
-	skb = validate_xmit_skb_list(skb, dev, &again);
-	if (skb != orig_skb)
-		goto drop;
-
 	skb_set_queue_mapping(skb, queue_id);
 	txq = skb_get_tx_queue(dev, skb);
 
-- 
2.41.3

Re: [PATCH net-next v2] xsk: skip validating skb list in xmit path

[Stanislav Fomichev]

On 07/16, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> This patch only does one thing that removes validate_xmit_skb_list()
> for xsk.
> 
> For xsk, it's not needed to validate and check the skb in
> validate_xmit_skb_list() in copy mode because xsk_build_skb() doesn't
> and doesn't need to prepare those requisites to validate. Xsk is just
> responsible for delivering raw data from userspace to the driver.
> 
> The __dev_direct_xmit was taken out of af_packet in commit 865b03f21162
> ("dev: packet: make packet_direct_xmit a common function"). And a call
> to validate_xmit_skb_list was added in commit 104ba78c9880 ("packet: on
> direct_xmit, limit tso and csum to supported devices") to support TSO.
> Since we don't support tso/vlan offloads in xsk_build_skb, we can remove
> validate_xmit_skb_list for xsk. Skipping numerous checks somehow
> contributes to the transmission especially in the extremely hot path.
> 
> Performance-wise, I used './xdpsock -i enp2s0f0np0 -t  -S -s 64' to verify
> the guess and then measured on the machine with ixgbe driver. It stably
> goes up by 5.48%, which can be seen in the shown below:
> Before:
>  sock0@enp2s0f0np0:0 txonly xdp-skb
>                    pps            pkts           1.00
> rx                 0              0
> tx                 1,187,410      3,513,536
> After:
>  sock0@enp2s0f0np0:0 txonly xdp-skb
>                    pps            pkts           1.00
> rx                 0              0
> tx                 1,252,590      2,459,456
> 
> This patch also removes total ~4% consumption which can be observed
> by perf:
> |--2.97%--validate_xmit_skb
> |          |
> |           --1.76%--netif_skb_features
> |                     |
> |                      --0.65%--skb_network_protocol
> |
> |--1.06%--validate_xmit_xfrm
> 
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
> V2
> Link: https://lore.kernel.org/all/20250713025756.24601-1-kerneljasonxing@gmail.com/
> 1. avoid adding a new flag
> 2. add more descriptions from Stan

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

LGTM, but would be nice if Willem or Magnus can chime in to confirm that
it's safe.

Re: [PATCH net-next v2] xsk: skip validating skb list in xmit path

[Jakub Kicinski]

On Wed, 16 Jul 2025 20:27:25 +0800 Jason Xing wrote:
> This patch only does one thing that removes validate_xmit_skb_list()
> for xsk.

Please no, I understand that it's fun to optimize the fallback paths
but it increases the complexity of the stack.
-- 
pw-bot: reject

Re: [PATCH net-next v2] xsk: skip validating skb list in xmit path

[Jason Xing]

On Thu, Jul 17, 2025 at 5:56 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 16 Jul 2025 20:27:25 +0800 Jason Xing wrote:
> > This patch only does one thing that removes validate_xmit_skb_list()
> > for xsk.
>
> Please no, I understand that it's fun to optimize the fallback paths
> but it increases the complexity of the stack.

Are you suggesting to remove this description? And I see you marked it
as 'rejected', so it seems that I should use the V1 patch which
doesn't increase the complexity.

https://lore.kernel.org/all/20250713025756.24601-1-kerneljasonxing@gmail.com/

Thanks,
Jason

> --
> pw-bot: reject

Re: [PATCH net-next v2] xsk: skip validating skb list in xmit path

[Jakub Kicinski]

On Thu, 17 Jul 2025 07:37:42 +0800 Jason Xing wrote:
> On Thu, Jul 17, 2025 at 5:56 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Wed, 16 Jul 2025 20:27:25 +0800 Jason Xing wrote:  
> > > This patch only does one thing that removes validate_xmit_skb_list()
> > > for xsk.  
> >
> > Please no, I understand that it's fun to optimize the fallback paths
> > but it increases the complexity of the stack.  
> 
> Are you suggesting to remove this description? And I see you marked it
> as 'rejected', so it seems that I should use the V1 patch which
> doesn't increase the complexity.

No, I'm questioning optimizing the copy mode AF_XDP in the first place.

Re: [PATCH net-next v2] xsk: skip validating skb list in xmit path

[Jason Xing]

On Thu, Jul 17, 2025 at 7:43 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 17 Jul 2025 07:37:42 +0800 Jason Xing wrote:
> > On Thu, Jul 17, 2025 at 5:56 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Wed, 16 Jul 2025 20:27:25 +0800 Jason Xing wrote:
> > > > This patch only does one thing that removes validate_xmit_skb_list()
> > > > for xsk.
> > >
> > > Please no, I understand that it's fun to optimize the fallback paths
> > > but it increases the complexity of the stack.
> >
> > Are you suggesting to remove this description? And I see you marked it
> > as 'rejected', so it seems that I should use the V1 patch which
> > doesn't increase the complexity.
>
> No, I'm questioning optimizing the copy mode AF_XDP in the first place.

Yesterday, Stan asked me the same question, but the zerocopy for me is
obviously not qualified for deployment right now. Let me rephrase a
bit:

1) There are still many VMs that don't support zerocopy in the real world.
2) Some old drivers have various unknown problems. I posted one at
https://lore.kernel.org/all/CAL+tcoCTHTptwmok9vhp7GEwQgMhNsBJxT3PStJDeVOLR_-Q3g@mail.gmail.com/

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?

Jakub, if you can, please don't mark it as rejected. Thanks.

Thanks,
Jason

Re: [PATCH net-next v2] xsk: skip validating skb list in xmit path

[Jakub Kicinski]

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?

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
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?

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.

Re: [PATCH net-next v2] xsk: skip validating skb list in xmit path

[Jason Xing]

On Thu, Jul 17, 2025 at 8:52 AM Jakub Kicinski <kuba@kernel.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())

>
> 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.

> 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

Re: [PATCH net-next v2] xsk: skip validating skb list in xmit path

[Willem de Bruijn]

Jason Xing wrote:
> On Thu, Jul 17, 2025 at 8:52 AM Jakub Kicinski <kuba@kernel.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

Re: [PATCH net-next v2] xsk: skip validating skb list in xmit path

[Jason Xing]

On Thu, Jul 17, 2025 at 10:52 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > On Thu, Jul 17, 2025 at 8:52 AM Jakub Kicinski <kuba@kernel.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?

Thanks for chiming in.

Sure thing. Validate only takes 4% total time, which could be easily
observed by using perf.

The story behind the patch is that I was scanning the code and found
the validation is not necessary based on the theory instead of
experiment, _then_ I tried xdpsock to see if any performance impact
and used perf to capture the hot spot.

And I don't think I can find any other useful improvement whether in
copy mode or zc mode after finishing investigation so far.

Last time, I mentioned that I tried two out-of-thin-air approaches[1].
But everything didn't go as well as expected...

[1]: https://lore.kernel.org/all/CAL+tcoAn8ADUGARSzZB=5dGoa+Kh7HnNBLxyqTa3W6tOhUK-sg@mail.gmail.com/

Thanks,
Jason