xdp: allow disable GRO per packet by XDP

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two.

On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by Yan Zhai <[email protected]>

---
 include/linux/netdevice.h |  9 +++++++--
 include/linux/skbuff.h    | 10 ++++++++++
 net/Kconfig               | 10 ++++++++++
 net/core/gro.c            |  2 +-
 net/core/gro_cells.c      |  2 +-
 net/core/skbuff.c         |  4 ++++
 6 files changed, 33 insertions(+), 4 deletions(-)

include/linux/netdevice.h +7 -2

index c83b390191d4..2ca0870b1221 100644
@@ -2415,11 +2415,16 @@ struct net_device {
 	((dev)->devlink_port = (port));				\
 })
 
-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)
 {
-	if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+	if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
 		return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+	return skb->gro_disabled;
+#else
 	return false;
+#endif
 }
 
 #define	NETDEV_ALIGN		32

include/linux/skbuff.h +10 -0

index f4cda3fbdb75..48b10ece95b5 100644
@@ -1008,6 +1008,9 @@ struct sk_buff {
 #if IS_ENABLED(CONFIG_IP_SCTP)
 	__u8			csum_not_inet:1;
 #endif
+#ifdef CONFIG_SKB_GRO_CONTROL
+	__u8			gro_disabled:1;
+#endif
 
 #if defined(CONFIG_NET_SCHED) || defined(CONFIG_NET_XGRESS)
 	__u16			tc_index;	/* traffic control index */

@@ -1215,6 +1218,13 @@ static inline bool skb_wifi_acked_valid(const struct sk_buff *skb)
 #endif
 }
 
+static inline void skb_disable_gro(struct sk_buff *skb)
+{
+#ifdef CONFIG_SKB_GRO_CONTROL
+	skb->gro_disabled = 1;
+#endif
+}
+
 /**
  * skb_unref - decrement the skb's reference count
  * @skb: buffer

net/Kconfig +0 -0

index 9fe65fa26e48..47d1ee92df15 100644
@@ -289,6 +289,16 @@ config MAX_SKB_FRAGS
 	  and in drivers using build_skb().
 	  If unsure, say 17.

net/Kconfig +1 -0

+
 config RPS
 	bool "Receive packet steering"
 	depends on SMP && SYSFS

net/core/gro.c +1 -1

index b3b43de1a650..46232a0d1983 100644
@@ -476,7 +476,7 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
 	enum gro_result ret;
 	int same_flow;
 
-	if (netif_elide_gro(skb->dev))
+	if (netif_elide_gro(skb))
 		goto normal;
 
 	gro_list_prepare(&gro_list->list, skb);

net/core/gro_cells.c +1 -1

index ff8e5b64bf6b..1bf15783300f 100644
@@ -20,7 +20,7 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb)
 	if (unlikely(!(dev->flags & IFF_UP)))
 		goto drop;
 
-	if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) {
+	if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(skb)) {
 		res = netif_rx(skb);
 		goto unlock;
 	}

net/core/skbuff.c +0 -0

index 2315c088e91d..82bd297921c1 100644

From: Yan Zhai <[email protected]>
Date: Thu, 20 Jun 2024 15:19:10 -0700

Quoted 8 lines

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

Quoted 7 lines

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two.

Implement it in the kernel then? :D

Quoted 76 lines

On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by Yan Zhai <[email protected]>

---
 include/linux/netdevice.h |  9 +++++++--
 include/linux/skbuff.h    | 10 ++++++++++
 net/Kconfig               | 10 ++++++++++
 net/core/gro.c            |  2 +-
 net/core/gro_cells.c      |  2 +-
 net/core/skbuff.c         |  4 ++++
 6 files changed, 33 insertions(+), 4 deletions(-)

include/linux/netdevice.h +7 -2

index c83b390191d4..2ca0870b1221 100644
@@ -2415,11 +2415,16 @@ struct net_device {
      ((dev)->devlink_port = (port));                         \
 })
 
-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)
 {
-     if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+     if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
              return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+     return skb->gro_disabled;
+#else
      return false;
+#endif
 }
 
 #define      NETDEV_ALIGN            32

include/linux/skbuff.h +10 -0

index f4cda3fbdb75..48b10ece95b5 100644
@@ -1008,6 +1008,9 @@ struct sk_buff {
 #if IS_ENABLED(CONFIG_IP_SCTP)
      __u8                    csum_not_inet:1;
 #endif
+#ifdef CONFIG_SKB_GRO_CONTROL
+     __u8                    gro_disabled:1;
+#endif
 
 #if defined(CONFIG_NET_SCHED) || defined(CONFIG_NET_XGRESS)
      __u16                   tc_index;       /* traffic control index */

@@ -1215,6 +1218,13 @@ static inline bool skb_wifi_acked_valid(const struct sk_buff *skb)
 #endif
 }
 
