Re: [PATCH v2 net] bonding: add xdp_features support

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- 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 --]