diff mbox series

[net] bonding: do not report NETDEV_XDP_ACT_XSK_ZEROCOPY

Message ID 20240205123011.22036-1-magnus.karlsson@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] bonding: do not report NETDEV_XDP_ACT_XSK_ZEROCOPY | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1064 this patch: 1064
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 3 blamed authors not CCed: horms@kernel.org joamaki@gmail.com lorenzo@kernel.org; 3 maintainers not CCed: horms@kernel.org joamaki@gmail.com lorenzo@kernel.org
netdev/build_clang success Errors and warnings before: 1081 this patch: 1081
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1081 this patch: 1081
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 19 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-05--18-00 (tests: 720)

Commit Message

Magnus Karlsson Feb. 5, 2024, 12:30 p.m. UTC
From: Magnus Karlsson <magnus.karlsson@intel.com>

Do not report the XDP capability NETDEV_XDP_ACT_XSK_ZEROCOPY as the
bonding driver does not support XDP and AF_XDP in zero-copy mode even
if the real NIC drivers do.

Fixes: cb9e6e584d58 ("bonding: add xdp_features support")
Reported-by: Prashant Batra <prbatra.mail@gmail.com>
Link: https://lore.kernel.org/all/CAJ8uoz2ieZCopgqTvQ9ZY6xQgTbujmC6XkMTamhp68O-h_-rLg@mail.gmail.com/T/
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 drivers/net/bonding/bond_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)


base-commit: fdeba0b57d61b40a708de361294fde3e1495588d

Comments

Toke Høiland-Jørgensen Feb. 5, 2024, 1:08 p.m. UTC | #1
Magnus Karlsson <magnus.karlsson@gmail.com> writes:

> From: Magnus Karlsson <magnus.karlsson@intel.com>
>
> Do not report the XDP capability NETDEV_XDP_ACT_XSK_ZEROCOPY as the
> bonding driver does not support XDP and AF_XDP in zero-copy mode even
> if the real NIC drivers do.
>
> Fixes: cb9e6e584d58 ("bonding: add xdp_features support")
> Reported-by: Prashant Batra <prbatra.mail@gmail.com>
> Link: https://lore.kernel.org/all/CAJ8uoz2ieZCopgqTvQ9ZY6xQgTbujmC6XkMTamhp68O-h_-rLg@mail.gmail.com/T/
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
>  drivers/net/bonding/bond_main.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 4e0600c7b050..79a37bed097b 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1819,6 +1819,8 @@ void bond_xdp_set_features(struct net_device *bond_dev)
>  	bond_for_each_slave(bond, slave, iter)
>  		val &= slave->dev->xdp_features;
>  
> +	val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY;
> +
>  	xdp_set_features_flag(bond_dev, val);
>  }
>  
> @@ -5910,8 +5912,10 @@ void bond_setup(struct net_device *bond_dev)
>  		bond_dev->features |= BOND_XFRM_FEATURES;
>  #endif /* CONFIG_XFRM_OFFLOAD */
>  
> -	if (bond_xdp_check(bond))
> +	if (bond_xdp_check(bond)) {
>  		bond_dev->xdp_features = NETDEV_XDP_ACT_MASK;
> +		bond_dev->xdp_features &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY;
> +	}

Shouldn't we rather drop this assignment completely? It makes no sense
to default to all features, it should default to none...