+static inline void skb_disable_gro(struct sk_buff *skb)
+{
+#ifdef CONFIG_SKB_GRO_CONTROL
+     skb->gro_disabled = 1;
+#endif
+}
+
 /**
  * skb_unref - decrement the skb's reference count
  * @skb: buffer

net/Kconfig +0 -0

index 9fe65fa26e48..47d1ee92df15 100644
@@ -289,6 +289,16 @@ config MAX_SKB_FRAGS
        and in drivers using build_skb().
        If unsure, say 17.
 

net/Kconfig config MAX_SKB_FRAGS · line 300

Quoted 9 lines

net/Kconfig +9 -0

+config SKB_GRO_CONTROL
+     bool "allow disable GRO on per-packet basis"
+     default y
+     help
+       By default GRO can only be enabled or disabled per network device.
+       This can be cumbersome for certain scenarios.
+       Toggling this option will allow disabling GRO for selected packets,
+       e.g. by XDP programs, so that it is more flexibile.
+       Extra overhead should be minimal.

I don't think we need a Kconfig option for that. Can't it be unconditional? Is there any real eye-visible overhead?

Thanks,
Olek

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two. On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by Yan Zhai <[email protected]>

---
 include/linux/netdevice.h |  9 +++++++--
 include/linux/skbuff.h    | 10 ++++++++++
 net/Kconfig               | 10 ++++++++++
 net/core/gro.c            |  2 +-
 net/core/gro_cells.c      |  2 +-
 net/core/skbuff.c         |  4 ++++
 6 files changed, 33 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -1

index c83b390191d4..2ca0870b1221 100644
@@ -2415,11 +2415,16 @@ struct net_device {
 	((dev)->devlink_port = (port));				\
 })
 
-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)

Quoted 42 lines · Yan Zhai

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two. On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by Yan Zhai <[email protected]>

---
 include/linux/netdevice.h |  9 +++++++--
 include/linux/skbuff.h    | 10 ++++++++++
 net/Kconfig               | 10 ++++++++++
 net/core/gro.c            |  2 +-
 net/core/gro_cells.c      |  2 +-
 net/core/skbuff.c         |  4 ++++
 6 files changed, 33 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -1

index c83b390191d4..2ca0870b1221 100644
@@ -2415,11 +2415,16 @@ struct net_device {
      ((dev)->devlink_port = (port));                         \
 })
 
-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)

include/linux/netdevice.h struct net_device { · line 2427

Quoted 10 lines

include/linux/netdevice.h +6 -1

 {
-     if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+     if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
              return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+     return skb->gro_disabled;
+#else
      return false;
+#endif

This will generate OoO if the gro_disabled is flipped in the middle of a stream.

Assuming the above is fine for your use case (I think it's _not_ in general), you could get the same result without an additional costly bit in sk_buff.

Let xdp_frame_fixup_skb_offloading() return a bool - e.g. 'true' when gro should be avoided - and let the NIC driver call netif_receive_skb() instead of the gro rx hook for such packet.

All in all the approach implemented in this series does not look worthy to me.

Thanks,

Paolo

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two. On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Quoted 8 lines · Yan Zhai

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

Quoted 10 lines

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two. On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Could you please add more details WRT this last statement? I'm unsure if I understand your problem. My guess is as follow:

Your device receive some traffic, GRO and forward it, and the multiple encapsulation can happen on such forwarded traffic (since I can't find almost none of the above your message is mainly a wild guess).

Assuming I guessed correctly, I think you could solve the problem with no kernel changes: redirect the to-be-tunneled traffic to some virtual device and all TX offload on top of it and let the encap happen there.

Cheers,

Paolo

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two. On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by Yan Zhai <[email protected]>

---
 include/linux/netdevice.h |  9 +++++++--
 include/linux/skbuff.h    | 10 ++++++++++
 net/Kconfig               | 10 ++++++++++
 net/core/gro.c            |  2 +-
 net/core/gro_cells.c      |  2 +-
 net/core/skbuff.c         |  4 ++++
 6 files changed, 33 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -1

index c83b390191d4..2ca0870b1221 100644
@@ -2415,11 +2415,16 @@ struct net_device {
 	((dev)->devlink_port = (port));				\
 })
 
-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)

include/linux/netdevice.h +0 -0

 }
 
 #define	NETDEV_ALIGN		32

include/linux/skbuff.h +10 -0

index f4cda3fbdb75..48b10ece95b5 100644
@@ -1008,6 +1008,9 @@ struct sk_buff {
 #if IS_ENABLED(CONFIG_IP_SCTP)
 	__u8			csum_not_inet:1;
 #endif
+#ifdef CONFIG_SKB_GRO_CONTROL
+	__u8			gro_disabled:1;
+#endif
 
 #if defined(CONFIG_NET_SCHED) || defined(CONFIG_NET_XGRESS)
 	__u16			tc_index;	/* traffic control index */

@@ -1215,6 +1218,13 @@ static inline bool skb_wifi_acked_valid(const struct sk_buff *skb)
 #endif
 }
 
+static inline void skb_disable_gro(struct sk_buff *skb)
+{
+#ifdef CONFIG_SKB_GRO_CONTROL
+	skb->gro_disabled = 1;
+#endif
+}
+
 /**
  * skb_unref - decrement the skb's reference count
  * @skb: buffer

net/Kconfig +10 -0

index 9fe65fa26e48..47d1ee92df15 100644
@@ -289,6 +289,16 @@ config MAX_SKB_FRAGS
 	  and in drivers using build_skb().
 	  If unsure, say 17.
 
+config SKB_GRO_CONTROL
+	bool "allow disable GRO on per-packet basis"
+	default y
+	help
+	  By default GRO can only be enabled or disabled per network device.
+	  This can be cumbersome for certain scenarios.
+	  Toggling this option will allow disabling GRO for selected packets,
+	  e.g. by XDP programs, so that it is more flexibile.
+	  Extra overhead should be minimal.
+
 config RPS
 	bool "Receive packet steering"
 	depends on SMP && SYSFS

net/core/gro.c +1 -1

index b3b43de1a650..46232a0d1983 100644
@@ -476,7 +476,7 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
 	enum gro_result ret;
 	int same_flow;
 
-	if (netif_elide_gro(skb->dev))
+	if (netif_elide_gro(skb))
 		goto normal;
 
 	gro_list_prepare(&gro_list->list, skb);

net/core/gro_cells.c +1 -1

index ff8e5b64bf6b..1bf15783300f 100644
@@ -20,7 +20,7 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb)
 	if (unlikely(!(dev->flags & IFF_UP)))
 		goto drop;
 
-	if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) {
+	if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(skb)) {
 		res = netif_rx(skb);
 		goto unlock;
 	}

net/core/skbuff.c +4 -0

index 2315c088e91d..82bd297921c1 100644
@@ -6030,6 +6030,10 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet)
 	ipvs_reset(skb);
 	skb->mark = 0;
 	skb_clear_tstamp(skb);
+#ifdef CONFIG_SKB_GRO_CONTROL
+	/* hand back GRO control to next netns */
+	skb->gro_disabled = 0;
+#endif
 }
 EXPORT_SYMBOL_GPL(skb_scrub_packet);
 

Signature · 1 line

2.30.2

Quoted 42 lines · Yan Zhai

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two. On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by Yan Zhai <[email protected]>

---
  include/linux/netdevice.h |  9 +++++++--
  include/linux/skbuff.h    | 10 ++++++++++
  net/Kconfig               | 10 ++++++++++
  net/core/gro.c            |  2 +-
  net/core/gro_cells.c      |  2 +-
  net/core/skbuff.c         |  4 ++++
  6 files changed, 33 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -1

index c83b390191d4..2ca0870b1221 100644
@@ -2415,11 +2415,16 @@ struct net_device {
  	((dev)->devlink_port = (port));				\
  })
 
