[PATCHv2 net-next 0/3] Bonding: returns detailed error about XDP failures

[PATCHv2 net-next 0/3] Bonding: returns detailed error about XDP failures

[Hangbin Liu]

Based on discussion[1], this patch set returns detailed error about XDP
failures. And update bonding document about XDP supports.

v2: update the title in the doc (Nikolay Aleksandrov)

[1] https://lore.kernel.org/netdev/8088f2a7-3ab1-4a1e-996d-c15703da13cc@blackwall.org/

Hangbin Liu (3):
  bonding: return detailed error when loading native XDP fails
  bonding: use correct return value
  Documentation: bonding: add XDP support explanation

 Documentation/networking/bonding.rst | 12 ++++++++++++
 drivers/net/bonding/bond_main.c      |  7 +++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.46.0

[PATCHv2 net-next 1/3] bonding: return detailed error when loading native XDP fails

[Hangbin Liu]

Bonding only supports native XDP for specific modes, which can lead to
confusion for users regarding why XDP loads successfully at times and
fails at others. This patch enhances error handling by returning detailed
error messages, providing users with clearer insights into the specific
reasons for the failure when loading native XDP.

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b1bffd8e9a95..f0f76b6ac8be 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5676,8 +5676,11 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 
 	ASSERT_RTNL();
 
-	if (!bond_xdp_check(bond))
+	if (!bond_xdp_check(bond)) {
+		BOND_NL_ERR(dev, extack,
+			    "No native XDP support for the current bonding mode");
 		return -EOPNOTSUPP;
+	}
 
 	old_prog = bond->xdp_prog;
 	bond->xdp_prog = prog;
-- 
2.46.0

[PATCHv2 net-next 2/3] bonding: use correct return value

[Hangbin Liu]

When a slave already has an XDP program loaded, the correct return value
should be -EEXIST instead of -EOPNOTSUPP.

Fixes: 9e2ee5c7e7c3 ("net, bonding: Add XDP support to the bonding driver")
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f0f76b6ac8be..6887a867fe8b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		if (dev_xdp_prog_count(slave_dev) > 0) {
 			SLAVE_NL_ERR(dev, slave_dev, extack,
 				     "Slave has XDP program loaded, please unload before enslaving");
-			err = -EOPNOTSUPP;
+			err = -EEXIST;
 			goto err;
 		}
 
-- 
2.46.0

[PATCHv2 net-next 3/3] Documentation: bonding: add XDP support explanation

[Hangbin Liu]

Add document about which modes have native XDP support.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 Documentation/networking/bonding.rst | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
index e774b48de9f5..5c4a83005025 100644
--- a/Documentation/networking/bonding.rst
+++ b/Documentation/networking/bonding.rst
@@ -2916,6 +2916,18 @@ from the bond (``ifenslave -d bond0 eth0``). The bonding driver will
 then restore the MAC addresses that the slaves had before they were
 enslaved.
 
+9.  What bonding modes support native XDP?
+------------------------------------------
+
+Currently, only the following bonding modes support native XDP:
+  * balance-rr (0)
+  * active-backup (1)
+  * balance-xor (2)
+  * 802.3ad (4)
+
+Note that the vlan+srcmac hash policy does not support native XDP.
+For other bonding modes, the XDP program must be loaded with generic mode.
+
 16. Resources and Links
 =======================
 
-- 
2.46.0

Re: [PATCHv2 net-next 0/3] Bonding: returns detailed error about XDP failures

[Nikolay Aleksandrov]

On 17/10/2024 05:06, Hangbin Liu wrote:
> Based on discussion[1], this patch set returns detailed error about XDP
> failures. And update bonding document about XDP supports.
> 
> v2: update the title in the doc (Nikolay Aleksandrov)
> 
> [1] https://lore.kernel.org/netdev/8088f2a7-3ab1-4a1e-996d-c15703da13cc@blackwall.org/
> 
> Hangbin Liu (3):
>   bonding: return detailed error when loading native XDP fails
>   bonding: use correct return value
>   Documentation: bonding: add XDP support explanation
> 
>  Documentation/networking/bonding.rst | 12 ++++++++++++
>  drivers/net/bonding/bond_main.c      |  7 +++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 

Please CC reviewers when sending new versions. I was CCed on patches 1 and 2
probably due to the tag, but wasn't on patch 3 and had to search for
the series.

Thanks,
 Nik

Re: [PATCHv2 net-next 3/3] Documentation: bonding: add XDP support explanation

[Nikolay Aleksandrov]

