| Message ID | 20240311124015.38106-1-ignat@cloudflare.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: veth: do not manipulate GRO when using XDP | expand |
On Mon, 11 Mar 2024 12:40:15 +0000 Ignat Korchagin wrote: > 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 sudddenly changes, when an XDP program is attached. It also s/ddd/dd/ spellcheck the whole message, pls > 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. Sound legit, AFAIU. But please also adjust tools/testing/selftests/net/veth.sh because it's failing now.
Ignat Korchagin <ignat@cloudflare.com> writes: > 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 sudddenly 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. Sounds reasonable Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
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;
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 sudddenly 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. Signed-off-by: Ignat Korchagin <ignat@cloudflare.com> --- drivers/net/veth.c | 18 ------------------ 1 file changed, 18 deletions(-)