-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)

include/linux/netdevice.h struct net_device { · line 2427

Quoted 10 lines

include/linux/netdevice.h +6 -1

  {
-	if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+	if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
  		return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+	return skb->gro_disabled;
+#else
  	return false;
+#endif

Yet more branches in the hot path.

Compile time configurability does not help, as that will be enabled by distros.

For a fairly niche use case. Where functionality of GRO already works. So just a performance for a very rare case at the cost of a regression in the common case. A small regression perhaps, but death by a thousand cuts.

Quoted 56 lines · Willem de Bruijn

Quoted 42 lines · Yan Zhai

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two. On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by Yan Zhai <[email protected]>

---
  include/linux/netdevice.h |  9 +++++++--
  include/linux/skbuff.h    | 10 ++++++++++
  net/Kconfig               | 10 ++++++++++
  net/core/gro.c            |  2 +-
  net/core/gro_cells.c      |  2 +-
  net/core/skbuff.c         |  4 ++++
  6 files changed, 33 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -1

index c83b390191d4..2ca0870b1221 100644
@@ -2415,11 +2415,16 @@ struct net_device {
     ((dev)->devlink_port = (port));                         \
  })
 
-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)

include/linux/netdevice.h struct net_device { · line 2427

Quoted 10 lines

include/linux/netdevice.h +6 -1

  {
-    if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+    if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
             return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+    return skb->gro_disabled;
+#else
     return false;
+#endif

Yet more branches in the hot path.

Quoted 7 lines

Compile time configurability does not help, as that will be enabled by distros.

For a fairly niche use case. Where functionality of GRO already works. So just a performance for a very rare case at the cost of a regression in the common case. A small regression perhaps, but death by a thousand cuts.

Mentioning it here b/c it perhaps fits in this context, longer time ago there was the idea mentioned to have BPF operating as GRO engine which might also help to reduce attack surface by only having to handle packets of interest for the concrete production use case. Perhaps here meta data buffer could be used to pass a notification from XDP to exit early w/o aggregation.

Quoted 65 lines · Daniel Borkmann

Quoted 56 lines · Willem de Bruijn

Quoted 42 lines · Yan Zhai

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two. On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by Yan Zhai <[email protected]>

---
   include/linux/netdevice.h |  9 +++++++--
   include/linux/skbuff.h    | 10 ++++++++++
   net/Kconfig               | 10 ++++++++++
   net/core/gro.c            |  2 +-
   net/core/gro_cells.c      |  2 +-
   net/core/skbuff.c         |  4 ++++
   6 files changed, 33 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -1

index c83b390191d4..2ca0870b1221 100644
@@ -2415,11 +2415,16 @@ struct net_device {
      ((dev)->devlink_port = (port));                         \
   })
 
-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)

include/linux/netdevice.h struct net_device { · line 2427

Quoted 10 lines

include/linux/netdevice.h +6 -1

   {
-    if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+    if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
              return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+    return skb->gro_disabled;
+#else
      return false;
+#endif

Yet more branches in the hot path.

Quoted 7 lines

Compile time configurability does not help, as that will be enabled by distros.

For a fairly niche use case. Where functionality of GRO already works. So just a performance for a very rare case at the cost of a regression in the common case. A small regression perhaps, but death by a thousand cuts.

Quoted 6 lines

Mentioning it here b/c it perhaps fits in this context, longer time ago there was the idea mentioned to have BPF operating as GRO engine which might also help to reduce attack surface by only having to handle packets of interest for the concrete production use case. Perhaps here meta data buffer could be used to pass a notification from XDP to exit early w/o aggregation.

Metadata is in fact one of our interests as well. We discussed using metadata instead of a skb bit to carry this information internally. Since metadata is opaque atm so it seems the only option is to have a GRO control hook before napi_gro_receive, and let BPF decide netif_receive_skb or napi_gro_receive (echo what Paolo said). With BPF it could indeed be more flexible, but the cons is that it could be even more slower than taking a bit on skb. I am actually open to either approach, as long as it gives us more control on when to enable GRO :)

Quoted 73 lines · Yan Zhai

Quoted 65 lines · Daniel Borkmann

Quoted 56 lines · Willem de Bruijn

Quoted 42 lines · Yan Zhai

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two. On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by Yan Zhai <[email protected]>

---
   include/linux/netdevice.h |  9 +++++++--
   include/linux/skbuff.h    | 10 ++++++++++
   net/Kconfig               | 10 ++++++++++
   net/core/gro.c            |  2 +-
   net/core/gro_cells.c      |  2 +-
   net/core/skbuff.c         |  4 ++++
   6 files changed, 33 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -1

index c83b390191d4..2ca0870b1221 100644
@@ -2415,11 +2415,16 @@ struct net_device {
      ((dev)->devlink_port = (port));                         \
   })
 
-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)

include/linux/netdevice.h struct net_device { · line 2427

Quoted 10 lines

include/linux/netdevice.h +6 -1

   {
-    if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+    if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
              return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+    return skb->gro_disabled;
+#else
      return false;
+#endif

Yet more branches in the hot path.

Quoted 7 lines

Compile time configurability does not help, as that will be enabled by distros.

For a fairly niche use case. Where functionality of GRO already works. So just a performance for a very rare case at the cost of a regression in the common case. A small regression perhaps, but death by a thousand cuts.

Quoted 6 lines

Mentioning it here b/c it perhaps fits in this context, longer time ago there was the idea mentioned to have BPF operating as GRO engine which might also help to reduce attack surface by only having to handle packets of interest for the concrete production use case. Perhaps here meta data buffer could be used to pass a notification from XDP to exit early w/o aggregation.

Quoted 9 lines

Metadata is in fact one of our interests as well. We discussed using metadata instead of a skb bit to carry this information internally. Since metadata is opaque atm so it seems the only option is to have a GRO control hook before napi_gro_receive, and let BPF decide netif_receive_skb or napi_gro_receive (echo what Paolo said). With BPF it could indeed be more flexible, but the cons is that it could be even more slower than taking a bit on skb. I am actually open to either approach, as long as it gives us more control on when to enable GRO :)

Oh wait, one thing that just came to mind.. have you tried u64 per-CPU counter map in XDP? For packets which should not be GRO-aggregated you add count++ into the meta data area, and this forces GRO to not aggregate since meta data that needs to be transported to tc BPF layer mismatches (and therefore the contract/intent is that tc BPF needs to see the different meta data passed to it).

Thanks,
Daniel

Quoted 85 lines · Daniel Borkmann

Quoted 73 lines · Yan Zhai

Quoted 65 lines · Daniel Borkmann

Quoted 56 lines · Willem de Bruijn

Quoted 42 lines · Yan Zhai

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two. On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by Yan Zhai <[email protected]>

---
   include/linux/netdevice.h |  9 +++++++--
   include/linux/skbuff.h    | 10 ++++++++++
   net/Kconfig               | 10 ++++++++++
   net/core/gro.c            |  2 +-
   net/core/gro_cells.c      |  2 +-
   net/core/skbuff.c         |  4 ++++
   6 files changed, 33 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -1

index c83b390191d4..2ca0870b1221 100644
@@ -2415,11 +2415,16 @@ struct net_device {
      ((dev)->devlink_port = (port));                         \
   })
 
