* Re: [PATCH v2 net] bonding: add xdp_features support
2023-04-30 10:02 [PATCH v2 net] bonding: add xdp_features support Lorenzo Bianconi
@ 2023-05-01 7:15 ` Simon Horman
2023-05-01 12:56 ` Jay Vosburgh
2023-05-01 13:33 ` Daniel Borkmann
2 siblings, 0 replies; 9+ messages in thread
From: Simon Horman @ 2023-05-01 7:15 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: netdev, lorenzo.bianconi, j.vosburgh, andy, davem, edumazet, kuba,
pabeni, bpf, andrii, mykolal, ast, daniel, martin.lau, alardam,
memxor, sdf, brouer, toke
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>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 net] bonding: add xdp_features support
2023-04-30 10:02 [PATCH v2 net] bonding: add xdp_features support Lorenzo Bianconi
2023-05-01 7:15 ` Simon Horman
@ 2023-05-01 12:56 ` Jay Vosburgh
2023-05-01 13:09 ` Lorenzo Bianconi
2023-05-01 13:33 ` Daniel Borkmann
2 siblings, 1 reply; 9+ messages in thread
From: Jay Vosburgh @ 2023-05-01 12:56 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: netdev, lorenzo.bianconi, andy, davem, edumazet, kuba, pabeni,
bpf, andrii, mykolal, ast, daniel, martin.lau, alardam, memxor,
sdf, brouer, toke
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 net] bonding: add xdp_features support
2023-05-01 12:56 ` Jay Vosburgh
@ 2023-05-01 13:09 ` Lorenzo Bianconi
0 siblings, 0 replies; 9+ messages in thread
From: Lorenzo Bianconi @ 2023-05-01 13:09 UTC (permalink / raw)
To: Jay Vosburgh
Cc: netdev, lorenzo.bianconi, andy, davem, edumazet, kuba, pabeni,
bpf, andrii, mykolal, ast, daniel, martin.lau, alardam, memxor,
sdf, brouer, toke
[-- Attachment #1: Type: text/plain, Size: 5488 bytes --]
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 net] bonding: add xdp_features support
2023-04-30 10:02 [PATCH v2 net] bonding: add xdp_features support Lorenzo Bianconi
2023-05-01 7:15 ` Simon Horman
2023-05-01 12:56 ` Jay Vosburgh
@ 2023-05-01 13:33 ` Daniel Borkmann
2023-05-01 14:50 ` Lorenzo Bianconi
2023-05-02 9:28 ` Paolo Abeni
2 siblings, 2 replies; 9+ messages in thread
From: Daniel Borkmann @ 2023-05-01 13:33 UTC (permalink / raw)
To: Lorenzo Bianconi, netdev
Cc: lorenzo.bianconi, j.vosburgh, andy, davem, edumazet, kuba, pabeni,
bpf, andrii, mykolal, ast, martin.lau, alardam, memxor, sdf,
brouer, toke, Jussi Maki
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);
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 net] bonding: add xdp_features support
2023-05-01 13:33 ` Daniel Borkmann
@ 2023-05-01 14:50 ` Lorenzo Bianconi
2023-05-02 9:28 ` Paolo Abeni
1 sibling, 0 replies; 9+ messages in thread
From: Lorenzo Bianconi @ 2023-05-01 14:50 UTC (permalink / raw)
To: Daniel Borkmann
Cc: netdev, lorenzo.bianconi, j.vosburgh, andy, davem, edumazet, kuba,
pabeni, bpf, andrii, mykolal, ast, martin.lau, alardam, memxor,
sdf, brouer, toke, Jussi Maki
[-- Attachment #1: Type: text/plain, Size: 5700 bytes --]
> 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);
> >
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 net] bonding: add xdp_features support
2023-05-01 13:33 ` Daniel Borkmann
2023-05-01 14:50 ` Lorenzo Bianconi
@ 2023-05-02 9:28 ` Paolo Abeni
2023-05-02 9:42 ` Lorenzo Bianconi
1 sibling, 1 reply; 9+ messages in thread
From: Paolo Abeni @ 2023-05-02 9:28 UTC (permalink / raw)
To: Daniel Borkmann, Lorenzo Bianconi, netdev
Cc: lorenzo.bianconi, j.vosburgh, andy, davem, edumazet, kuba, bpf,
andrii, mykolal, ast, martin.lau, alardam, memxor, sdf, brouer,
toke, Jussi Maki
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 net] bonding: add xdp_features support
2023-05-02 9:28 ` Paolo Abeni
@ 2023-05-02 9:42 ` Lorenzo Bianconi
2023-05-02 14:14 ` Lorenzo Bianconi
0 siblings, 1 reply; 9+ messages in thread
From: Lorenzo Bianconi @ 2023-05-02 9:42 UTC (permalink / raw)
To: Paolo Abeni
Cc: Daniel Borkmann, netdev, lorenzo.bianconi, j.vosburgh, andy,
davem, edumazet, kuba, bpf, andrii, mykolal, ast, martin.lau,
alardam, memxor, sdf, brouer, toke, Jussi Maki
[-- Attachment #1: Type: text/plain, Size: 3473 bytes --]
> 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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 net] bonding: add xdp_features support
2023-05-02 9:42 ` Lorenzo Bianconi
@ 2023-05-02 14:14 ` Lorenzo Bianconi
0 siblings, 0 replies; 9+ messages in thread
From: Lorenzo Bianconi @ 2023-05-02 14:14 UTC (permalink / raw)
To: Paolo Abeni
Cc: Daniel Borkmann, netdev, lorenzo.bianconi, j.vosburgh, andy,
davem, edumazet, kuba, bpf, andrii, mykolal, ast, martin.lau,
alardam, memxor, sdf, brouer, toke, Jussi Maki
[-- Attachment #1: Type: text/plain, Size: 5085 bytes --]
> > 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
> >
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread