[PATCH net v2 0/2] net: veth: ability to toggle GRO and XDP independently

[PATCH net v2 0/2] net: veth: ability to toggle GRO and XDP independently

[Ignat Korchagin]

It is rather confusing that GRO is automatically enabled, when an XDP program
is attached to a veth interface. Moreover, it is not possible to disable GRO
on a veth, if an XDP program is attached (which might be desirable in some use
cases).

Make GRO and XDP independent for a veth interface.

Ignat Korchagin (2):
  net: veth: do not manipulate GRO when using XDP
  selftests: net: veth: test the ability to independently manipulate GRO
    and XDP

 drivers/net/veth.c                  | 18 ------------------
 tools/testing/selftests/net/veth.sh | 24 +++++++++++++++++++++---
 2 files changed, 21 insertions(+), 21 deletions(-)

-- 
2.39.2

[PATCH net v2 1/2] net: veth: do not manipulate GRO when using XDP

[Ignat Korchagin]

Commit d3256efd8e8b ("veth: allow enabling NAPI even without XDP") tried to fix
the fact that GRO was not possible without XDP, because veth did not use NAPI
without XDP. However, it also introduced the behaviour that GRO is always
enabled, when XDP is enabled.

While it might be desired for most cases, it is confusing for the user at best
as the GRO flag suddenly changes, when an XDP program is attached. It also
introduces some complexities in state management as was partially addressed in
commit fe9f801355f0 ("net: veth: clear GRO when clearing XDP even when down").

But the biggest problem is that it is not possible to disable GRO at all, when
an XDP program is attached, which might be needed for some use cases.

Fix this by not touching the GRO flag on XDP enable/disable as the code already
supports switching to NAPI if either GRO or XDP is requested.

Changes in v2:
  * add Fixes reference to commit description
  * fix commit message spelling

Link: https://lore.kernel.org/lkml/20240311124015.38106-1-ignat@cloudflare.com/
Fixes: d3256efd8e8b ("veth: allow enabling NAPI even without XDP")
Fixes: fe9f801355f0 ("net: veth: clear GRO when clearing XDP even when down")
Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 drivers/net/veth.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index cd4a6fe458f9..f0b2c4d5fe43 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1533,8 +1533,6 @@ static netdev_features_t veth_fix_features(struct net_device *dev,
 		if (peer_priv->_xdp_prog)
 			features &= ~NETIF_F_GSO_SOFTWARE;
 	}
-	if (priv->_xdp_prog)
-		features |= NETIF_F_GRO;
 
 	return features;
 }
@@ -1638,14 +1636,6 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		}
 
 		if (!old_prog) {
-			if (!veth_gro_requested(dev)) {
-				/* user-space did not require GRO, but adding
-				 * XDP is supposed to get GRO working
-				 */
-				dev->features |= NETIF_F_GRO;
-				netdev_features_change(dev);
-			}
-
 			peer->hw_features &= ~NETIF_F_GSO_SOFTWARE;
 			peer->max_mtu = max_mtu;
 		}
@@ -1661,14 +1651,6 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 			if (dev->flags & IFF_UP)
 				veth_disable_xdp(dev);
 