-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)

include/linux/netdevice.h struct net_device { · line 2427

Quoted 10 lines

include/linux/netdevice.h +6 -1

   {
-    if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+    if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
              return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+    return skb->gro_disabled;
+#else
      return false;
+#endif

Yet more branches in the hot path.

Quoted 7 lines

Compile time configurability does not help, as that will be enabled by distros.

For a fairly niche use case. Where functionality of GRO already works. So just a performance for a very rare case at the cost of a regression in the common case. A small regression perhaps, but death by a thousand cuts.

Quoted 6 lines

Mentioning it here b/c it perhaps fits in this context, longer time ago there was the idea mentioned to have BPF operating as GRO engine which might also help to reduce attack surface by only having to handle packets of interest for the concrete production use case. Perhaps here meta data buffer could be used to pass a notification from XDP to exit early w/o aggregation.

Quoted 9 lines

Metadata is in fact one of our interests as well. We discussed using metadata instead of a skb bit to carry this information internally. Since metadata is opaque atm so it seems the only option is to have a GRO control hook before napi_gro_receive, and let BPF decide netif_receive_skb or napi_gro_receive (echo what Paolo said). With BPF it could indeed be more flexible, but the cons is that it could be even more slower than taking a bit on skb. I am actually open to either approach, as long as it gives us more control on when to enable GRO :)

Quoted 7 lines

Oh wait, one thing that just came to mind.. have you tried u64 per-CPU counter map in XDP? For packets which should not be GRO-aggregated you add count++ into the meta data area, and this forces GRO to not aggregate since meta data that needs to be transported to tc BPF layer mismatches (and therefore the contract/intent is that tc BPF needs to see the different meta data passed to it).

We did this before accidentally (we put a timestamp for debugging purposes in metadata) and this actually caused about 20% of OoO for TCP in production: all PSH packets are reordered. GRO does not fire the packet to the upper layer when a diff in metadata is found for a non-PSH packet, instead it is queued as a “new flow” on the GRO list and waits for flushing. When a PSH packet arrives, its semantic is to flush this packet immediately and thus precedes earlier packets of the same flow.

Quoted 92 lines · Yan Zhai

Quoted 84 lines · Daniel Borkmann

Quoted 73 lines · Yan Zhai

Quoted 65 lines · Daniel Borkmann

Quoted 56 lines · Willem de Bruijn

Quoted 42 lines · Yan Zhai

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two. On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by Yan Zhai <[email protected]>

---
    include/linux/netdevice.h |  9 +++++++--
    include/linux/skbuff.h    | 10 ++++++++++
    net/Kconfig               | 10 ++++++++++
    net/core/gro.c            |  2 +-
    net/core/gro_cells.c      |  2 +-
    net/core/skbuff.c         |  4 ++++
    6 files changed, 33 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -1

index c83b390191d4..2ca0870b1221 100644
@@ -2415,11 +2415,16 @@ struct net_device {
       ((dev)->devlink_port = (port));                         \
    })
 
-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)

include/linux/netdevice.h struct net_device { · line 2427

Quoted 10 lines

include/linux/netdevice.h +6 -1

    {
-    if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+    if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
               return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+    return skb->gro_disabled;
+#else
       return false;
+#endif

Yet more branches in the hot path.

Quoted 7 lines

Compile time configurability does not help, as that will be enabled by distros.

For a fairly niche use case. Where functionality of GRO already works. So just a performance for a very rare case at the cost of a regression in the common case. A small regression perhaps, but death by a thousand cuts.

Quoted 6 lines

Mentioning it here b/c it perhaps fits in this context, longer time ago there was the idea mentioned to have BPF operating as GRO engine which might also help to reduce attack surface by only having to handle packets of interest for the concrete production use case. Perhaps here meta data buffer could be used to pass a notification from XDP to exit early w/o aggregation.

Quoted 9 lines

Metadata is in fact one of our interests as well. We discussed using metadata instead of a skb bit to carry this information internally. Since metadata is opaque atm so it seems the only option is to have a GRO control hook before napi_gro_receive, and let BPF decide netif_receive_skb or napi_gro_receive (echo what Paolo said). With BPF it could indeed be more flexible, but the cons is that it could be even more slower than taking a bit on skb. I am actually open to either approach, as long as it gives us more control on when to enable GRO :)

Quoted 6 lines

Oh wait, one thing that just came to mind.. have you tried u64 per-CPU counter map in XDP? For packets which should not be GRO-aggregated you add count++ into the meta data area, and this forces GRO to not aggregate since meta data that needs to be transported to tc BPF layer mismatches (and therefore the contract/intent is that tc BPF needs to see the different meta data passed to it).

Quoted 8 lines

We did this before accidentally (we put a timestamp for debugging purposes in metadata) and this actually caused about 20% of OoO for TCP in production: all PSH packets are reordered. GRO does not fire the packet to the upper layer when a diff in metadata is found for a non-PSH packet, instead it is queued as a “new flow” on the GRO list and waits for flushing. When a PSH packet arrives, its semantic is to flush this packet immediately and thus precedes earlier packets of the same flow.

Is that a bug in XDP metadata handling for GRO?

Mismatching metadata should not be taken as separate flows, but as a flush condition.

Quoted 102 lines · Willem de Bruijn

Quoted 100 lines · Yan Zhai

Quoted 84 lines · Daniel Borkmann

Quoted 73 lines · Yan Zhai

Quoted 65 lines · Daniel Borkmann

Quoted 56 lines · Willem de Bruijn

Quoted 42 lines · Yan Zhai

Software GRO is currently controlled by a single switch, i.e.

ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the kernel cannot GRO certain traffic, it has to run through the GRO receive handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For example, our production environment has a scenario that a TC egress hook may add multiple encapsulation headers to forwarded skbs for load balancing and isolation purpose. The encapsulation is implemented via BPF. But the problem arises then: there is no way to properly offload a double-encapsulated packet, since skb only has network_header and inner_network_header to track one layer of encapsulation, but not two. On the other hand, not all the traffic through this device needs double encapsulation. But we have to turn off GRO completely for any ingress device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by Yan Zhai <[email protected]>

---
    include/linux/netdevice.h |  9 +++++++--
    include/linux/skbuff.h    | 10 ++++++++++
    net/Kconfig               | 10 ++++++++++
    net/core/gro.c            |  2 +-
    net/core/gro_cells.c      |  2 +-
    net/core/skbuff.c         |  4 ++++
    6 files changed, 33 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -1

index c83b390191d4..2ca0870b1221 100644
@@ -2415,11 +2415,16 @@ struct net_device {
       ((dev)->devlink_port = (port));                         \
    })
 