On 17/10/2024 05:06, Hangbin Liu wrote:
> Add document about which modes have native XDP support.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  Documentation/networking/bonding.rst | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
> index e774b48de9f5..5c4a83005025 100644
> --- a/Documentation/networking/bonding.rst
> +++ b/Documentation/networking/bonding.rst
> @@ -2916,6 +2916,18 @@ from the bond (``ifenslave -d bond0 eth0``). The bonding driver will
>  then restore the MAC addresses that the slaves had before they were
>  enslaved.
>  
> +9.  What bonding modes support native XDP?
> +------------------------------------------
> +
> +Currently, only the following bonding modes support native XDP:

If there's a new version please consider dropping this sentence.
It just repeats the title above in a different way.

> +  * balance-rr (0)
> +  * active-backup (1)
> +  * balance-xor (2)
> +  * 802.3ad (4)
> +
> +Note that the vlan+srcmac hash policy does not support native XDP.
> +For other bonding modes, the XDP program must be loaded with generic mode.
> +
>  16. Resources and Links
>  =======================
>  

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCHv2 net-next 0/3] Bonding: returns detailed error about XDP failures

[Hangbin Liu]

On Thu, Oct 17, 2024 at 11:40:34AM +0300, Nikolay Aleksandrov wrote:
> Please CC reviewers when sending new versions. I was CCed on patches 1 and 2
> probably due to the tag, but wasn't on patch 3 and had to search for
> the series.

Oh, sorry for the inconvenient. I thought you are in the cc list. Next time I
will do double check.

Hangbin

Re: [PATCHv2 net-next 1/3] bonding: return detailed error when loading native XDP fails

[Toke Høiland-Jørgensen]

Hangbin Liu <liuhangbin@gmail.com> writes:

> Bonding only supports native XDP for specific modes, which can lead to
> confusion for users regarding why XDP loads successfully at times and
> fails at others. This patch enhances error handling by returning detailed
> error messages, providing users with clearer insights into the specific
> reasons for the failure when loading native XDP.
>
> Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

[Toke Høiland-Jørgensen]

Hangbin Liu <liuhangbin@gmail.com> writes:

