[PATCH net-next 0/3] Bonding: return detailed error about XDP failures

[PATCH net-next 0/3] Bonding: return detailed error about XDP failures

[Hangbin Liu]

This patch set return detailed error about XDP failures. And update
bonding document about XDP supports.

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

[PATCH 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.

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

[PATCH 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")
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

[PATCH 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..6a1a6293dd3a 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 modes does bonding have native XDP support?
+----------------------------------------------------
+
+Currently, native XDP is supported only in the following bonding modes:
+  * balance-rr (0)
+  * active-backup (1)
+  * balance-xor (2)
+  * 802.3ad (4)
+
+Note that the vlan+srcmac hash policy is not supported with native XDP.
+For other bonding modes, the XDP program must be loaded in generic mode.
+
 16. Resources and Links
 =======================
 
-- 
2.46.0

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

[Nikolay Aleksandrov]

On 16/10/2024 06:16, Hangbin Liu wrote:
> 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.
> 
> 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;

I guess this is based on our discussion earlier?

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

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

[Nikolay Aleksandrov]

On 16/10/2024 06:16, Hangbin Liu wrote:
> 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")
> 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;
>  		}
> 

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

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

[Nikolay Aleksandrov]

On 16/10/2024 06:16, 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..6a1a6293dd3a 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 modes does bonding have native XDP support?
TBH this sounds strange and to be correct it probably needs
to end with "for" (What modes does bonding have native XDP support for), but
how about something straight-forward like:

 What bonding modes have native XDP support?

or

 What bonding modes support native XDP?

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

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

[Daniel Borkmann]

On 10/16/24 5:16 AM, Hangbin Liu wrote:
> 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.
> 
> 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;

LGTM, but independent of these I was more thinking whether something like this
could do the trick (only compile tested). That way you also get the fallback
without changing anything in the core XDP code.

Thanks,
Daniel

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b1bffd8e9a95..2861b3a895ff 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5915,6 +5915,10 @@ static const struct ethtool_ops bond_ethtool_ops = {
  	.get_ts_info		= bond_ethtool_get_ts_info,
  };
  
+static const struct device_type bond_type = {
+	.name = "bond",
+};
+
  static const struct net_device_ops bond_netdev_ops = {
  	.ndo_init		= bond_init,
  	.ndo_uninit		= bond_uninit,
@@ -5951,9 +5955,20 @@ static const struct net_device_ops bond_netdev_ops = {
  	.ndo_hwtstamp_set	= bond_hwtstamp_set,
  };
  
-static const struct device_type bond_type = {
-	.name = "bond",
-};
+static struct net_device_ops bond_netdev_ops_noxdp __ro_after_init;
+
+static void __init bond_setup_noxdp_ops(void)
+{
+	memcpy(&bond_netdev_ops_noxdp, &bond_netdev_ops,
+	       sizeof(bond_netdev_ops));
+
+	/* Used for bond device mode which does not support XDP
+	 * yet, see also bond_xdp_check().
+	 */
+	bond_netdev_ops_noxdp.ndo_bpf = NULL;
+	bond_netdev_ops_noxdp.ndo_xdp_xmit = NULL;
+	bond_netdev_ops_noxdp.ndo_xdp_get_xmit_slave = NULL;
+}
  
  static void bond_destructor(struct net_device *bond_dev)
  {
@@ -5978,7 +5993,9 @@ void bond_setup(struct net_device *bond_dev)
  	/* Initialize the device entry points */
  	ether_setup(bond_dev);
  	bond_dev->max_mtu = ETH_MAX_MTU;
-	bond_dev->netdev_ops = &bond_netdev_ops;
+	bond_dev->netdev_ops = bond_xdp_check(bond) ?
+			       &bond_netdev_ops :
+			       &bond_netdev_ops_noxdp;
  	bond_dev->ethtool_ops = &bond_ethtool_ops;
  
  	bond_dev->needs_free_netdev = true;
@@ -6591,6 +6608,8 @@ static int __init bonding_init(void)
  	int i;
  	int res;
  
+	bond_setup_noxdp_ops();
+
  	res = bond_check_params(&bonding_defaults);
  	if (res)
  		goto out;

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

[Nikolay Aleksandrov]

On 16/10/2024 10:59, Daniel Borkmann wrote:
> On 10/16/24 5:16 AM, Hangbin Liu wrote:
>> 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.
>>
>> 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;
> 
> LGTM, but independent of these I was more thinking whether something like this
> could do the trick (only compile tested). That way you also get the fallback
> without changing anything in the core XDP code.
> 
> Thanks,
> Daniel
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index b1bffd8e9a95..2861b3a895ff 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5915,6 +5915,10 @@ static const struct ethtool_ops bond_ethtool_ops = {
>      .get_ts_info        = bond_ethtool_get_ts_info,
>  };
>  
> +static const struct device_type bond_type = {
> +    .name = "bond",
> +};
> +
>  static const struct net_device_ops bond_netdev_ops = {
>      .ndo_init        = bond_init,
>      .ndo_uninit        = bond_uninit,
> @@ -5951,9 +5955,20 @@ static const struct net_device_ops bond_netdev_ops = {
>      .ndo_hwtstamp_set    = bond_hwtstamp_set,
>  };
>  
> -static const struct device_type bond_type = {
> -    .name = "bond",
> -};
> +static struct net_device_ops bond_netdev_ops_noxdp __ro_after_init;
> +
> +static void __init bond_setup_noxdp_ops(void)
> +{
> +    memcpy(&bond_netdev_ops_noxdp, &bond_netdev_ops,
> +           sizeof(bond_netdev_ops));
> +
> +    /* Used for bond device mode which does not support XDP
> +     * yet, see also bond_xdp_check().
> +     */
> +    bond_netdev_ops_noxdp.ndo_bpf = NULL;
> +    bond_netdev_ops_noxdp.ndo_xdp_xmit = NULL;
> +    bond_netdev_ops_noxdp.ndo_xdp_get_xmit_slave = NULL;
> +}
>  
>  static void bond_destructor(struct net_device *bond_dev)
>  {
> @@ -5978,7 +5993,9 @@ void bond_setup(struct net_device *bond_dev)
>      /* Initialize the device entry points */
>      ether_setup(bond_dev);
>      bond_dev->max_mtu = ETH_MAX_MTU;
> -    bond_dev->netdev_ops = &bond_netdev_ops;
> +    bond_dev->netdev_ops = bond_xdp_check(bond) ?
> +                   &bond_netdev_ops :
> +                   &bond_netdev_ops_noxdp;

This will have to be done safely on bond mode change as well.
If all slaves are released we can switch modes without destroying
the device.

>      bond_dev->ethtool_ops = &bond_ethtool_ops;
>  
>      bond_dev->needs_free_netdev = true;
> @@ -6591,6 +6608,8 @@ static int __init bonding_init(void)
>      int i;
>      int res;
>  
> +    bond_setup_noxdp_ops();
> +
>      res = bond_check_params(&bonding_defaults);
>      if (res)
>          goto out;

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

[Hangbin Liu]

On Wed, Oct 16, 2024 at 10:38:05AM +0300, Nikolay Aleksandrov wrote:
> > +9.  What modes does bonding have native XDP support?
> TBH this sounds strange and to be correct it probably needs
> to end with "for" (What modes does bonding have native XDP support for), but
> how about something straight-forward like:
> 
>  What bonding modes have native XDP support?
> 
> or
> 
>  What bonding modes support native XDP?

Thanks, I will use this one.

Hangbin

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

[Hangbin Liu]

On Wed, Oct 16, 2024 at 09:59:01AM +0200, Daniel Borkmann wrote:
> > 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;
> 
> LGTM, but independent of these I was more thinking whether something like this
> could do the trick (only compile tested). That way you also get the fallback
> without changing anything in the core XDP code.

Yes, I also thought about do fallback on bonding. But Nikolay suggested
just use extack msg[1], and Jakub think this is report by QE rather than
a real user. So I think we can use extack first, and convert to auto
fallback on bonding if a real user complains. What do you think?

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

Thanks
Hangbin

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

[Daniel Borkmann]

On 10/16/24 10:13 AM, Nikolay Aleksandrov wrote:
> On 16/10/2024 10:59, Daniel Borkmann wrote:
>> On 10/16/24 5:16 AM, Hangbin Liu wrote:
>>> 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.
>>>
>>> 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;
>>
>> LGTM, but independent of these I was more thinking whether something like this
>> could do the trick (only compile tested). That way you also get the fallback
>> without changing anything in the core XDP code.
>>
>> Thanks,
>> Daniel
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index b1bffd8e9a95..2861b3a895ff 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -5915,6 +5915,10 @@ static const struct ethtool_ops bond_ethtool_ops = {
>>       .get_ts_info        = bond_ethtool_get_ts_info,
>>   };
>>   
>> +static const struct device_type bond_type = {
>> +    .name = "bond",
>> +};
>> +
>>   static const struct net_device_ops bond_netdev_ops = {
>>       .ndo_init        = bond_init,
>>       .ndo_uninit        = bond_uninit,
>> @@ -5951,9 +5955,20 @@ static const struct net_device_ops bond_netdev_ops = {
>>       .ndo_hwtstamp_set    = bond_hwtstamp_set,
>>   };
>>   
>> -static const struct device_type bond_type = {
>> -    .name = "bond",
>> -};
>> +static struct net_device_ops bond_netdev_ops_noxdp __ro_after_init;
>> +
>> +static void __init bond_setup_noxdp_ops(void)
>> +{
>> +    memcpy(&bond_netdev_ops_noxdp, &bond_netdev_ops,
>> +           sizeof(bond_netdev_ops));
>> +
>> +    /* Used for bond device mode which does not support XDP
>> +     * yet, see also bond_xdp_check().
>> +     */
>> +    bond_netdev_ops_noxdp.ndo_bpf = NULL;
>> +    bond_netdev_ops_noxdp.ndo_xdp_xmit = NULL;
>> +    bond_netdev_ops_noxdp.ndo_xdp_get_xmit_slave = NULL;
>> +}
>>   
>>   static void bond_destructor(struct net_device *bond_dev)
>>   {
>> @@ -5978,7 +5993,9 @@ void bond_setup(struct net_device *bond_dev)
>>       /* Initialize the device entry points */
>>       ether_setup(bond_dev);
>>       bond_dev->max_mtu = ETH_MAX_MTU;
>> -    bond_dev->netdev_ops = &bond_netdev_ops;
>> +    bond_dev->netdev_ops = bond_xdp_check(bond) ?
>> +                   &bond_netdev_ops :
>> +                   &bond_netdev_ops_noxdp;
> 
> This will have to be done safely on bond mode change as well.
> If all slaves are released we can switch modes without destroying
> the device.

Ah fair, yeah perhaps not worth the added complexity. Tbh, if someone
loads an XDP program with the fallback to generic, it feels super fragile
in the first place and I wouldn't do this ever for production workloads.
Meaning, fixed to native for production, generic XDP for testing where
native is not available (e.g. CI), at least that's how we use it.

Thanks,
Daniel