-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)

include/linux/netdevice.h struct net_device { · line 2427

Quoted 10 lines

include/linux/netdevice.h +6 -1

    {
-    if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+    if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
               return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+    return skb->gro_disabled;
+#else
       return false;
+#endif

Yet more branches in the hot path.

Quoted 7 lines

Compile time configurability does not help, as that will be enabled by distros.

For a fairly niche use case. Where functionality of GRO already works. So just a performance for a very rare case at the cost of a regression in the common case. A small regression perhaps, but death by a thousand cuts.

Quoted 6 lines

Mentioning it here b/c it perhaps fits in this context, longer time ago there was the idea mentioned to have BPF operating as GRO engine which might also help to reduce attack surface by only having to handle packets of interest for the concrete production use case. Perhaps here meta data buffer could be used to pass a notification from XDP to exit early w/o aggregation.

Quoted 9 lines

Metadata is in fact one of our interests as well. We discussed using metadata instead of a skb bit to carry this information internally. Since metadata is opaque atm so it seems the only option is to have a GRO control hook before napi_gro_receive, and let BPF decide netif_receive_skb or napi_gro_receive (echo what Paolo said). With BPF it could indeed be more flexible, but the cons is that it could be even more slower than taking a bit on skb. I am actually open to either approach, as long as it gives us more control on when to enable GRO :)

Quoted 6 lines

Oh wait, one thing that just came to mind.. have you tried u64 per-CPU counter map in XDP? For packets which should not be GRO-aggregated you add count++ into the meta data area, and this forces GRO to not aggregate since meta data that needs to be transported to tc BPF layer mismatches (and therefore the contract/intent is that tc BPF needs to see the different meta data passed to it).

We did this before accidentally (we put a timestamp for debugging purposes in metadata) and this actually caused about 20% of OoO for TCP in production: all PSH packets are reordered. GRO does not fire the packet to the upper layer when a diff in metadata is found for a non-PSH packet, instead it is queued as a “new flow” on the GRO list and waits for flushing. When a PSH packet arrives, its semantic is to flush this packet immediately and thus precedes earlier packets of the same flow.

Quoted 4 lines

Is that a bug in XDP metadata handling for GRO?

Mismatching metadata should not be taken as separate flows, but as a flush condition.

Definitely a bug as it should flush. If noone is faster I can add it to my backlog todo to fix it, but might probably take a week before I get to it.

Thanks,
Daniel

include/linux/netdevice.h struct net_device { · line 2427

Quoted 12 lines

include/linux/netdevice.h +7 -2

-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)
 {
-     if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+     if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
              return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+     return skb->gro_disabled;
+#else
      return false;
+#endif

Yet more branches in the hot path.

Compile time configurability does not help, as that will be enabled by distros.

For a fairly niche use case. Where functionality of GRO already works. So just a performance for a very rare case at the cost of a regression in the common case. A small regression perhaps, but death by a thousand cuts.

Quoted 18 lines

include/linux/netdevice.h struct net_device { · line 2427

Quoted 12 lines

include/linux/netdevice.h +7 -2

-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)
 {
-     if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+     if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
              return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+     return skb->gro_disabled;
+#else
      return false;
+#endif

Yet more branches in the hot path.

Compile time configurability does not help, as that will be enabled by distros.

Quoted 5 lines

For a fairly niche use case. Where functionality of GRO already works. So just a performance for a very rare case at the cost of a regression in the common case. A small regression perhaps, but death by a thousand cuts.

I share your concern on operating on this hotpath. Will a static_branch + sysctl make it less aggressive?

Speaking of
performance, I'd hope this can give us more control so we can achieve the best of two worlds: for TCP and some UDP traffic, we can enable GRO, while for some other classes that we know GRO does no good or even harm, let's disable GRO to save more cycles. The key observation is that developers may already know which traffic is blessed by GRO, but lack a way to realize it.

best
Yan

Quoted 18 lines · Yan Zhai

Quoted 18 lines

include/linux/netdevice.h struct net_device { · line 2427

Quoted 12 lines

include/linux/netdevice.h +7 -2

-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)
 {
-     if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+     if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
              return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+     return skb->gro_disabled;
+#else
      return false;
+#endif

Yet more branches in the hot path.

Compile time configurability does not help, as that will be enabled by distros.

Quoted 8 lines

Quoted 5 lines

For a fairly niche use case. Where functionality of GRO already works. So just a performance for a very rare case at the cost of a regression in the common case. A small regression perhaps, but death by a thousand cuts.

I share your concern on operating on this hotpath. Will a static_branch + sysctl make it less aggressive?

That is always a possibility. But we have to use it judiciously, cannot add a sysctl for every branch.

I'm still of the opinion that Paolo shared that this seems a lot of complexity for a fairly minor performance optimization for a rare case.

Quoted 7 lines

Speaking of
performance, I'd hope this can give us more control so we can achieve the best of two worlds: for TCP and some UDP traffic, we can enable GRO, while for some other classes that we know GRO does no good or even harm, let's disable GRO to save more cycles. The key observation is that developers may already know which traffic is blessed by GRO, but lack a way to realize it.

Following up also on Daniel's point on using BPF as GRO engine. Even earlier I tried to add an option to selectively enable GRO protocols without BPF. Definitely worthwhile to be able to disable GRO handlers to reduce attack surface to bad input.

Quoted 4 lines

best
Yan

Quoted 29 lines · Willem de Bruijn

Quoted 18 lines · Yan Zhai

Quoted 18 lines

include/linux/netdevice.h struct net_device { · line 2427

Quoted 12 lines

include/linux/netdevice.h +7 -2

-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)
 {
-     if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+     if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
              return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+     return skb->gro_disabled;
+#else
      return false;
+#endif

Yet more branches in the hot path.

Compile time configurability does not help, as that will be enabled by distros.

Quoted 8 lines

Quoted 5 lines

For a fairly niche use case. Where functionality of GRO already works. So just a performance for a very rare case at the cost of a regression in the common case. A small regression perhaps, but death by a thousand cuts.

I share your concern on operating on this hotpath. Will a static_branch + sysctl make it less aggressive?

Quoted 7 lines

That is always a possibility. But we have to use it judiciously, cannot add a sysctl for every branch.

I'm still of the opinion that Paolo shared that this seems a lot of complexity for a fairly minor performance optimization for a rare case.

Actually combining the discussion in this thread, I think it would be more than the corner cases that we encounter. Let me elaborate below.

Quoted 8 lines

Quoted 7 lines

Speaking of
performance, I'd hope this can give us more control so we can achieve the best of two worlds: for TCP and some UDP traffic, we can enable GRO, while for some other classes that we know GRO does no good or even harm, let's disable GRO to save more cycles. The key observation is that developers may already know which traffic is blessed by GRO, but lack a way to realize it.

Quoted 5 lines

Following up also on Daniel's point on using BPF as GRO engine. Even earlier I tried to add an option to selectively enable GRO protocols without BPF. Definitely worthwhile to be able to disable GRO handlers to reduce attack surface to bad input.

I was probably staring too hard at my own things, which is indeed a corner case. But reducing the attack surface is indeed a good motivation for this patch. I checked briefly with our DoS team today, the DoS scenario will definitely benefit from skipping GRO, for example on SYN/RST floods. XDP is our main weapon to drop attack traffic today, but it does not always drop 100% of the floods, and time by time it does need to fall back to iptables due to the delay of XDP program assembly or the BPF limitation on analyzing the packet. I did an ad hoc measurement just now on a mostly idle server, with ~1.3Mpps SYN flood concentrated on one CPU and dropped them early in raw-PREROUTING. w/ GRO this would consume about 35-41% of the CPU time, while w/o GRO the time dropped to 9-12%. This seems a pretty significant breath room under heavy attacks.

But I am not sure I understand "BPF as GRO engine" here, it seems to me that being able to disable GRO by XDP is already good enough. Any more motivations to do more complex work here?

best
Yan

Quoted 6 lines

Quoted 4 lines

best
Yan

Quoted 54 lines · Yan Zhai

Quoted 29 lines · Willem de Bruijn

Quoted 18 lines · Yan Zhai

Quoted 18 lines

include/linux/netdevice.h struct net_device { · line 2427

Quoted 12 lines

include/linux/netdevice.h +7 -2

-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)
 {
-     if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+     if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
              return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+     return skb->gro_disabled;
+#else
      return false;
+#endif

Yet more branches in the hot path.

Compile time configurability does not help, as that will be enabled by distros.

Quoted 8 lines

Quoted 5 lines

For a fairly niche use case. Where functionality of GRO already works. So just a performance for a very rare case at the cost of a regression in the common case. A small regression perhaps, but death by a thousand cuts.

I share your concern on operating on this hotpath. Will a static_branch + sysctl make it less aggressive?

Quoted 7 lines

That is always a possibility. But we have to use it judiciously, cannot add a sysctl for every branch.

I'm still of the opinion that Paolo shared that this seems a lot of complexity for a fairly minor performance optimization for a rare case.

Actually combining the discussion in this thread, I think it would be more than the corner cases that we encounter. Let me elaborate below.

Quoted 8 lines

Quoted 7 lines

Speaking of
performance, I'd hope this can give us more control so we can achieve the best of two worlds: for TCP and some UDP traffic, we can enable GRO, while for some other classes that we know GRO does no good or even harm, let's disable GRO to save more cycles. The key observation is that developers may already know which traffic is blessed by GRO, but lack a way to realize it.

Quoted 5 lines

Following up also on Daniel's point on using BPF as GRO engine. Even earlier I tried to add an option to selectively enable GRO protocols without BPF. Definitely worthwhile to be able to disable GRO handlers to reduce attack surface to bad input.

Quoted 13 lines

I was probably staring too hard at my own things, which is indeed a corner case. But reducing the attack surface is indeed a good motivation for this patch. I checked briefly with our DoS team today, the DoS scenario will definitely benefit from skipping GRO, for example on SYN/RST floods. XDP is our main weapon to drop attack traffic today, but it does not always drop 100% of the floods, and time by time it does need to fall back to iptables due to the delay of XDP program assembly or the BPF limitation on analyzing the packet. I did an ad hoc measurement just now on a mostly idle server, with ~1.3Mpps SYN flood concentrated on one CPU and dropped them early in raw-PREROUTING. w/ GRO this would consume about 35-41% of the CPU time, while w/o GRO the time dropped to 9-12%. This seems a pretty significant breath room under heavy attacks.

A GRO opt-out might make sense.

A long time ago I sent a patch that configured GRO protocols using syscalls, selectively (un)registering handlers. The interface was not very nice, so I did not pursue it further. On the upside, the datapath did not introduce any extra code. The intent was to reduce attack surface of packet parsing code.

A few concerns with an XDP based opt-out. It is more work to enable:
requires compiling and load an XDP program. It adds cycles in the hot path. And I do not entirely understand when an XDP program will be able to detect that a packet should not enter the GRO engine, but cannot drop the packet (your netfilter example above).

Quoted 3 lines

But I am not sure I understand "BPF as GRO engine" here, it seems to me that being able to disable GRO by XDP is already good enough. Any more motivations to do more complex work here?

FWIW, we looked into this a few years ago. Analogous to the BPF flow
dissector: if the BPF program is loaded, use that instead of the C code path. But we did not arrive at a practical implementation at the time. Things may have changed, but one issue is how to store and access the list (or table) of outstanding GRO skbs.

Quoted 9 lines

best
Yan

Quoted 6 lines

Quoted 4 lines

best
Yan

Allow XDP program to set XDP_FLAGS_GRO_DISABLED flag in xdp_buff and xdp_frame, and disable GRO when building an sk_buff if this flag is set.

Signed-off-by Yan Zhai <[email protected]>

---
 include/net/xdp.h | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

include/net/xdp.h +0 -0

index e6770dd40c91..cc3bce8817b0 100644

include/net/xdp.h +6 -0

 };
 
 struct xdp_buff {

@@ -113,12 +114,35 @@ static __always_inline void xdp_buff_set_frag_pfmemalloc(struct xdp_buff *xdp)
 	xdp->flags |= XDP_FLAGS_FRAGS_PF_MEMALLOC;
 }
 
+static __always_inline void xdp_buff_disable_gro(struct xdp_buff *xdp)
+{
+	xdp->flags |= XDP_FLAGS_GRO_DISABLED;
+}
+
+static __always_inline bool xdp_buff_gro_disabled(struct xdp_buff *xdp)

include/net/xdp.h +3 -1

+
 static __always_inline void
 xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq)
 {
 	xdp->frame_sz = frame_sz;
 	xdp->rxq = rxq;
-	xdp->flags = 0;
+	xdp_init_buff_minimal(xdp);
 }
 
 static __always_inline void

@@ -187,6 +211,18 @@ static __always_inline bool xdp_frame_is_frag_pfmemalloc(struct xdp_frame *frame
 	return !!(frame->flags & XDP_FLAGS_FRAGS_PF_MEMALLOC);
 }
 
+static __always_inline bool xdp_frame_gro_disabled(struct xdp_frame *frame)

From: Yan Zhai <[email protected]>
Date: Thu, 20 Jun 2024 15:19:13 -0700

Quoted 13 lines

Allow XDP program to set XDP_FLAGS_GRO_DISABLED flag in xdp_buff and xdp_frame, and disable GRO when building an sk_buff if this flag is set.

Signed-off-by Yan Zhai <[email protected]>

---
 include/net/xdp.h | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

include/net/xdp.h +0 -0

index e6770dd40c91..cc3bce8817b0 100644

include/net/xdp.h enum xdp_buff_flags { · line 78

Quoted 4 lines

include/net/xdp.h +1 -0

@@ -75,6 +75,7 @@ enum xdp_buff_flags {
      XDP_FLAGS_FRAGS_PF_MEMALLOC     = BIT(1), /* xdp paged memory is under
                                                 * pressure
                                                 */
+     XDP_FLAGS_GRO_DISABLED          = BIT(2), /* GRO disabled */

There should be tabs, not spaces.

Quoted 13 lines

include/net/xdp.h +6 -0

 };
 
 struct xdp_buff {

@@ -113,12 +114,35 @@ static __always_inline void xdp_buff_set_frag_pfmemalloc(struct xdp_buff *xdp)
      xdp->flags |= XDP_FLAGS_FRAGS_PF_MEMALLOC;
 }
 
+static __always_inline void xdp_buff_disable_gro(struct xdp_buff *xdp)
+{
+     xdp->flags |= XDP_FLAGS_GRO_DISABLED;
+}
+
+static __always_inline bool xdp_buff_gro_disabled(struct xdp_buff *xdp)

include/net/xdp.h static __always_inline void xdp_buff_set_frag_pfmemalloc(struct xdp_buff *xdp) · line 132

Quoted 10 lines

include/net/xdp.h +10 -0

+{
+     return !!(xdp->flags & XDP_FLAGS_GRO_DISABLED);
+}
+
+static __always_inline void
+xdp_buff_fixup_skb_offloading(struct xdp_buff *xdp, struct sk_buff *skb)
+{
+     if (xdp_buff_gro_disabled(xdp))
+             skb_disable_gro(skb);
+}

I don't think this should be named "fixup". "propagate", "update", "promote", ...?

Maybe if is not needed here?

        skb->gro_disabled = xdp_buff_gro_disabled(xdp)

?

include/net/xdp.h static __always_inline void xdp_buff_set_frag_pfmemalloc(struct xdp_buff *xdp) · line 138

Quoted 6 lines

include/net/xdp.h +6 -0

+
+static __always_inline void
+xdp_init_buff_minimal(struct xdp_buff *xdp)
+{
+     xdp->flags = 0;
+}

"xdp_buff_clear_flags"?

Quoted 16 lines

include/net/xdp.h +3 -1

+
 static __always_inline void
 xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq)
 {
      xdp->frame_sz = frame_sz;
      xdp->rxq = rxq;
-     xdp->flags = 0;
+     xdp_init_buff_minimal(xdp);
 }
 
 static __always_inline void

@@ -187,6 +211,18 @@ static __always_inline bool xdp_frame_is_frag_pfmemalloc(struct xdp_frame *frame
      return !!(frame->flags & XDP_FLAGS_FRAGS_PF_MEMALLOC);
 }
 
+static __always_inline bool xdp_frame_gro_disabled(struct xdp_frame *frame)

include/net/xdp.h static __always_inline bool xdp_frame_is_frag_pfmemalloc(struct xdp_frame *frame · line 224

Quoted 10 lines

include/net/xdp.h +10 -0

+{
+     return !!(frame->flags & XDP_FLAGS_GRO_DISABLED);
+}
+
+static __always_inline void
+xdp_frame_fixup_skb_offloading(struct xdp_frame *frame, struct sk_buff *skb)
+{
+     if (xdp_frame_gro_disabled(frame))
+             skb_disable_gro(skb);
+}

(same)

include/net/xdp.h static __always_inline bool xdp_frame_is_frag_pfmemalloc(struct xdp_frame *frame · line 228

Quoted 4 lines

include/net/xdp.h +1 -0

+
 #define XDP_BULK_QUEUE_SIZE  16
 struct xdp_frame_bulk {
      int count;

Thanks,
Olek

Add a common point to transfer offloading info from XDP context to skb.

Signed-off-by Yan Zhai <[email protected]>

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
 drivers/net/ethernet/intel/ice/ice_txrx.c | 2 ++
 drivers/net/ethernet/intel/ice/ice_xsk.c  | 6 +++++-
 include/net/xdp_sock_drv.h                | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

drivers/net/ethernet/intel/ice/ice_txrx.c +0 -0

index 8bb743f78fcb..a247306837ed 100644

drivers/net/ethernet/intel/ice/ice_txrx.c +1 -0

 			xdp_prepare_buff(xdp, hard_start, offset, size, !!offset);
 #if (PAGE_SIZE > 4096)
 			/* At larger PAGE_SIZE, frame_sz depend on len size */

@@ -1287,6 +1288,7 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget)
 
 		/* populate checksum, VLAN, and protocol */
 		ice_process_skb_fields(rx_ring, rx_desc, skb);
+		xdp_buff_fixup_skb_offloading(xdp, skb);
 
 		ice_trace(clean_rx_irq_indicate, rx_ring, rx_desc, skb);
 		/* send completed skb up the stack */

drivers/net/ethernet/intel/ice/ice_xsk.c +0 -0

index a65955eb23c0..367658acaab8 100644

drivers/net/ethernet/intel/ice/ice_xsk.c +3 -0

+	}
 
 	while (likely(total_rx_packets < (unsigned int)budget)) {
 		union ice_32b_rx_flex_desc *rx_desc;

@@ -920,6 +922,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 			break;
 		}
 
+		xdp = first;
 		first = NULL;
 		rx_ring->first_desc = ntc;
 

@@ -934,6 +937,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 		vlan_tci = ice_get_vlan_tci(rx_desc);
 
 		ice_process_skb_fields(rx_ring, rx_desc, skb);
+		xdp_buff_fixup_skb_offloading(xdp, skb);
 		ice_receive_skb(rx_ring, skb, vlan_tci);
 	}
 

include/net/xdp_sock_drv.h +0 -0

index 0a5dca2b2b3f..02243dc064c2 100644

From: Yan Zhai <[email protected]>
Date: Thu, 20 Jun 2024 15:19:22 -0700

Quoted 15 lines

Add a common point to transfer offloading info from XDP context to skb.

Signed-off-by Yan Zhai <[email protected]>

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
 drivers/net/ethernet/intel/ice/ice_txrx.c | 2 ++
 drivers/net/ethernet/intel/ice/ice_xsk.c  | 6 +++++-
 include/net/xdp_sock_drv.h                | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

drivers/net/ethernet/intel/ice/ice_txrx.c +0 -0

index 8bb743f78fcb..a247306837ed 100644

drivers/net/ethernet/intel/ice/ice_txrx.c int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget) · line 1225

Quoted 4 lines

drivers/net/ethernet/intel/ice/ice_txrx.c +1 -0

@@ -1222,6 +1222,7 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget)
 
                      hard_start = page_address(rx_buf->page) + rx_buf->page_offset -
                                   offset;
+                     xdp_init_buff_minimal(xdp);

Two lines below, you have this:

        xdp_buff_clear_frags_flag(xdp);

Which clears frags bit in xdp->flags. I.e. since you always clear flags here, this call becomes redundant.
But I'd say that xdp->flags = 0 really wants to be moved from xdp_init_buff() to xdp_prepare_buff().

Quoted 16 lines

drivers/net/ethernet/intel/ice/ice_txrx.c +1 -0

                      xdp_prepare_buff(xdp, hard_start, offset, size, !!offset);
 #if (PAGE_SIZE > 4096)
                      /* At larger PAGE_SIZE, frame_sz depend on len size */

@@ -1287,6 +1288,7 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget)
 
              /* populate checksum, VLAN, and protocol */
              ice_process_skb_fields(rx_ring, rx_desc, skb);
+             xdp_buff_fixup_skb_offloading(xdp, skb);
 
              ice_trace(clean_rx_irq_indicate, rx_ring, rx_desc, skb);
              /* send completed skb up the stack */

drivers/net/ethernet/intel/ice/ice_xsk.c +0 -0

index a65955eb23c0..367658acaab8 100644

drivers/net/ethernet/intel/ice/ice_xsk.c int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget) · line 850