-			/* if user-space did not require GRO, since adding XDP
-			 * enabled it, clear it now
-			 */
-			if (!veth_gro_requested(dev)) {
-				dev->features &= ~NETIF_F_GRO;
-				netdev_features_change(dev);
-			}
-
 			if (peer) {
 				peer->hw_features |= NETIF_F_GSO_SOFTWARE;
 				peer->max_mtu = ETH_MAX_MTU;
-- 
2.39.2

[PATCH net v2 2/2] selftests: net: veth: test the ability to independently manipulate GRO and XDP

[Ignat Korchagin]

We should be able to independently flip either XDP or GRO states and toggling
one should not affect the other.

Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 tools/testing/selftests/net/veth.sh | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/veth.sh b/tools/testing/selftests/net/veth.sh
index 5ae85def0739..3a394b43e274 100755
--- a/tools/testing/selftests/net/veth.sh
+++ b/tools/testing/selftests/net/veth.sh
@@ -249,9 +249,9 @@ cleanup
 create_ns
 ip -n $NS_DST link set dev veth$DST up
 ip -n $NS_DST link set dev veth$DST xdp object ${BPF_FILE} section xdp
-chk_gro_flag "gro vs xdp while down - gro flag on" $DST on
+chk_gro_flag "gro vs xdp while down - gro flag off" $DST off
 ip -n $NS_DST link set dev veth$DST down
-chk_gro_flag "                      - after down" $DST on
+chk_gro_flag "                      - after down" $DST off
 ip -n $NS_DST link set dev veth$DST xdp off
 chk_gro_flag "                      - after xdp off" $DST off
 ip -n $NS_DST link set dev veth$DST up
@@ -260,6 +260,21 @@ ip -n $NS_SRC link set dev veth$SRC xdp object ${BPF_FILE} section xdp
 chk_gro_flag "                      - after peer xdp" $DST off
 cleanup
 
+create_ns
+ip -n $NS_DST link set dev veth$DST up
+ip -n $NS_DST link set dev veth$DST xdp object ${BPF_FILE} section xdp
+ip netns exec $NS_DST ethtool -K veth$DST generic-receive-offload on
+chk_gro_flag "gro vs xdp while down - gro flag on" $DST on
+ip -n $NS_DST link set dev veth$DST down
+chk_gro_flag "                      - after down" $DST on
+ip -n $NS_DST link set dev veth$DST xdp off
+chk_gro_flag "                      - after xdp off" $DST on
+ip -n $NS_DST link set dev veth$DST up
+chk_gro_flag "                      - after up" $DST on
+ip -n $NS_SRC link set dev veth$SRC xdp object ${BPF_FILE} section xdp
+chk_gro_flag "                      - after peer xdp" $DST on
+cleanup
+
 create_ns
 chk_channels "default channels" $DST 1 1
 
@@ -327,11 +342,14 @@ if [ $CPUS -gt 2 ]; then
 fi
 
 ip -n $NS_DST link set dev veth$DST xdp object ${BPF_FILE} section xdp 2>/dev/null
-chk_gro_flag "with xdp attached - gro flag" $DST on
+chk_gro_flag "with xdp attached - gro flag" $DST off
 chk_gro_flag "        - peer gro flag" $SRC off
 chk_tso_flag "        - tso flag" $SRC off
 chk_tso_flag "        - peer tso flag" $DST on
 ip netns exec $NS_DST ethtool -K veth$DST rx-udp-gro-forwarding on
+chk_gro "        - no aggregation" 10
+ip netns exec $NS_DST ethtool -K veth$DST generic-receive-offload on
+chk_gro_flag "        - gro flag with GRO on" $DST on
 chk_gro "        - aggregation" 1
 
 
-- 
2.39.2

Re: [PATCH net v2 2/2] selftests: net: veth: test the ability to independently manipulate GRO and XDP

[Michal Kubiak]

On Tue, Mar 12, 2024 at 04:05:52PM +0000, Ignat Korchagin wrote:
> We should be able to independently flip either XDP or GRO states and toggling
> one should not affect the other.
> 
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>

Missing "Fixes" tag for the patch targeted to the "net" tree.

Thanks,
Michal

Re: [PATCH net v2 2/2] selftests: net: veth: test the ability to independently manipulate GRO and XDP

[Jakub Kicinski]

On Wed, 13 Mar 2024 12:28:41 +0100 Michal Kubiak wrote:
> On Tue, Mar 12, 2024 at 04:05:52PM +0000, Ignat Korchagin wrote:
> > We should be able to independently flip either XDP or GRO states and toggling
> > one should not affect the other.
> > 
> > Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>  
> 
> Missing "Fixes" tag for the patch targeted to the "net" tree.

it's adjusting a selftest, I don't think we need a Fixes tag for that

Re: [PATCH net v2 2/2] selftests: net: veth: test the ability to independently manipulate GRO and XDP

[Michal Kubiak]

On Wed, Mar 13, 2024 at 06:57:25AM -0700, Jakub Kicinski wrote:
> On Wed, 13 Mar 2024 12:28:41 +0100 Michal Kubiak wrote:
> > On Tue, Mar 12, 2024 at 04:05:52PM +0000, Ignat Korchagin wrote:
> > > We should be able to independently flip either XDP or GRO states and toggling
> > > one should not affect the other.
> > > 
> > > Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>  
> > 
> > Missing "Fixes" tag for the patch targeted to the "net" tree.
> 
> it's adjusting a selftest, I don't think we need a Fixes tag for that

OK, sorry! My mistake, then.

Michal

Re: [PATCH net v2 2/2] selftests: net: veth: test the ability to independently manipulate GRO and XDP

[Jakub Kicinski]

On Wed, 13 Mar 2024 15:03:16 +0100 Michal Kubiak wrote:
> > > Missing "Fixes" tag for the patch targeted to the "net" tree.  
> > 
> > it's adjusting a selftest, I don't think we need a Fixes tag for that  
> 
> OK, sorry! My mistake, then.

No worries, it's pretty subjective. And the patchwork check for
the Fixes tag doesn't do a great job on the corner cases either :(

Re: [PATCH net v2 0/2] net: veth: ability to toggle GRO and XDP independently

[Jakub Kicinski]

On Tue, 12 Mar 2024 16:05:49 +0000 Ignat Korchagin wrote:
> It is rather confusing that GRO is automatically enabled, when an XDP program
> is attached to a veth interface. Moreover, it is not possible to disable GRO
> on a veth, if an XDP program is attached (which might be desirable in some use
> cases).
> 
> Make GRO and XDP independent for a veth interface.

Looks like the udpgro_fwd.sh test also needs tweakin'

https://netdev-3.bots.linux.dev/vmksft-net/results/504620/17-udpgro-fwd-sh/stdout
-- 
pw-bot: cr

Re: [PATCH net v2 0/2] net: veth: ability to toggle GRO and XDP independently

[Ignat Korchagin]

On Wed, Mar 13, 2024 at 12:42 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 12 Mar 2024 16:05:49 +0000 Ignat Korchagin wrote:
> > It is rather confusing that GRO is automatically enabled, when an XDP program
> > is attached to a veth interface. Moreover, it is not possible to disable GRO
> > on a veth, if an XDP program is attached (which might be desirable in some use
> > cases).
> >
> > Make GRO and XDP independent for a veth interface.
>
> Looks like the udpgro_fwd.sh test also needs tweakin'

Sent a v3 with adjusted test

> https://netdev-3.bots.linux.dev/vmksft-net/results/504620/17-udpgro-fwd-sh/stdout
> --
> pw-bot: cr