| Message ID | e82117190648e1cbb2740be44de71a21351c5107.1682848658.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] bonding: add xdp_features support | expand |
On Sun, Apr 30, 2023 at 12:02:44PM +0200, Lorenzo Bianconi wrote: > Introduce xdp_features support for bonding driver according to the slave > devices attached to the master one. xdp_features is required whenever we > want to xdp_redirect traffic into a bond device and then into selected > slaves attached to it. > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> Reviewed-by: Simon Horman <simon.horman@corigine.com>
Lorenzo Bianconi <lorenzo@kernel.org> wrote: >Introduce xdp_features support for bonding driver according to the slave >devices attached to the master one. xdp_features is required whenever we >want to xdp_redirect traffic into a bond device and then into selected >slaves attached to it. > >Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") >Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> The patch looks ok to me, but the description sounds more like feature enablement rather than a bug fix as the "Fixes:" tag and net tree suggest. Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com> -J >--- >Change since v1: >- remove bpf self-test patch from the series >--- > drivers/net/bonding/bond_main.c | 48 ++++++++++++++++++++++++++++++ > drivers/net/bonding/bond_options.c | 2 ++ > include/net/bonding.h | 1 + > 3 files changed, 51 insertions(+) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 710548dbd0c1..c98121b426a4 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -1789,6 +1789,45 @@ static void bond_ether_setup(struct net_device *bond_dev) > bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; > } > >+void bond_xdp_set_features(struct net_device *bond_dev) >+{ >+ struct bonding *bond = netdev_priv(bond_dev); >+ xdp_features_t val = NETDEV_XDP_ACT_MASK; >+ struct list_head *iter; >+ struct slave *slave; >+ >+ ASSERT_RTNL(); >+ >+ if (!bond_xdp_check(bond)) { >+ xdp_clear_features_flag(bond_dev); >+ return; >+ } >+ >+ bond_for_each_slave(bond, slave, iter) { >+ struct net_device *dev = slave->dev; >+ >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_BASIC)) { >+ xdp_clear_features_flag(bond_dev); >+ return; >+ } >+ >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_REDIRECT)) >+ val &= ~NETDEV_XDP_ACT_REDIRECT; >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT)) >+ val &= ~NETDEV_XDP_ACT_NDO_XMIT; >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_XSK_ZEROCOPY)) >+ val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY; >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_HW_OFFLOAD)) >+ val &= ~NETDEV_XDP_ACT_HW_OFFLOAD; >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_RX_SG)) >+ val &= ~NETDEV_XDP_ACT_RX_SG; >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG)) >+ val &= ~NETDEV_XDP_ACT_NDO_XMIT_SG; >+ } >+ >+ xdp_set_features_flag(bond_dev, val); >+} >+ > /* enslave device <slave> to bond device <master> */ > int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > struct netlink_ext_ack *extack) >@@ -2236,6 +2275,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > bpf_prog_inc(bond->xdp_prog); > } > >+ bond_xdp_set_features(bond_dev); >+ > slave_info(bond_dev, slave_dev, "Enslaving as %s interface with %s link\n", > bond_is_active_slave(new_slave) ? "an active" : "a backup", > new_slave->link != BOND_LINK_DOWN ? "an up" : "a down"); >@@ -2483,6 +2524,7 @@ static int __bond_release_one(struct net_device *bond_dev, > if (!netif_is_bond_master(slave_dev)) > slave_dev->priv_flags &= ~IFF_BONDING; > >+ bond_xdp_set_features(bond_dev); > kobject_put(&slave->kobj); > > return 0; >@@ -3930,6 +3972,9 @@ static int bond_slave_netdev_event(unsigned long event, > /* Propagate to master device */ > call_netdevice_notifiers(event, slave->bond->dev); > break; >+ case NETDEV_XDP_FEAT_CHANGE: >+ bond_xdp_set_features(bond_dev); >+ break; > default: > break; > } >@@ -5874,6 +5919,9 @@ void bond_setup(struct net_device *bond_dev) > if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) > bond_dev->features |= BOND_XFRM_FEATURES; > #endif /* CONFIG_XFRM_OFFLOAD */ >+ >+ if (bond_xdp_check(bond)) >+ bond_dev->xdp_features = NETDEV_XDP_ACT_MASK; > } > > /* Destroy a bonding device. >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c >index f71d5517f829..0498fc6731f8 100644 >--- a/drivers/net/bonding/bond_options.c >+++ b/drivers/net/bonding/bond_options.c >@@ -877,6 +877,8 @@ static int bond_option_mode_set(struct bonding *bond, > netdev_update_features(bond->dev); > } > >+ bond_xdp_set_features(bond->dev); >+ > return 0; > } > >diff --git a/include/net/bonding.h b/include/net/bonding.h >index c3843239517d..a60a24923b55 100644 >--- a/include/net/bonding.h >+++ b/include/net/bonding.h >@@ -659,6 +659,7 @@ void bond_destroy_sysfs(struct bond_net *net); > void bond_prepare_sysfs_group(struct bonding *bond); > int bond_sysfs_slave_add(struct slave *slave); > void bond_sysfs_slave_del(struct slave *slave); >+void bond_xdp_set_features(struct net_device *bond_dev); > int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > struct netlink_ext_ack *extack); > int bond_release(struct net_device *bond_dev, struct net_device *slave_dev); >-- >2.40.0 --- -Jay Vosburgh, jay.vosburgh@canonical.com
> Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > >Introduce xdp_features support for bonding driver according to the slave > >devices attached to the master one. xdp_features is required whenever we > >want to xdp_redirect traffic into a bond device and then into selected > >slaves attached to it. > > > >Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") > >Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > The patch looks ok to me, but the description sounds more like > feature enablement rather than a bug fix as the "Fixes:" tag and net > tree suggest. > > Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com> Hi Jay, I think it is a fix because otherwise XDP_REDIRECT into a bond device is failing and the feature is already in Linus's tree. Regards, Lorenzo > > -J > > >--- > >Change since v1: > >- remove bpf self-test patch from the series > >--- > > drivers/net/bonding/bond_main.c | 48 ++++++++++++++++++++++++++++++ > > drivers/net/bonding/bond_options.c | 2 ++ > > include/net/bonding.h | 1 + > > 3 files changed, 51 insertions(+) > > > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > >index 710548dbd0c1..c98121b426a4 100644 > >--- a/drivers/net/bonding/bond_main.c > >+++ b/drivers/net/bonding/bond_main.c > >@@ -1789,6 +1789,45 @@ static void bond_ether_setup(struct net_device *bond_dev) > > bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; > > } > > > >+void bond_xdp_set_features(struct net_device *bond_dev) > >+{ > >+ struct bonding *bond = netdev_priv(bond_dev); > >+ xdp_features_t val = NETDEV_XDP_ACT_MASK; > >+ struct list_head *iter; > >+ struct slave *slave; > >+ > >+ ASSERT_RTNL(); > >+ > >+ if (!bond_xdp_check(bond)) { > >+ xdp_clear_features_flag(bond_dev); > >+ return; > >+ } > >+ > >+ bond_for_each_slave(bond, slave, iter) { > >+ struct net_device *dev = slave->dev; > >+ > >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_BASIC)) { > >+ xdp_clear_features_flag(bond_dev); > >+ return; > >+ } > >+ > >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_REDIRECT)) > >+ val &= ~NETDEV_XDP_ACT_REDIRECT; > >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT)) > >+ val &= ~NETDEV_XDP_ACT_NDO_XMIT; > >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_XSK_ZEROCOPY)) > >+ val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY; > >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_HW_OFFLOAD)) > >+ val &= ~NETDEV_XDP_ACT_HW_OFFLOAD; > >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_RX_SG)) > >+ val &= ~NETDEV_XDP_ACT_RX_SG; > >+ if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG)) > >+ val &= ~NETDEV_XDP_ACT_NDO_XMIT_SG; > >+ } > >+ > >+ xdp_set_features_flag(bond_dev, val); > >+} > >+ > > /* enslave device <slave> to bond device <master> */ > > int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > > struct netlink_ext_ack *extack) > >@@ -2236,6 +2275,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > > bpf_prog_inc(bond->xdp_prog); > > } > > > >+ bond_xdp_set_features(bond_dev); > >+ > > slave_info(bond_dev, slave_dev, "Enslaving as %s interface with %s link\n", > > bond_is_active_slave(new_slave) ? "an active" : "a backup", > > new_slave->link != BOND_LINK_DOWN ? "an up" : "a down"); > >@@ -2483,6 +2524,7 @@ static int __bond_release_one(struct net_device *bond_dev, > > if (!netif_is_bond_master(slave_dev)) > > slave_dev->priv_flags &= ~IFF_BONDING; > > > >+ bond_xdp_set_features(bond_dev); > > kobject_put(&slave->kobj); > > > > return 0; > >@@ -3930,6 +3972,9 @@ static int bond_slave_netdev_event(unsigned long event, > > /* Propagate to master device */ > > call_netdevice_notifiers(event, slave->bond->dev); > > break; > >+ case NETDEV_XDP_FEAT_CHANGE: > >+ bond_xdp_set_features(bond_dev); > >+ break; > > default: > > break; > > } > >@@ -5874,6 +5919,9 @@ void bond_setup(struct net_device *bond_dev) > > if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) > > bond_dev->features |= BOND_XFRM_FEATURES; > > #endif /* CONFIG_XFRM_OFFLOAD */ > >+ > >+ if (bond_xdp_check(bond)) > >+ bond_dev->xdp_features = NETDEV_XDP_ACT_MASK; > > } > > > > /* Destroy a bonding device. > >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c > >index f71d5517f829..0498fc6731f8 100644 > >--- a/drivers/net/bonding/bond_options.c > >+++ b/drivers/net/bonding/bond_options.c > >@@ -877,6 +877,8 @@ static int bond_option_mode_set(struct bonding *bond, > > netdev_update_features(bond->dev); > > } > > > >+ bond_xdp_set_features(bond->dev); > >+ > > return 0; > > } > > > >diff --git a/include/net/bonding.h b/include/net/bonding.h > >index c3843239517d..a60a24923b55 100644 > >--- a/include/net/bonding.h > >+++ b/include/net/bonding.h > >@@ -659,6 +659,7 @@ void bond_destroy_sysfs(struct bond_net *net); > > void bond_prepare_sysfs_group(struct bonding *bond); > > int bond_sysfs_slave_add(struct slave *slave); > > void bond_sysfs_slave_del(struct slave *slave); > >+void bond_xdp_set_features(struct net_device *bond_dev); > > int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > > struct netlink_ext_ack *extack); > > int bond_release(struct net_device *bond_dev, struct net_device *slave_dev); > >-- > >2.40.0 > > --- > -Jay Vosburgh, jay.vosburgh@canonical.com
On 4/30/23 12:02 PM, Lorenzo Bianconi wrote: > Introduce xdp_features support for bonding driver according to the slave > devices attached to the master one. xdp_features is required whenever we > want to xdp_redirect traffic into a bond device and then into selected > slaves attached to it. > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> Please also keep Jussi in Cc for bonding + XDP reviews [added here]. > --- > Change since v1: > - remove bpf self-test patch from the series Given you targeted net tree, was this patch run against BPF CI locally from your side to avoid breakage again? Thanks, Daniel > --- > drivers/net/bonding/bond_main.c | 48 ++++++++++++++++++++++++++++++ > drivers/net/bonding/bond_options.c | 2 ++ > include/net/bonding.h | 1 + > 3 files changed, 51 insertions(+) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 710548dbd0c1..c98121b426a4 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -1789,6 +1789,45 @@ static void bond_ether_setup(struct net_device *bond_dev) > bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; > } > > +void bond_xdp_set_features(struct net_device *bond_dev) > +{ > + struct bonding *bond = netdev_priv(bond_dev); > + xdp_features_t val = NETDEV_XDP_ACT_MASK; > + struct list_head *iter; > + struct slave *slave; > + > + ASSERT_RTNL(); > + > + if (!bond_xdp_check(bond)) { > + xdp_clear_features_flag(bond_dev); > + return; > + } > + > + bond_for_each_slave(bond, slave, iter) { > + struct net_device *dev = slave->dev; > + > + if (!(dev->xdp_features & NETDEV_XDP_ACT_BASIC)) { > + xdp_clear_features_flag(bond_dev); > + return; > + } > + > + if (!(dev->xdp_features & NETDEV_XDP_ACT_REDIRECT)) > + val &= ~NETDEV_XDP_ACT_REDIRECT; > + if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT)) > + val &= ~NETDEV_XDP_ACT_NDO_XMIT; > + if (!(dev->xdp_features & NETDEV_XDP_ACT_XSK_ZEROCOPY)) > + val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY; > + if (!(dev->xdp_features & NETDEV_XDP_ACT_HW_OFFLOAD)) > + val &= ~NETDEV_XDP_ACT_HW_OFFLOAD; > + if (!(dev->xdp_features & NETDEV_XDP_ACT_RX_SG)) > + val &= ~NETDEV_XDP_ACT_RX_SG; > + if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG)) > + val &= ~NETDEV_XDP_ACT_NDO_XMIT_SG; > + } > + > + xdp_set_features_flag(bond_dev, val); > +} > + > /* enslave device <slave> to bond device <master> */ > int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > struct netlink_ext_ack *extack) > @@ -2236,6 +2275,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > bpf_prog_inc(bond->xdp_prog); > } > > + bond_xdp_set_features(bond_dev); > + > slave_info(bond_dev, slave_dev, "Enslaving as %s interface with %s link\n", > bond_is_active_slave(new_slave) ? "an active" : "a backup", > new_slave->link != BOND_LINK_DOWN ? "an up" : "a down"); > @@ -2483,6 +2524,7 @@ static int __bond_release_one(struct net_device *bond_dev, > if (!netif_is_bond_master(slave_dev)) > slave_dev->priv_flags &= ~IFF_BONDING; > > + bond_xdp_set_features(bond_dev); > kobject_put(&slave->kobj); > > return 0; > @@ -3930,6 +3972,9 @@ static int bond_slave_netdev_event(unsigned long event, > /* Propagate to master device */ > call_netdevice_notifiers(event, slave->bond->dev); > break; > + case NETDEV_XDP_FEAT_CHANGE: > + bond_xdp_set_features(bond_dev); > + break; > default: > break; > } > @@ -5874,6 +5919,9 @@ void bond_setup(struct net_device *bond_dev) > if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) > bond_dev->features |= BOND_XFRM_FEATURES; > #endif /* CONFIG_XFRM_OFFLOAD */ > + > + if (bond_xdp_check(bond)) > + bond_dev->xdp_features = NETDEV_XDP_ACT_MASK; > } > > /* Destroy a bonding device. > diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c > index f71d5517f829..0498fc6731f8 100644 > --- a/drivers/net/bonding/bond_options.c > +++ b/drivers/net/bonding/bond_options.c > @@ -877,6 +877,8 @@ static int bond_option_mode_set(struct bonding *bond, > netdev_update_features(bond->dev); > } > > + bond_xdp_set_features(bond->dev); > + > return 0; > } > > diff --git a/include/net/bonding.h b/include/net/bonding.h > index c3843239517d..a60a24923b55 100644 > --- a/include/net/bonding.h > +++ b/include/net/bonding.h > @@ -659,6 +659,7 @@ void bond_destroy_sysfs(struct bond_net *net); > void bond_prepare_sysfs_group(struct bonding *bond); > int bond_sysfs_slave_add(struct slave *slave); > void bond_sysfs_slave_del(struct slave *slave); > +void bond_xdp_set_features(struct net_device *bond_dev); > int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > struct netlink_ext_ack *extack); > int bond_release(struct net_device *bond_dev, struct net_device *slave_dev); >
> On 4/30/23 12:02 PM, Lorenzo Bianconi wrote: > > Introduce xdp_features support for bonding driver according to the slave > > devices attached to the master one. xdp_features is required whenever we > > want to xdp_redirect traffic into a bond device and then into selected > > slaves attached to it. > > > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > Please also keep Jussi in Cc for bonding + XDP reviews [added here]. ack, will do > > > --- > > Change since v1: > > - remove bpf self-test patch from the series > > Given you targeted net tree, was this patch run against BPF CI locally from > your side to avoid breakage again? yes, I tested it locally and opening a PR upstream [0] (upstream xdp bonding tests are fine but the PR fails, however the issue seems not related to the code I added and the error is in common even with some previous pending PR). Regards, Lorenzo [0] https://github.com/kernel-patches/bpf/pull/5021 > > Thanks, > Daniel > > > --- > > drivers/net/bonding/bond_main.c | 48 ++++++++++++++++++++++++++++++ > > drivers/net/bonding/bond_options.c | 2 ++ > > include/net/bonding.h | 1 + > > 3 files changed, 51 insertions(+) > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > index 710548dbd0c1..c98121b426a4 100644 > > --- a/drivers/net/bonding/bond_main.c > > +++ b/drivers/net/bonding/bond_main.c > > @@ -1789,6 +1789,45 @@ static void bond_ether_setup(struct net_device *bond_dev) > > bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; > > } > > +void bond_xdp_set_features(struct net_device *bond_dev) > > +{ > > + struct bonding *bond = netdev_priv(bond_dev); > > + xdp_features_t val = NETDEV_XDP_ACT_MASK; > > + struct list_head *iter; > > + struct slave *slave; > > + > > + ASSERT_RTNL(); > > + > > + if (!bond_xdp_check(bond)) { > > + xdp_clear_features_flag(bond_dev); > > + return; > > + } > > + > > + bond_for_each_slave(bond, slave, iter) { > > + struct net_device *dev = slave->dev; > > + > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_BASIC)) { > > + xdp_clear_features_flag(bond_dev); > > + return; > > + } > > + > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_REDIRECT)) > > + val &= ~NETDEV_XDP_ACT_REDIRECT; > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT)) > > + val &= ~NETDEV_XDP_ACT_NDO_XMIT; > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_XSK_ZEROCOPY)) > > + val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY; > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_HW_OFFLOAD)) > > + val &= ~NETDEV_XDP_ACT_HW_OFFLOAD; > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_RX_SG)) > > + val &= ~NETDEV_XDP_ACT_RX_SG; > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG)) > > + val &= ~NETDEV_XDP_ACT_NDO_XMIT_SG; > > + } > > + > > + xdp_set_features_flag(bond_dev, val); > > +} > > + > > /* enslave device <slave> to bond device <master> */ > > int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > > struct netlink_ext_ack *extack) > > @@ -2236,6 +2275,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > > bpf_prog_inc(bond->xdp_prog); > > } > > + bond_xdp_set_features(bond_dev); > > + > > slave_info(bond_dev, slave_dev, "Enslaving as %s interface with %s link\n", > > bond_is_active_slave(new_slave) ? "an active" : "a backup", > > new_slave->link != BOND_LINK_DOWN ? "an up" : "a down"); > > @@ -2483,6 +2524,7 @@ static int __bond_release_one(struct net_device *bond_dev, > > if (!netif_is_bond_master(slave_dev)) > > slave_dev->priv_flags &= ~IFF_BONDING; > > + bond_xdp_set_features(bond_dev); > > kobject_put(&slave->kobj); > > return 0; > > @@ -3930,6 +3972,9 @@ static int bond_slave_netdev_event(unsigned long event, > > /* Propagate to master device */ > > call_netdevice_notifiers(event, slave->bond->dev); > > break; > > + case NETDEV_XDP_FEAT_CHANGE: > > + bond_xdp_set_features(bond_dev); > > + break; > > default: > > break; > > } > > @@ -5874,6 +5919,9 @@ void bond_setup(struct net_device *bond_dev) > > if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) > > bond_dev->features |= BOND_XFRM_FEATURES; > > #endif /* CONFIG_XFRM_OFFLOAD */ > > + > > + if (bond_xdp_check(bond)) > > + bond_dev->xdp_features = NETDEV_XDP_ACT_MASK; > > } > > /* Destroy a bonding device. > > diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c > > index f71d5517f829..0498fc6731f8 100644 > > --- a/drivers/net/bonding/bond_options.c > > +++ b/drivers/net/bonding/bond_options.c > > @@ -877,6 +877,8 @@ static int bond_option_mode_set(struct bonding *bond, > > netdev_update_features(bond->dev); > > } > > + bond_xdp_set_features(bond->dev); > > + > > return 0; > > } > > diff --git a/include/net/bonding.h b/include/net/bonding.h > > index c3843239517d..a60a24923b55 100644 > > --- a/include/net/bonding.h > > +++ b/include/net/bonding.h > > @@ -659,6 +659,7 @@ void bond_destroy_sysfs(struct bond_net *net); > > void bond_prepare_sysfs_group(struct bonding *bond); > > int bond_sysfs_slave_add(struct slave *slave); > > void bond_sysfs_slave_del(struct slave *slave); > > +void bond_xdp_set_features(struct net_device *bond_dev); > > int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > > struct netlink_ext_ack *extack); > > int bond_release(struct net_device *bond_dev, struct net_device *slave_dev); > > >
On Mon, 2023-05-01 at 15:33 +0200, Daniel Borkmann wrote: > On 4/30/23 12:02 PM, Lorenzo Bianconi wrote: > > Introduce xdp_features support for bonding driver according to the slave > > devices attached to the master one. xdp_features is required whenever we > > want to xdp_redirect traffic into a bond device and then into selected > > slaves attached to it. > > > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > Please also keep Jussi in Cc for bonding + XDP reviews [added here]. Perhaps worth adding such info to the maintainer file for future memory? > > --- > > Change since v1: > > - remove bpf self-test patch from the series > > Given you targeted net tree, was this patch run against BPF CI locally from > your side to avoid breakage again? > > Thanks, > Daniel > > > --- > > drivers/net/bonding/bond_main.c | 48 ++++++++++++++++++++++++++++++ > > drivers/net/bonding/bond_options.c | 2 ++ > > include/net/bonding.h | 1 + > > 3 files changed, 51 insertions(+) > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > index 710548dbd0c1..c98121b426a4 100644 > > --- a/drivers/net/bonding/bond_main.c > > +++ b/drivers/net/bonding/bond_main.c > > @@ -1789,6 +1789,45 @@ static void bond_ether_setup(struct net_device *bond_dev) > > bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; > > } > > > > +void bond_xdp_set_features(struct net_device *bond_dev) > > +{ > > + struct bonding *bond = netdev_priv(bond_dev); > > + xdp_features_t val = NETDEV_XDP_ACT_MASK; > > + struct list_head *iter; > > + struct slave *slave; > > + > > + ASSERT_RTNL(); > > + > > + if (!bond_xdp_check(bond)) { > > + xdp_clear_features_flag(bond_dev); > > + return; > > + } > > + > > + bond_for_each_slave(bond, slave, iter) { > > + struct net_device *dev = slave->dev; > > + > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_BASIC)) { > > + xdp_clear_features_flag(bond_dev); > > + return; > > + } > > + > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_REDIRECT)) > > + val &= ~NETDEV_XDP_ACT_REDIRECT; > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT)) > > + val &= ~NETDEV_XDP_ACT_NDO_XMIT; > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_XSK_ZEROCOPY)) > > + val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY; > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_HW_OFFLOAD)) > > + val &= ~NETDEV_XDP_ACT_HW_OFFLOAD; > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_RX_SG)) > > + val &= ~NETDEV_XDP_ACT_RX_SG; > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG)) > > + val &= ~NETDEV_XDP_ACT_NDO_XMIT_SG; Can we expect NETDEV_XDP_ACT_MASK changing in the future (e.g. new features to be added)? If so the above code will break silently, as the new features will be unconditionally enabled. What about adding a BUILD_BUG() to catch such situation? > Cheers, Paolo
> On Mon, 2023-05-01 at 15:33 +0200, Daniel Borkmann wrote: > > On 4/30/23 12:02 PM, Lorenzo Bianconi wrote: > > > Introduce xdp_features support for bonding driver according to the slave > > > devices attached to the master one. xdp_features is required whenever we > > > want to xdp_redirect traffic into a bond device and then into selected > > > slaves attached to it. > > > > > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > > > Please also keep Jussi in Cc for bonding + XDP reviews [added here]. > > Perhaps worth adding such info to the maintainer file for future > memory? > > > > --- > > > Change since v1: > > > - remove bpf self-test patch from the series > > > > Given you targeted net tree, was this patch run against BPF CI locally from > > your side to avoid breakage again? > > > > Thanks, > > Daniel > > > > > --- > > > drivers/net/bonding/bond_main.c | 48 ++++++++++++++++++++++++++++++ > > > drivers/net/bonding/bond_options.c | 2 ++ > > > include/net/bonding.h | 1 + > > > 3 files changed, 51 insertions(+) > > > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > > index 710548dbd0c1..c98121b426a4 100644 > > > --- a/drivers/net/bonding/bond_main.c > > > +++ b/drivers/net/bonding/bond_main.c > > > @@ -1789,6 +1789,45 @@ static void bond_ether_setup(struct net_device *bond_dev) > > > bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; > > > } > > > > > > +void bond_xdp_set_features(struct net_device *bond_dev) > > > +{ > > > + struct bonding *bond = netdev_priv(bond_dev); > > > + xdp_features_t val = NETDEV_XDP_ACT_MASK; > > > + struct list_head *iter; > > > + struct slave *slave; > > > + > > > + ASSERT_RTNL(); > > > + > > > + if (!bond_xdp_check(bond)) { > > > + xdp_clear_features_flag(bond_dev); > > > + return; > > > + } > > > + > > > + bond_for_each_slave(bond, slave, iter) { > > > + struct net_device *dev = slave->dev; > > > + > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_BASIC)) { > > > + xdp_clear_features_flag(bond_dev); > > > + return; > > > + } > > > + > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_REDIRECT)) > > > + val &= ~NETDEV_XDP_ACT_REDIRECT; > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT)) > > > + val &= ~NETDEV_XDP_ACT_NDO_XMIT; > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_XSK_ZEROCOPY)) > > > + val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY; > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_HW_OFFLOAD)) > > > + val &= ~NETDEV_XDP_ACT_HW_OFFLOAD; > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_RX_SG)) > > > + val &= ~NETDEV_XDP_ACT_RX_SG; > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG)) > > > + val &= ~NETDEV_XDP_ACT_NDO_XMIT_SG; > > Can we expect NETDEV_XDP_ACT_MASK changing in the future (e.g. new > features to be added)? If so the above code will break silently, as the > new features will be unconditionally enabled. What about adding a > BUILD_BUG() to catch such situation? I used NETDEV_XDP_ACT_MASK here in order to enable all the XDP features when we do not have any slave device attache to the bond one. If we add a new feature to netdev_xdp_act in the future I would say it is fine we inherit it here otherwise we will need to explicitly add it. Regards, Lorenzo > > > > Cheers, > > Paolo >
> > On Mon, 2023-05-01 at 15:33 +0200, Daniel Borkmann wrote: > > > On 4/30/23 12:02 PM, Lorenzo Bianconi wrote: > > > > Introduce xdp_features support for bonding driver according to the slave > > > > devices attached to the master one. xdp_features is required whenever we > > > > want to xdp_redirect traffic into a bond device and then into selected > > > > slaves attached to it. > > > > > > > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > > > > > Please also keep Jussi in Cc for bonding + XDP reviews [added here]. > > > > Perhaps worth adding such info to the maintainer file for future > > memory? > > > > > > --- > > > > Change since v1: > > > > - remove bpf self-test patch from the series > > > > > > Given you targeted net tree, was this patch run against BPF CI locally from > > > your side to avoid breakage again? > > > > > > Thanks, > > > Daniel > > > > > > > --- > > > > drivers/net/bonding/bond_main.c | 48 ++++++++++++++++++++++++++++++ > > > > drivers/net/bonding/bond_options.c | 2 ++ > > > > include/net/bonding.h | 1 + > > > > 3 files changed, 51 insertions(+) > > > > > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > > > index 710548dbd0c1..c98121b426a4 100644 > > > > --- a/drivers/net/bonding/bond_main.c > > > > +++ b/drivers/net/bonding/bond_main.c > > > > @@ -1789,6 +1789,45 @@ static void bond_ether_setup(struct net_device *bond_dev) > > > > bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; > > > > } > > > > > > > > +void bond_xdp_set_features(struct net_device *bond_dev) > > > > +{ > > > > + struct bonding *bond = netdev_priv(bond_dev); > > > > + xdp_features_t val = NETDEV_XDP_ACT_MASK; > > > > + struct list_head *iter; > > > > + struct slave *slave; > > > > + > > > > + ASSERT_RTNL(); > > > > + > > > > + if (!bond_xdp_check(bond)) { > > > > + xdp_clear_features_flag(bond_dev); > > > > + return; > > > > + } > > > > + > > > > + bond_for_each_slave(bond, slave, iter) { > > > > + struct net_device *dev = slave->dev; > > > > + > > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_BASIC)) { > > > > + xdp_clear_features_flag(bond_dev); > > > > + return; > > > > + } > > > > + > > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_REDIRECT)) > > > > + val &= ~NETDEV_XDP_ACT_REDIRECT; > > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT)) > > > > + val &= ~NETDEV_XDP_ACT_NDO_XMIT; > > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_XSK_ZEROCOPY)) > > > > + val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY; > > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_HW_OFFLOAD)) > > > > + val &= ~NETDEV_XDP_ACT_HW_OFFLOAD; > > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_RX_SG)) > > > > + val &= ~NETDEV_XDP_ACT_RX_SG; > > > > + if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG)) > > > > + val &= ~NETDEV_XDP_ACT_NDO_XMIT_SG; > > > > Can we expect NETDEV_XDP_ACT_MASK changing in the future (e.g. new > > features to be added)? If so the above code will break silently, as the > > new features will be unconditionally enabled. What about adding a > > BUILD_BUG() to catch such situation? > > I used NETDEV_XDP_ACT_MASK here in order to enable all the XDP features when > we do not have any slave device attache to the bond one. If we add a new > feature to netdev_xdp_act in the future I would say it is fine we inherit it > here otherwise we will need to explicitly add it. > > Regards, > Lorenzo Looking again at the code we can generalize it a bit taking into account even new features added in the future, something like: diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index c98121b426a4..9c9f25c8f9bc 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1805,24 +1805,18 @@ void bond_xdp_set_features(struct net_device *bond_dev) bond_for_each_slave(bond, slave, iter) { struct net_device *dev = slave->dev; + int f; if (!(dev->xdp_features & NETDEV_XDP_ACT_BASIC)) { xdp_clear_features_flag(bond_dev); return; } - if (!(dev->xdp_features & NETDEV_XDP_ACT_REDIRECT)) - val &= ~NETDEV_XDP_ACT_REDIRECT; - if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT)) - val &= ~NETDEV_XDP_ACT_NDO_XMIT; - if (!(dev->xdp_features & NETDEV_XDP_ACT_XSK_ZEROCOPY)) - val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY; - if (!(dev->xdp_features & NETDEV_XDP_ACT_HW_OFFLOAD)) - val &= ~NETDEV_XDP_ACT_HW_OFFLOAD; - if (!(dev->xdp_features & NETDEV_XDP_ACT_RX_SG)) - val &= ~NETDEV_XDP_ACT_RX_SG; - if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG)) - val &= ~NETDEV_XDP_ACT_NDO_XMIT_SG; + for (f = NETDEV_XDP_ACT_REDIRECT; f < NETDEV_XDP_ACT_MASK; + f <<= 1) { + if (!(dev->xdp_features & f)) + val &= ~f; + } } xdp_set_features_flag(bond_dev, val); Regards, Lorenzo > > > > > > > > Cheers, > > > > Paolo > >
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 710548dbd0c1..c98121b426a4 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1789,6 +1789,45 @@ static void bond_ether_setup(struct net_device *bond_dev) bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; } +void bond_xdp_set_features(struct net_device *bond_dev) +{ + struct bonding *bond = netdev_priv(bond_dev); + xdp_features_t val = NETDEV_XDP_ACT_MASK; + struct list_head *iter; + struct slave *slave; + + ASSERT_RTNL(); + + if (!bond_xdp_check(bond)) { + xdp_clear_features_flag(bond_dev); + return; + } + + bond_for_each_slave(bond, slave, iter) { + struct net_device *dev = slave->dev; + + if (!(dev->xdp_features & NETDEV_XDP_ACT_BASIC)) { + xdp_clear_features_flag(bond_dev); + return; + } + + if (!(dev->xdp_features & NETDEV_XDP_ACT_REDIRECT)) + val &= ~NETDEV_XDP_ACT_REDIRECT; + if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT)) + val &= ~NETDEV_XDP_ACT_NDO_XMIT; + if (!(dev->xdp_features & NETDEV_XDP_ACT_XSK_ZEROCOPY)) + val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY; + if (!(dev->xdp_features & NETDEV_XDP_ACT_HW_OFFLOAD)) + val &= ~NETDEV_XDP_ACT_HW_OFFLOAD; + if (!(dev->xdp_features & NETDEV_XDP_ACT_RX_SG)) + val &= ~NETDEV_XDP_ACT_RX_SG; + if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG)) + val &= ~NETDEV_XDP_ACT_NDO_XMIT_SG; + } + + xdp_set_features_flag(bond_dev, val); +} + /* enslave device <slave> to bond device <master> */ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, struct netlink_ext_ack *extack) @@ -2236,6 +2275,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, bpf_prog_inc(bond->xdp_prog); } + bond_xdp_set_features(bond_dev); + slave_info(bond_dev, slave_dev, "Enslaving as %s interface with %s link\n", bond_is_active_slave(new_slave) ? "an active" : "a backup", new_slave->link != BOND_LINK_DOWN ? "an up" : "a down"); @@ -2483,6 +2524,7 @@ static int __bond_release_one(struct net_device *bond_dev, if (!netif_is_bond_master(slave_dev)) slave_dev->priv_flags &= ~IFF_BONDING; + bond_xdp_set_features(bond_dev); kobject_put(&slave->kobj); return 0; @@ -3930,6 +3972,9 @@ static int bond_slave_netdev_event(unsigned long event, /* Propagate to master device */ call_netdevice_notifiers(event, slave->bond->dev); break; + case NETDEV_XDP_FEAT_CHANGE: + bond_xdp_set_features(bond_dev); + break; default: break; } @@ -5874,6 +5919,9 @@ void bond_setup(struct net_device *bond_dev) if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) bond_dev->features |= BOND_XFRM_FEATURES; #endif /* CONFIG_XFRM_OFFLOAD */ + + if (bond_xdp_check(bond)) + bond_dev->xdp_features = NETDEV_XDP_ACT_MASK; } /* Destroy a bonding device. diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c index f71d5517f829..0498fc6731f8 100644 --- a/drivers/net/bonding/bond_options.c +++ b/drivers/net/bonding/bond_options.c @@ -877,6 +877,8 @@ static int bond_option_mode_set(struct bonding *bond, netdev_update_features(bond->dev); } + bond_xdp_set_features(bond->dev); + return 0; } diff --git a/include/net/bonding.h b/include/net/bonding.h index c3843239517d..a60a24923b55 100644 --- a/include/net/bonding.h +++ b/include/net/bonding.h @@ -659,6 +659,7 @@ void bond_destroy_sysfs(struct bond_net *net); void bond_prepare_sysfs_group(struct bonding *bond); int bond_sysfs_slave_add(struct slave *slave); void bond_sysfs_slave_del(struct slave *slave); +void bond_xdp_set_features(struct net_device *bond_dev); int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, struct netlink_ext_ack *extack); int bond_release(struct net_device *bond_dev, struct net_device *slave_dev);
Introduce xdp_features support for bonding driver according to the slave devices attached to the master one. xdp_features is required whenever we want to xdp_redirect traffic into a bond device and then into selected slaves attached to it. Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- Change since v1: - remove bpf self-test patch from the series --- drivers/net/bonding/bond_main.c | 48 ++++++++++++++++++++++++++++++ drivers/net/bonding/bond_options.c | 2 ++ include/net/bonding.h | 1 + 3 files changed, 51 insertions(+)