Quoted 7 lines

drivers/net/ethernet/intel/ice/ice_xsk.c +2 -1

@@ -845,8 +845,10 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
      xdp_prog = READ_ONCE(rx_ring->xdp_prog);
      xdp_ring = rx_ring->xdp_ring;
 
-     if (ntc != rx_ring->first_desc)
+     if (ntc != rx_ring->first_desc) {
              first = *ice_xdp_buf(rx_ring, rx_ring->first_desc);
+             xdp_init_buff_minimal(first);

xdp_buff_set_size() always clears flags, this is redundant.

Quoted 20 lines

drivers/net/ethernet/intel/ice/ice_xsk.c +3 -0

+     }
 
      while (likely(total_rx_packets < (unsigned int)budget)) {
              union ice_32b_rx_flex_desc *rx_desc;

@@ -920,6 +922,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
                      break;
              }
 
+             xdp = first;
              first = NULL;
              rx_ring->first_desc = ntc;
 

@@ -934,6 +937,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
              vlan_tci = ice_get_vlan_tci(rx_desc);
 
              ice_process_skb_fields(rx_ring, rx_desc, skb);
+             xdp_buff_fixup_skb_offloading(xdp, skb);
              ice_receive_skb(rx_ring, skb, vlan_tci);
      }
 

include/net/xdp_sock_drv.h static inline void xsk_buff_set_size(struct xdp_buff *xdp, u32 size) · line 184

Quoted 10 lines

include/net/xdp_sock_drv.h +1 -1

index 0a5dca2b2b3f..02243dc064c2 100644
@@ -181,7 +181,7 @@ static inline void xsk_buff_set_size(struct xdp_buff *xdp, u32 size)
      xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
      xdp->data_meta = xdp->data;
      xdp->data_end = xdp->data + size;
-     xdp->flags = 0;
+     xdp_init_buff_minimal(xdp);

Why is this done in the patch prefixed with "ice:"?

include/net/xdp_sock_drv.h static inline void xsk_buff_set_size(struct xdp_buff *xdp, u32 size) · line 187

Quoted 3 lines

include/net/xdp_sock_drv.h +0 -0

 }
 
 static inline dma_addr_t xsk_buff_raw_get_dma(struct xsk_buff_pool *pool,

Thanks,
Olek