-Toke
Magnus Karlsson Feb. 6, 2024, 9:55 a.m. UTC | #2
On Mon, 5 Feb 2024 at 14:08, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Magnus Karlsson <magnus.karlsson@gmail.com> writes:
>
> > From: Magnus Karlsson <magnus.karlsson@intel.com>
> >
> > Do not report the XDP capability NETDEV_XDP_ACT_XSK_ZEROCOPY as the
> > bonding driver does not support XDP and AF_XDP in zero-copy mode even
> > if the real NIC drivers do.
> >
> > Fixes: cb9e6e584d58 ("bonding: add xdp_features support")
> > Reported-by: Prashant Batra <prbatra.mail@gmail.com>
> > Link: https://lore.kernel.org/all/CAJ8uoz2ieZCopgqTvQ9ZY6xQgTbujmC6XkMTamhp68O-h_-rLg@mail.gmail.com/T/
> > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > ---
> >  drivers/net/bonding/bond_main.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index 4e0600c7b050..79a37bed097b 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -1819,6 +1819,8 @@ void bond_xdp_set_features(struct net_device *bond_dev)
> >       bond_for_each_slave(bond, slave, iter)
> >               val &= slave->dev->xdp_features;
> >
> > +     val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY;
> > +
> >       xdp_set_features_flag(bond_dev, val);
> >  }
> >
> > @@ -5910,8 +5912,10 @@ void bond_setup(struct net_device *bond_dev)
> >               bond_dev->features |= BOND_XFRM_FEATURES;
> >  #endif /* CONFIG_XFRM_OFFLOAD */
> >
> > -     if (bond_xdp_check(bond))
> > +     if (bond_xdp_check(bond)) {
> >               bond_dev->xdp_features = NETDEV_XDP_ACT_MASK;
> > +             bond_dev->xdp_features &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY;
> > +     }
>
> Shouldn't we rather drop this assignment completely? It makes no sense
> to default to all features, it should default to none...

Good point. Seems the bond device defaults to supporting everything
before a device is bonded to it, but I might have misunderstood
something. Lorenzo, could you enlighten us please?

Thanks: Magnus

> -Toke
>
Lorenzo Bianconi Feb. 6, 2024, 3:16 p.m. UTC | #3
> On Mon, 5 Feb 2024 at 14:08, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >
> > Magnus Karlsson <magnus.karlsson@gmail.com> writes:
> >
> > > From: Magnus Karlsson <magnus.karlsson@intel.com>
> > >
> > > Do not report the XDP capability NETDEV_XDP_ACT_XSK_ZEROCOPY as the
> > > bonding driver does not support XDP and AF_XDP in zero-copy mode even
> > > if the real NIC drivers do.
> > >
> > > Fixes: cb9e6e584d58 ("bonding: add xdp_features support")
> > > Reported-by: Prashant Batra <prbatra.mail@gmail.com>
> > > Link: https://lore.kernel.org/all/CAJ8uoz2ieZCopgqTvQ9ZY6xQgTbujmC6XkMTamhp68O-h_-rLg@mail.gmail.com/T/
> > > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > > ---
> > >  drivers/net/bonding/bond_main.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > index 4e0600c7b050..79a37bed097b 100644
> > > --- a/drivers/net/bonding/bond_main.c
> > > +++ b/drivers/net/bonding/bond_main.c
> > > @@ -1819,6 +1819,8 @@ void bond_xdp_set_features(struct net_device *bond_dev)
> > >       bond_for_each_slave(bond, slave, iter)
> > >               val &= slave->dev->xdp_features;
> > >
> > > +     val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY;
> > > +
> > >       xdp_set_features_flag(bond_dev, val);
> > >  }
> > >
> > > @@ -5910,8 +5912,10 @@ void bond_setup(struct net_device *bond_dev)
> > >               bond_dev->features |= BOND_XFRM_FEATURES;
> > >  #endif /* CONFIG_XFRM_OFFLOAD */
> > >
> > > -     if (bond_xdp_check(bond))
> > > +     if (bond_xdp_check(bond)) {
> > >               bond_dev->xdp_features = NETDEV_XDP_ACT_MASK;
> > > +             bond_dev->xdp_features &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY;
> > > +     }
> >
> > Shouldn't we rather drop this assignment completely? It makes no sense
> > to default to all features, it should default to none...
> 
> Good point. Seems the bond device defaults to supporting everything
> before a device is bonded to it, but I might have misunderstood
> something. Lorenzo, could you enlighten us please?

