| Message ID | 20180722151308.5480-4-toshiaki.makita1@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | veth: Driver XDP | expand |
On Mon, 23 Jul 2018 00:13:03 +0900, Toshiaki Makita wrote: > From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > > All oversized packets including GSO packets are dropped if XDP is > enabled on receiver side, so don't send such packets from peer. > > Drop TSO and SCTP fragmentation features so that veth devices themselves > segment packets with XDP enabled. Also cap MTU accordingly. > > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> Is there any precedence for fixing up features and MTU like this? Most drivers just refuse to install the program if settings are incompatible. > diff --git a/drivers/net/veth.c b/drivers/net/veth.c > index 78fa08cb6e24..f5b72e937d9d 100644 > --- a/drivers/net/veth.c > +++ b/drivers/net/veth.c > @@ -542,6 +542,23 @@ static int veth_get_iflink(const struct net_device *dev) > return iflink; > } > > +static netdev_features_t veth_fix_features(struct net_device *dev, > + netdev_features_t features) > +{ > + struct veth_priv *priv = netdev_priv(dev); > + struct net_device *peer; > + > + peer = rtnl_dereference(priv->peer); > + if (peer) { > + struct veth_priv *peer_priv = netdev_priv(peer); > + > + if (peer_priv->_xdp_prog) > + features &= ~NETIF_F_GSO_SOFTWARE; > + } > + > + return features; > +} > + > static void veth_set_rx_headroom(struct net_device *dev, int new_hr) > { > struct veth_priv *peer_priv, *priv = netdev_priv(dev); > @@ -591,14 +608,33 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, > goto err; > } > } > + > + if (!old_prog) { > + peer->hw_features &= ~NETIF_F_GSO_SOFTWARE; > + peer->max_mtu = PAGE_SIZE - VETH_XDP_HEADROOM - > + peer->hard_header_len - > + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > + if (peer->mtu > peer->max_mtu) > + dev_set_mtu(peer, peer->max_mtu); > + } > } > > if (old_prog) { > - if (!prog && dev->flags & IFF_UP) > - veth_disable_xdp(dev); > + if (!prog) { > + if (dev->flags & IFF_UP) > + veth_disable_xdp(dev); > + > + if (peer) { > + peer->hw_features |= NETIF_F_GSO_SOFTWARE; > + peer->max_mtu = ETH_MAX_MTU; > + } > + } > bpf_prog_put(old_prog); > } > > + if ((!!old_prog ^ !!prog) && peer) > + netdev_update_features(peer); > + > return 0; > err: > priv->_xdp_prog = old_prog; > @@ -643,6 +679,7 @@ static const struct net_device_ops veth_netdev_ops = { > .ndo_poll_controller = veth_poll_controller, > #endif > .ndo_get_iflink = veth_get_iflink, > + .ndo_fix_features = veth_fix_features, > .ndo_features_check = passthru_features_check, > .ndo_set_rx_headroom = veth_set_rx_headroom, > .ndo_bpf = veth_xdp,
On 2018/07/24 9:27, Jakub Kicinski wrote: > On Mon, 23 Jul 2018 00:13:03 +0900, Toshiaki Makita wrote: >> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> >> >> All oversized packets including GSO packets are dropped if XDP is >> enabled on receiver side, so don't send such packets from peer. >> >> Drop TSO and SCTP fragmentation features so that veth devices themselves >> segment packets with XDP enabled. Also cap MTU accordingly. >> >> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > > Is there any precedence for fixing up features and MTU like this? Most > drivers just refuse to install the program if settings are incompatible. I don't know any precedence. I can refuse the program on installing it when features and MTU are not appropriate. Is it preferred? Note that with current implementation wanted_features are not touched so features will be restored when the XDP program is removed. MTU will not be restored though, as I do not remember the original MTU. >> diff --git a/drivers/net/veth.c b/drivers/net/veth.c >> index 78fa08cb6e24..f5b72e937d9d 100644 >> --- a/drivers/net/veth.c >> +++ b/drivers/net/veth.c >> @@ -542,6 +542,23 @@ static int veth_get_iflink(const struct net_device *dev) >> return iflink; >> } >> >> +static netdev_features_t veth_fix_features(struct net_device *dev, >> + netdev_features_t features) >> +{ >> + struct veth_priv *priv = netdev_priv(dev); >> + struct net_device *peer; >> + >> + peer = rtnl_dereference(priv->peer); >> + if (peer) { >> + struct veth_priv *peer_priv = netdev_priv(peer); >> + >> + if (peer_priv->_xdp_prog) >> + features &= ~NETIF_F_GSO_SOFTWARE; >> + } >> + >> + return features; >> +} >> + >> static void veth_set_rx_headroom(struct net_device *dev, int new_hr) >> { >> struct veth_priv *peer_priv, *priv = netdev_priv(dev); >> @@ -591,14 +608,33 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, >> goto err; >> } >> } >> + >> + if (!old_prog) { >> + peer->hw_features &= ~NETIF_F_GSO_SOFTWARE; >> + peer->max_mtu = PAGE_SIZE - VETH_XDP_HEADROOM - >> + peer->hard_header_len - >> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); >> + if (peer->mtu > peer->max_mtu) >> + dev_set_mtu(peer, peer->max_mtu); >> + } >> } >> >> if (old_prog) { >> - if (!prog && dev->flags & IFF_UP) >> - veth_disable_xdp(dev); >> + if (!prog) { >> + if (dev->flags & IFF_UP) >> + veth_disable_xdp(dev); >> + >> + if (peer) { >> + peer->hw_features |= NETIF_F_GSO_SOFTWARE; >> + peer->max_mtu = ETH_MAX_MTU; >> + } >> + } >> bpf_prog_put(old_prog); >> } >> >> + if ((!!old_prog ^ !!prog) && peer) >> + netdev_update_features(peer); >> + >> return 0; >> err: >> priv->_xdp_prog = old_prog; >> @@ -643,6 +679,7 @@ static const struct net_device_ops veth_netdev_ops = { >> .ndo_poll_controller = veth_poll_controller, >> #endif >> .ndo_get_iflink = veth_get_iflink, >> + .ndo_fix_features = veth_fix_features, >> .ndo_features_check = passthru_features_check, >> .ndo_set_rx_headroom = veth_set_rx_headroom, >> .ndo_bpf = veth_xdp,
On 2018/07/24 10:56, Toshiaki Makita wrote: > On 2018/07/24 9:27, Jakub Kicinski wrote: >> On Mon, 23 Jul 2018 00:13:03 +0900, Toshiaki Makita wrote: >>> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> >>> >>> All oversized packets including GSO packets are dropped if XDP is >>> enabled on receiver side, so don't send such packets from peer. >>> >>> Drop TSO and SCTP fragmentation features so that veth devices themselves >>> segment packets with XDP enabled. Also cap MTU accordingly. >>> >>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> >> >> Is there any precedence for fixing up features and MTU like this? Most >> drivers just refuse to install the program if settings are incompatible. > > I don't know any precedence. I can refuse the program on installing it > when features and MTU are not appropriate. Is it preferred? > Note that with current implementation wanted_features are not touched so > features will be restored when the XDP program is removed. MTU will not > be restored though, as I do not remember the original MTU. I just recalled that virtio_net used to refused XDP when guest offload features are incompatible but now it dynamically fixup them on installing an XDP program. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3f93522ffab2d46a36b57adf324a54e674fc9536
On Tue, 24 Jul 2018 18:39:09 +0900, Toshiaki Makita wrote: > On 2018/07/24 10:56, Toshiaki Makita wrote: > > On 2018/07/24 9:27, Jakub Kicinski wrote: > >> On Mon, 23 Jul 2018 00:13:03 +0900, Toshiaki Makita wrote: > >>> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > >>> > >>> All oversized packets including GSO packets are dropped if XDP is > >>> enabled on receiver side, so don't send such packets from peer. > >>> > >>> Drop TSO and SCTP fragmentation features so that veth devices themselves > >>> segment packets with XDP enabled. Also cap MTU accordingly. > >>> > >>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > >> > >> Is there any precedence for fixing up features and MTU like this? Most > >> drivers just refuse to install the program if settings are incompatible. > > > > I don't know any precedence. I can refuse the program on installing it > > when features and MTU are not appropriate. Is it preferred? > > Note that with current implementation wanted_features are not touched so > > features will be restored when the XDP program is removed. MTU will not > > be restored though, as I do not remember the original MTU. > > I just recalled that virtio_net used to refused XDP when guest offload > features are incompatible but now it dynamically fixup them on > installing an XDP program. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3f93522ffab2d46a36b57adf324a54e674fc9536 That's slightly different AFAIU, because the virtio features weren't really controllable at runtime at all. I'm not dead set on leaving the features be, but I just want to make sure we think this through properly before we commit to any magic behaviour for ever... Taking a quick glance at the MTU side, it seems that today if someone decides to set MTU on one side of a veth pair the packets will simply get dropped. So the MTU coupling for XDP doesn't seem in line with existing behaviour of veth, not only other XDP drivers.
On 2018/07/25 4:10, Jakub Kicinski wrote: > On Tue, 24 Jul 2018 18:39:09 +0900, Toshiaki Makita wrote: >> On 2018/07/24 10:56, Toshiaki Makita wrote: >>> On 2018/07/24 9:27, Jakub Kicinski wrote: >>>> On Mon, 23 Jul 2018 00:13:03 +0900, Toshiaki Makita wrote: >>>>> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> >>>>> >>>>> All oversized packets including GSO packets are dropped if XDP is >>>>> enabled on receiver side, so don't send such packets from peer. >>>>> >>>>> Drop TSO and SCTP fragmentation features so that veth devices themselves >>>>> segment packets with XDP enabled. Also cap MTU accordingly. >>>>> >>>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> >>>> >>>> Is there any precedence for fixing up features and MTU like this? Most >>>> drivers just refuse to install the program if settings are incompatible. >>> >>> I don't know any precedence. I can refuse the program on installing it >>> when features and MTU are not appropriate. Is it preferred? >>> Note that with current implementation wanted_features are not touched so >>> features will be restored when the XDP program is removed. MTU will not >>> be restored though, as I do not remember the original MTU. >> >> I just recalled that virtio_net used to refused XDP when guest offload >> features are incompatible but now it dynamically fixup them on >> installing an XDP program. >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3f93522ffab2d46a36b57adf324a54e674fc9536 > > That's slightly different AFAIU, because the virtio features weren't > really controllable at runtime at all. I'm not dead set on leaving the > features be, but I just want to make sure we think this through > properly before we commit to any magic behaviour for ever... To me it does not look so different. What the above virtio commit is doing is almost disabling LRO so we could add a feature to toggle LRO instead but it chose to automatically disable it. And this veth commit is also almost equivalent to disabling LRO. IMHO we should do this feature adjustment. It just avoids packet drops and has no downside. It forces software segmentation on the peer veth. Features will be restored when XDP is removed so there would be no surprising for users. It seems there is no benefit to not doing this. > Taking a quick glance at the MTU side, it seems that today if someone > decides to set MTU on one side of a veth pair the packets will simply > get dropped. So the MTU coupling for XDP doesn't seem in line with > existing behaviour of veth, not only other XDP drivers. It looks weird to allow such inconsistent MTU settings. But anyway changing MTU can have negative effect on users, such as causing fragmentation or EMSGSIZE error on UDP sendmsg() or not restoring MTU. I think I should not adjust MTU but just cap max_mtu.
diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 78fa08cb6e24..f5b72e937d9d 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -542,6 +542,23 @@ static int veth_get_iflink(const struct net_device *dev) return iflink; } +static netdev_features_t veth_fix_features(struct net_device *dev, + netdev_features_t features) +{ + struct veth_priv *priv = netdev_priv(dev); + struct net_device *peer; + + peer = rtnl_dereference(priv->peer); + if (peer) { + struct veth_priv *peer_priv = netdev_priv(peer); + + if (peer_priv->_xdp_prog) + features &= ~NETIF_F_GSO_SOFTWARE; + } + + return features; +} + static void veth_set_rx_headroom(struct net_device *dev, int new_hr) { struct veth_priv *peer_priv, *priv = netdev_priv(dev); @@ -591,14 +608,33 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, goto err; } } + + if (!old_prog) { + peer->hw_features &= ~NETIF_F_GSO_SOFTWARE; + peer->max_mtu = PAGE_SIZE - VETH_XDP_HEADROOM - + peer->hard_header_len - + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); + if (peer->mtu > peer->max_mtu) + dev_set_mtu(peer, peer->max_mtu); + } } if (old_prog) { - if (!prog && dev->flags & IFF_UP) - veth_disable_xdp(dev); + if (!prog) { + if (dev->flags & IFF_UP) + veth_disable_xdp(dev); + + if (peer) { + peer->hw_features |= NETIF_F_GSO_SOFTWARE; + peer->max_mtu = ETH_MAX_MTU; + } + } bpf_prog_put(old_prog); } + if ((!!old_prog ^ !!prog) && peer) + netdev_update_features(peer); + return 0; err: priv->_xdp_prog = old_prog; @@ -643,6 +679,7 @@ static const struct net_device_ops veth_netdev_ops = { .ndo_poll_controller = veth_poll_controller, #endif .ndo_get_iflink = veth_get_iflink, + .ndo_fix_features = veth_fix_features, .ndo_features_check = passthru_features_check, .ndo_set_rx_headroom = veth_set_rx_headroom, .ndo_bpf = veth_xdp,