> When a slave already has an XDP program loaded, the correct return value
> should be -EEXIST instead of -EOPNOTSUPP.
>
> Fixes: 9e2ee5c7e7c3 ("net, bonding: Add XDP support to the bonding driver")
> Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/bonding/bond_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index f0f76b6ac8be..6887a867fe8b 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>  		if (dev_xdp_prog_count(slave_dev) > 0) {
>  			SLAVE_NL_ERR(dev, slave_dev, extack,
>  				     "Slave has XDP program loaded, please unload before enslaving");
> -			err = -EOPNOTSUPP;
> +			err = -EEXIST;

Hmm, this has been UAPI since kernel 5.15, so can we really change it
now? What's the purpose of changing it, anyway?

-Toke

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

[Hangbin Liu]

On Thu, Oct 17, 2024 at 04:47:19PM +0200, Toke Høiland-Jørgensen wrote:
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index f0f76b6ac8be..6887a867fe8b 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> >  		if (dev_xdp_prog_count(slave_dev) > 0) {
> >  			SLAVE_NL_ERR(dev, slave_dev, extack,
> >  				     "Slave has XDP program loaded, please unload before enslaving");
> > -			err = -EOPNOTSUPP;
> > +			err = -EEXIST;
> 
> Hmm, this has been UAPI since kernel 5.15, so can we really change it
> now? What's the purpose of changing it, anyway?

I just think it should return EXIST when the error is "Slave has XDP program
loaded". No special reason. If all others think we should not change it, I
can drop this patch.

Thanks
Hangbin

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

[Simon Horman]

On Fri, Oct 18, 2024 at 12:46:18AM +0000, Hangbin Liu wrote:
> On Thu, Oct 17, 2024 at 04:47:19PM +0200, Toke Høiland-Jørgensen wrote:
> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > index f0f76b6ac8be..6887a867fe8b 100644
> > > --- a/drivers/net/bonding/bond_main.c
> > > +++ b/drivers/net/bonding/bond_main.c
> > > @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > >  		if (dev_xdp_prog_count(slave_dev) > 0) {
> > >  			SLAVE_NL_ERR(dev, slave_dev, extack,
> > >  				     "Slave has XDP program loaded, please unload before enslaving");
> > > -			err = -EOPNOTSUPP;
> > > +			err = -EEXIST;
> > 
> > Hmm, this has been UAPI since kernel 5.15, so can we really change it
> > now? What's the purpose of changing it, anyway?
> 
> I just think it should return EXIST when the error is "Slave has XDP program
> loaded". No special reason. If all others think we should not change it, I
> can drop this patch.

Hi Toke,

Could you add some colour to what extent user's might rely on this error code?

Basically I think that if they do then we shouldn't change this.

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

[Toke Høiland-Jørgensen]

Simon Horman <horms@kernel.org> writes:

> On Fri, Oct 18, 2024 at 12:46:18AM +0000, Hangbin Liu wrote:
>> On Thu, Oct 17, 2024 at 04:47:19PM +0200, Toke Høiland-Jørgensen wrote:
>> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> > > index f0f76b6ac8be..6887a867fe8b 100644
>> > > --- a/drivers/net/bonding/bond_main.c
>> > > +++ b/drivers/net/bonding/bond_main.c
>> > > @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>> > >  		if (dev_xdp_prog_count(slave_dev) > 0) {
>> > >  			SLAVE_NL_ERR(dev, slave_dev, extack,
>> > >  				     "Slave has XDP program loaded, please unload before enslaving");
>> > > -			err = -EOPNOTSUPP;
>> > > +			err = -EEXIST;
>> > 
>> > Hmm, this has been UAPI since kernel 5.15, so can we really change it
>> > now? What's the purpose of changing it, anyway?
>> 
>> I just think it should return EXIST when the error is "Slave has XDP program
>> loaded". No special reason. If all others think we should not change it, I
>> can drop this patch.
>
> Hi Toke,
>
> Could you add some colour to what extent user's might rely on this error code?
>
> Basically I think that if they do then we shouldn't change this.

Well, that's the trouble with UAPI, we don't really know. In libxdp and
xdp-tools we look at the return code to provide a nicer error message,
like:

https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L615

and as a signal to fall back to loading the programme without a dispatcher:

https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L1824

Both of these cases would be unaffected (or even improved) by this
patch, so in that sense I don't have a concrete objection, just a
general "userspace may react to this". In other words, my concern is
more of a general "we don't know, so this seems risky". If any of you
have more information about how bonding XDP is generally used, that may
help get a better idea of this?

-Toke

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

[Simon Horman]

On Fri, Oct 18, 2024 at 01:29:30PM +0200, Toke Høiland-Jørgensen wrote:
> Simon Horman <horms@kernel.org> writes:
> 
> > On Fri, Oct 18, 2024 at 12:46:18AM +0000, Hangbin Liu wrote:
> >> On Thu, Oct 17, 2024 at 04:47:19PM +0200, Toke Høiland-Jørgensen wrote:
> >> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >> > > index f0f76b6ac8be..6887a867fe8b 100644
> >> > > --- a/drivers/net/bonding/bond_main.c
> >> > > +++ b/drivers/net/bonding/bond_main.c
> >> > > @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> >> > >  		if (dev_xdp_prog_count(slave_dev) > 0) {
> >> > >  			SLAVE_NL_ERR(dev, slave_dev, extack,
> >> > >  				     "Slave has XDP program loaded, please unload before enslaving");
> >> > > -			err = -EOPNOTSUPP;
> >> > > +			err = -EEXIST;
> >> > 
> >> > Hmm, this has been UAPI since kernel 5.15, so can we really change it
> >> > now? What's the purpose of changing it, anyway?
> >> 
> >> I just think it should return EXIST when the error is "Slave has XDP program
> >> loaded". No special reason. If all others think we should not change it, I
> >> can drop this patch.
> >
> > Hi Toke,
> >
> > Could you add some colour to what extent user's might rely on this error code?
> >
> > Basically I think that if they do then we shouldn't change this.
> 
> Well, that's the trouble with UAPI, we don't really know. In libxdp and
> xdp-tools we look at the return code to provide a nicer error message,
> like:
> 
> https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L615
> 
> and as a signal to fall back to loading the programme without a dispatcher:
> 
> https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L1824
> 
> Both of these cases would be unaffected (or even improved) by this
> patch, so in that sense I don't have a concrete objection, just a
> general "userspace may react to this". In other words, my concern is
> more of a general "we don't know, so this seems risky". If any of you
> have more information about how bonding XDP is generally used, that may
> help get a better idea of this?

Yes, that is the trouble with the UAPI. I was hoping you might be able to
provide the clarity you ask for above. But alas, things are as clear as
mud.

In lieu of more information I suggest caution and dropping this change for
now.

-- 
pw-bot: cr

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

[Hangbin Liu]

On Fri, Oct 18, 2024 at 03:21:04PM +0100, Simon Horman wrote:
> On Fri, Oct 18, 2024 at 01:29:30PM +0200, Toke Høiland-Jørgensen wrote:
> > Simon Horman <horms@kernel.org> writes:
> > 
> > > On Fri, Oct 18, 2024 at 12:46:18AM +0000, Hangbin Liu wrote:
> > >> On Thu, Oct 17, 2024 at 04:47:19PM +0200, Toke Høiland-Jørgensen wrote:
> > >> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > >> > > index f0f76b6ac8be..6887a867fe8b 100644
> > >> > > --- a/drivers/net/bonding/bond_main.c
> > >> > > +++ b/drivers/net/bonding/bond_main.c
> > >> > > @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > >> > >  		if (dev_xdp_prog_count(slave_dev) > 0) {
> > >> > >  			SLAVE_NL_ERR(dev, slave_dev, extack,
> > >> > >  				     "Slave has XDP program loaded, please unload before enslaving");
> > >> > > -			err = -EOPNOTSUPP;
> > >> > > +			err = -EEXIST;
> > >> > 
> > >> > Hmm, this has been UAPI since kernel 5.15, so can we really change it
> > >> > now? What's the purpose of changing it, anyway?
> > >> 
> > >> I just think it should return EXIST when the error is "Slave has XDP program
> > >> loaded". No special reason. If all others think we should not change it, I
> > >> can drop this patch.
> > >
> > > Hi Toke,
> > >
> > > Could you add some colour to what extent user's might rely on this error code?
> > >
> > > Basically I think that if they do then we shouldn't change this.
> > 
> > Well, that's the trouble with UAPI, we don't really know. In libxdp and
> > xdp-tools we look at the return code to provide a nicer error message,
> > like:
> > 
> > https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L615
> > 
> > and as a signal to fall back to loading the programme without a dispatcher:
> > 
> > https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L1824
> > 
> > Both of these cases would be unaffected (or even improved) by this
> > patch, so in that sense I don't have a concrete objection, just a
> > general "userspace may react to this". In other words, my concern is
> > more of a general "we don't know, so this seems risky". If any of you
> > have more information about how bonding XDP is generally used, that may
> > help get a better idea of this?
> 
> Yes, that is the trouble with the UAPI. I was hoping you might be able to
> provide the clarity you ask for above. But alas, things are as clear as
> mud.
> 
> In lieu of more information I suggest caution and dropping this change for
> now.

OK, I will drop this one.

Thanks
Hangbin

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

[Simon Horman]

On Sat, Oct 19, 2024 at 12:51:00AM +0000, Hangbin Liu wrote:
> On Fri, Oct 18, 2024 at 03:21:04PM +0100, Simon Horman wrote:
> > On Fri, Oct 18, 2024 at 01:29:30PM +0200, Toke Høiland-Jørgensen wrote:
> > > Simon Horman <horms@kernel.org> writes:
> > > 
> > > > On Fri, Oct 18, 2024 at 12:46:18AM +0000, Hangbin Liu wrote:
> > > >> On Thu, Oct 17, 2024 at 04:47:19PM +0200, Toke Høiland-Jørgensen wrote:
> > > >> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > >> > > index f0f76b6ac8be..6887a867fe8b 100644
> > > >> > > --- a/drivers/net/bonding/bond_main.c
> > > >> > > +++ b/drivers/net/bonding/bond_main.c
> > > >> > > @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > > >> > >  		if (dev_xdp_prog_count(slave_dev) > 0) {
> > > >> > >  			SLAVE_NL_ERR(dev, slave_dev, extack,
> > > >> > >  				     "Slave has XDP program loaded, please unload before enslaving");
> > > >> > > -			err = -EOPNOTSUPP;
> > > >> > > +			err = -EEXIST;
> > > >> > 
> > > >> > Hmm, this has been UAPI since kernel 5.15, so can we really change it
> > > >> > now? What's the purpose of changing it, anyway?
> > > >> 
> > > >> I just think it should return EXIST when the error is "Slave has XDP program
> > > >> loaded". No special reason. If all others think we should not change it, I
> > > >> can drop this patch.
> > > >
> > > > Hi Toke,
> > > >
> > > > Could you add some colour to what extent user's might rely on this error code?
> > > >
> > > > Basically I think that if they do then we shouldn't change this.
> > > 
> > > Well, that's the trouble with UAPI, we don't really know. In libxdp and
> > > xdp-tools we look at the return code to provide a nicer error message,
> > > like:
> > > 
> > > https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L615
> > > 
> > > and as a signal to fall back to loading the programme without a dispatcher:
> > > 
> > > https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L1824
> > > 
> > > Both of these cases would be unaffected (or even improved) by this
> > > patch, so in that sense I don't have a concrete objection, just a
> > > general "userspace may react to this". In other words, my concern is
> > > more of a general "we don't know, so this seems risky". If any of you
> > > have more information about how bonding XDP is generally used, that may
> > > help get a better idea of this?
> > 
> > Yes, that is the trouble with the UAPI. I was hoping you might be able to
> > provide the clarity you ask for above. But alas, things are as clear as
> > mud.
> > 
> > In lieu of more information I suggest caution and dropping this change for
> > now.
> 
> OK, I will drop this one.

Thanks.