ack, I agree we can get rid of it since the xdp features will be calculated
again as soon as a new device is added to the bond.

Regards,
Lorenzo

> 
> Thanks: Magnus
> 
> > -Toke
> >
Magnus Karlsson Feb. 6, 2024, 3:56 p.m. UTC | #4
On Tue, 6 Feb 2024 at 16:16, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> > On Mon, 5 Feb 2024 at 14:08, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> > >
> > > Magnus Karlsson <magnus.karlsson@gmail.com> writes:
> > >
> > > > From: Magnus Karlsson <magnus.karlsson@intel.com>
> > > >
> > > > Do not report the XDP capability NETDEV_XDP_ACT_XSK_ZEROCOPY as the
> > > > bonding driver does not support XDP and AF_XDP in zero-copy mode even
> > > > if the real NIC drivers do.
> > > >
> > > > Fixes: cb9e6e584d58 ("bonding: add xdp_features support")
> > > > Reported-by: Prashant Batra <prbatra.mail@gmail.com>
> > > > Link: https://lore.kernel.org/all/CAJ8uoz2ieZCopgqTvQ9ZY6xQgTbujmC6XkMTamhp68O-h_-rLg@mail.gmail.com/T/
> > > > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > > > ---
> > > >  drivers/net/bonding/bond_main.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > > index 4e0600c7b050..79a37bed097b 100644
> > > > --- a/drivers/net/bonding/bond_main.c
> > > > +++ b/drivers/net/bonding/bond_main.c
> > > > @@ -1819,6 +1819,8 @@ void bond_xdp_set_features(struct net_device *bond_dev)
> > > >       bond_for_each_slave(bond, slave, iter)
> > > >               val &= slave->dev->xdp_features;
> > > >
> > > > +     val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY;
> > > > +
> > > >       xdp_set_features_flag(bond_dev, val);
> > > >  }
> > > >
> > > > @@ -5910,8 +5912,10 @@ void bond_setup(struct net_device *bond_dev)
> > > >               bond_dev->features |= BOND_XFRM_FEATURES;
> > > >  #endif /* CONFIG_XFRM_OFFLOAD */
> > > >
> > > > -     if (bond_xdp_check(bond))
> > > > +     if (bond_xdp_check(bond)) {
> > > >               bond_dev->xdp_features = NETDEV_XDP_ACT_MASK;
> > > > +             bond_dev->xdp_features &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY;
> > > > +     }
> > >
> > > Shouldn't we rather drop this assignment completely? It makes no sense
> > > to default to all features, it should default to none...
> >
> > Good point. Seems the bond device defaults to supporting everything
> > before a device is bonded to it, but I might have misunderstood
> > something. Lorenzo, could you enlighten us please?
>
> ack, I agree we can get rid of it since the xdp features will be calculated
> again as soon as a new device is added to the bond.

Thanks. Will spin a v2.

> Regards,
> Lorenzo
>
> >
> > Thanks: Magnus
> >
> > > -Toke
> > >
diff mbox series

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 4e0600c7b050..79a37bed097b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1819,6 +1819,8 @@  void bond_xdp_set_features(struct net_device *bond_dev)
 	bond_for_each_slave(bond, slave, iter)
 		val &= slave->dev->xdp_features;
 
+	val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY;
+
 	xdp_set_features_flag(bond_dev, val);
 }
 
@@ -5910,8 +5912,10 @@  void bond_setup(struct net_device *bond_dev)
 		bond_dev->features |= BOND_XFRM_FEATURES;
 #endif /* CONFIG_XFRM_OFFLOAD */
 
-	if (bond_xdp_check(bond))
+	if (bond_xdp_check(bond)) {
 		bond_dev->xdp_features = NETDEV_XDP_ACT_MASK;
+		bond_dev->xdp_features &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY;
+	}
 }
 
 /* Destroy a bonding device.