[PATCH net RESEND] bonding: don't change to 802.3ad mode while ARP monitoring is running

[PATCH net RESEND] bonding: don't change to 802.3ad mode while ARP monitoring is running

[Ding Tianhong]

Because the ARP monitoring is not support for 802.3ad, but I still
could change the mode to 802.3ad from ab mode while ARP monitoring
is running, it is incorrect.

So add a check for 802.3ad in bonding_store_mode to fix the problem,
and make a new macro BOND_NO_USES_ARP() to simplify the code.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_options.c | 2 +-
 drivers/net/bonding/bonding.h      | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 9a5223c..abb4218 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -45,7 +45,7 @@ int bond_option_mode_set(struct bonding *bond, int mode)
 		return -EPERM;
 	}
 
-	if (BOND_MODE_IS_LB(mode) && bond->params.arp_interval) {
+	if (BOND_NO_USES_ARP(mode) && bond->params.arp_interval) {
 		pr_err("%s: %s mode is incompatible with arp monitoring.\n",
 		       bond->dev->name, bond_mode_tbl[mode].modename);
 		return -EINVAL;
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 046a605..e2c11cb 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -63,6 +63,10 @@
 		(((mode) == BOND_MODE_TLB) ||	\
 		 ((mode) == BOND_MODE_ALB))
 
+#define BOND_NO_USES_ARP(mode)				\
+		(((mode) == BOND_MODE_8023AD)	||	\
+		 ((mode) == BOND_MODE_TLB)	||	\
+		 ((mode) == BOND_MODE_ALB))
 /*
  * Less bad way to call ioctl from within the kernel; this needs to be
  * done some other way to get the call out of interrupt context.
-- 
1.7.12

Re: [PATCH net RESEND] bonding: don't change to 802.3ad mode while ARP monitoring is running

[Dan Williams]

On Sat, 2013-11-16 at 14:30 +0800, Ding Tianhong wrote:
> Because the ARP monitoring is not support for 802.3ad, but I still
> could change the mode to 802.3ad from ab mode while ARP monitoring
> is running, it is incorrect.
> 
> So add a check for 802.3ad in bonding_store_mode to fix the problem,
> and make a new macro BOND_NO_USES_ARP() to simplify the code.

Instead of failing, couldn't the code stop ARP monitoring and allow the
mode change?  This is similar to setting miimon, which disables ARP
monitoring, or setting ARP monitoring, which disables miimon.

	if (new_value && bond->params.arp_interval) {
		pr_info("%s: MII monitoring cannot be used with ARP monitoring. Disabling ARP monitoring...\n",
			bond->dev->name);
		bond->params.arp_interval = 0;
		if (bond->params.arp_validate)
			bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
	}

Bond mode is the most important bond option, so it seems like it should
override any of the other sub-options.  I know the code doesn't do this
now, but maybe instead of the patch you propose, it would be nicer to
allow the mode change instead?

Dan

> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
>  drivers/net/bonding/bond_options.c | 2 +-
>  drivers/net/bonding/bonding.h      | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 9a5223c..abb4218 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -45,7 +45,7 @@ int bond_option_mode_set(struct bonding *bond, int mode)
>  		return -EPERM;
>  	}
>  
> -	if (BOND_MODE_IS_LB(mode) && bond->params.arp_interval) {
> +	if (BOND_NO_USES_ARP(mode) && bond->params.arp_interval) {
>  		pr_err("%s: %s mode is incompatible with arp monitoring.\n",
>  		       bond->dev->name, bond_mode_tbl[mode].modename);
>  		return -EINVAL;
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index 046a605..e2c11cb 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -63,6 +63,10 @@
>  		(((mode) == BOND_MODE_TLB) ||	\
>  		 ((mode) == BOND_MODE_ALB))
>  
> +#define BOND_NO_USES_ARP(mode)				\
> +		(((mode) == BOND_MODE_8023AD)	||	\
> +		 ((mode) == BOND_MODE_TLB)	||	\
> +		 ((mode) == BOND_MODE_ALB))
>  /*
>   * Less bad way to call ioctl from within the kernel; this needs to be
>   * done some other way to get the call out of interrupt context.

Re: [PATCH net RESEND] bonding: don't change to 802.3ad mode while ARP monitoring is running

[David Miller]

From: Dan Williams <dcbw@redhat.com>
Date: Mon, 18 Nov 2013 11:44:42 -0600

> On Sat, 2013-11-16 at 14:30 +0800, Ding Tianhong wrote:
>> Because the ARP monitoring is not support for 802.3ad, but I still
>> could change the mode to 802.3ad from ab mode while ARP monitoring
>> is running, it is incorrect.
>> 
>> So add a check for 802.3ad in bonding_store_mode to fix the problem,
>> and make a new macro BOND_NO_USES_ARP() to simplify the code.
> 
> Instead of failing, couldn't the code stop ARP monitoring and allow the
> mode change?  This is similar to setting miimon, which disables ARP
> monitoring, or setting ARP monitoring, which disables miimon.
> 
> 	if (new_value && bond->params.arp_interval) {
> 		pr_info("%s: MII monitoring cannot be used with ARP monitoring. Disabling ARP monitoring...\n",
> 			bond->dev->name);
> 		bond->params.arp_interval = 0;
> 		if (bond->params.arp_validate)
> 			bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
> 	}
> 
> Bond mode is the most important bond option, so it seems like it should
> override any of the other sub-options.  I know the code doesn't do this
> now, but maybe instead of the patch you propose, it would be nicer to
> allow the mode change instead?

I agree with Dan, if other mode changes behave this way (by dropping the
incompatible feature) we should make 802.3ad do so as well at the very
least for consistency.

Re: [PATCH net RESEND] bonding: don't change to 802.3ad mode while ARP monitoring is running

[Dan Williams]

On Mon, 2013-11-18 at 15:48 -0500, David Miller wrote:
> From: Dan Williams <dcbw@redhat.com>
> Date: Mon, 18 Nov 2013 11:44:42 -0600
> 
> > On Sat, 2013-11-16 at 14:30 +0800, Ding Tianhong wrote:
> >> Because the ARP monitoring is not support for 802.3ad, but I still
> >> could change the mode to 802.3ad from ab mode while ARP monitoring
> >> is running, it is incorrect.
> >> 
> >> So add a check for 802.3ad in bonding_store_mode to fix the problem,
> >> and make a new macro BOND_NO_USES_ARP() to simplify the code.
> > 
> > Instead of failing, couldn't the code stop ARP monitoring and allow the
> > mode change?  This is similar to setting miimon, which disables ARP
> > monitoring, or setting ARP monitoring, which disables miimon.
> > 
> > 	if (new_value && bond->params.arp_interval) {
> > 		pr_info("%s: MII monitoring cannot be used with ARP monitoring. Disabling ARP monitoring...\n",
> > 			bond->dev->name);
> > 		bond->params.arp_interval = 0;
> > 		if (bond->params.arp_validate)
> > 			bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
> > 	}
> > 
> > Bond mode is the most important bond option, so it seems like it should
> > override any of the other sub-options.  I know the code doesn't do this
> > now, but maybe instead of the patch you propose, it would be nicer to
> > allow the mode change instead?
> 
> I agree with Dan, if other mode changes behave this way (by dropping the
> incompatible feature) we should make 802.3ad do so as well at the very
> least for consistency.

Currently ALB and TLB modes will fail if arp_interval > 0, so Ding's
patch is technically correct.

Instead, I'm proposing that 'mode' trumps all, and if the user changes
the mode, conflicting values should be cleared or reset.  Otherwise
userspace has to duplicate a lot of kernel logic/validation.  For
example:

1) set mode to ROUNDROBIN
2) set arp_interval
3) set mode to ALB or TLB
4) FAIL - incompatible with arp_interval
5) ok, set arp_interval to zero
6) set mode to ALB or TLB
7) SUCCESS

Wouldn't it be nice if the kernel handled clearing arp_interval for us,
since it knows that arp_interval is incompatible with ALB/TLB...

Could be done separately.  I have no objection to Ding's patch other
than "life could be even better".

Dan

Re: [PATCH net RESEND] bonding: don't change to 802.3ad mode while ARP monitoring is running

[Ding Tianhong]

On 2013/11/19 6:50, Dan Williams wrote:
> On Mon, 2013-11-18 at 15:48 -0500, David Miller wrote:
>> From: Dan Williams <dcbw@redhat.com>
>> Date: Mon, 18 Nov 2013 11:44:42 -0600
>>
>>> On Sat, 2013-11-16 at 14:30 +0800, Ding Tianhong wrote:
>>>> Because the ARP monitoring is not support for 802.3ad, but I still
>>>> could change the mode to 802.3ad from ab mode while ARP monitoring
>>>> is running, it is incorrect.
>>>>
>>>> So add a check for 802.3ad in bonding_store_mode to fix the problem,
>>>> and make a new macro BOND_NO_USES_ARP() to simplify the code.
>>>
>>> Instead of failing, couldn't the code stop ARP monitoring and allow the
>>> mode change?  This is similar to setting miimon, which disables ARP
>>> monitoring, or setting ARP monitoring, which disables miimon.
>>>
>>> 	if (new_value && bond->params.arp_interval) {
>>> 		pr_info("%s: MII monitoring cannot be used with ARP monitoring. Disabling ARP monitoring...\n",
>>> 			bond->dev->name);
>>> 		bond->params.arp_interval = 0;
>>> 		if (bond->params.arp_validate)
>>> 			bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
>>> 	}
>>>
>>> Bond mode is the most important bond option, so it seems like it should
>>> override any of the other sub-options.  I know the code doesn't do this
>>> now, but maybe instead of the patch you propose, it would be nicer to
>>> allow the mode change instead?
>>
>> I agree with Dan, if other mode changes behave this way (by dropping the
>> incompatible feature) we should make 802.3ad do so as well at the very
>> least for consistency.
> 
> Currently ALB and TLB modes will fail if arp_interval > 0, so Ding's
> patch is technically correct.
> 
> Instead, I'm proposing that 'mode' trumps all, and if the user changes
> the mode, conflicting values should be cleared or reset.  Otherwise
> userspace has to duplicate a lot of kernel logic/validation.  For
> example:
> 
> 1) set mode to ROUNDROBIN
> 2) set arp_interval
> 3) set mode to ALB or TLB
> 4) FAIL - incompatible with arp_interval
> 5) ok, set arp_interval to zero
> 6) set mode to ALB or TLB
> 7) SUCCESS
> 
> Wouldn't it be nice if the kernel handled clearing arp_interval for us,
> since it knows that arp_interval is incompatible with ALB/TLB...
> 
> Could be done separately.  I have no objection to Ding's patch other
> than "life could be even better".
> 
> Dan
> 
> 
agree, it could be better, when we set the mode which only support mii, we should
disable the arp monitoring if arp_interval > 0, and restart mii monitor, I will rebuild
the patch and resend it later.

Regards
Ding

> 
> .
> 

Re: [PATCH net RESEND] bonding: don't change to 802.3ad mode while ARP monitoring is running

[Andy Gospodarek]

On 11/18/2013 05:50 PM, Dan Williams wrote:
> On Mon, 2013-11-18 at 15:48 -0500, David Miller wrote:
>> From: Dan Williams <dcbw@redhat.com>
>> Date: Mon, 18 Nov 2013 11:44:42 -0600
>>
>>> On Sat, 2013-11-16 at 14:30 +0800, Ding Tianhong wrote:
>>>> Because the ARP monitoring is not support for 802.3ad, but I still
>>>> could change the mode to 802.3ad from ab mode while ARP monitoring
>>>> is running, it is incorrect.
>>>>
>>>> So add a check for 802.3ad in bonding_store_mode to fix the problem,
>>>> and make a new macro BOND_NO_USES_ARP() to simplify the code.
>>> Instead of failing, couldn't the code stop ARP monitoring and allow the
>>> mode change?  This is similar to setting miimon, which disables ARP
>>> monitoring, or setting ARP monitoring, which disables miimon.
>>>
>>> 	if (new_value && bond->params.arp_interval) {
>>> 		pr_info("%s: MII monitoring cannot be used with ARP monitoring. Disabling ARP monitoring...\n",
>>> 			bond->dev->name);
>>> 		bond->params.arp_interval = 0;
>>> 		if (bond->params.arp_validate)
>>> 			bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
>>> 	}
>>>
>>> Bond mode is the most important bond option, so it seems like it should
>>> override any of the other sub-options.  I know the code doesn't do this
>>> now, but maybe instead of the patch you propose, it would be nicer to
>>> allow the mode change instead?
>> I agree with Dan, if other mode changes behave this way (by dropping the
>> incompatible feature) we should make 802.3ad do so as well at the very
>> least for consistency.
> Currently ALB and TLB modes will fail if arp_interval > 0, so Ding's
> patch is technically correct.
>
> Instead, I'm proposing that 'mode' trumps all, and if the user changes
> the mode, conflicting values should be cleared or reset.  Otherwise
> userspace has to duplicate a lot of kernel logic/validation.  For
> example:
>
> 1) set mode to ROUNDROBIN
> 2) set arp_interval
> 3) set mode to ALB or TLB
> 4) FAIL - incompatible with arp_interval
> 5) ok, set arp_interval to zero
> 6) set mode to ALB or TLB
> 7) SUCCESS
>
> Wouldn't it be nice if the kernel handled clearing arp_interval for us,
> since it knows that arp_interval is incompatible with ALB/TLB...
>
> Could be done separately.  I have no objection to Ding's patch other
> than "life could be even better".
>
> Dan
Nik was actually planning to work on a pretty significant rewrite of the 
code that handles setting and clearing of different config options, but 
I have not seen him chime in on this thread yet.

Nik, anything you can share on this or are you still a bit away from 
coming up with a design and implementation?

Re: [PATCH net RESEND] bonding: don't change to 802.3ad mode while ARP monitoring is running

[Ding Tianhong]

On 2013/11/19 10:31, Andy Gospodarek wrote:
> On 11/18/2013 05:50 PM, Dan Williams wrote:
>> On Mon, 2013-11-18 at 15:48 -0500, David Miller wrote:
>>> From: Dan Williams <dcbw@redhat.com>
>>> Date: Mon, 18 Nov 2013 11:44:42 -0600
>>>
>>>> On Sat, 2013-11-16 at 14:30 +0800, Ding Tianhong wrote:
>>>>> Because the ARP monitoring is not support for 802.3ad, but I still
>>>>> could change the mode to 802.3ad from ab mode while ARP monitoring
>>>>> is running, it is incorrect.
>>>>>
>>>>> So add a check for 802.3ad in bonding_store_mode to fix the problem,
>>>>> and make a new macro BOND_NO_USES_ARP() to simplify the code.
>>>> Instead of failing, couldn't the code stop ARP monitoring and allow the
>>>> mode change?  This is similar to setting miimon, which disables ARP
>>>> monitoring, or setting ARP monitoring, which disables miimon.
>>>>
>>>>     if (new_value && bond->params.arp_interval) {
>>>>         pr_info("%s: MII monitoring cannot be used with ARP monitoring. Disabling ARP monitoring...\n",
>>>>             bond->dev->name);
>>>>         bond->params.arp_interval = 0;
>>>>         if (bond->params.arp_validate)
>>>>             bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
>>>>     }
>>>>
>>>> Bond mode is the most important bond option, so it seems like it should
>>>> override any of the other sub-options.  I know the code doesn't do this
>>>> now, but maybe instead of the patch you propose, it would be nicer to
>>>> allow the mode change instead?
>>> I agree with Dan, if other mode changes behave this way (by dropping the
>>> incompatible feature) we should make 802.3ad do so as well at the very
>>> least for consistency.
>> Currently ALB and TLB modes will fail if arp_interval > 0, so Ding's
>> patch is technically correct.
>>
>> Instead, I'm proposing that 'mode' trumps all, and if the user changes
>> the mode, conflicting values should be cleared or reset.  Otherwise
>> userspace has to duplicate a lot of kernel logic/validation.  For
>> example:
>>
>> 1) set mode to ROUNDROBIN
>> 2) set arp_interval
>> 3) set mode to ALB or TLB
>> 4) FAIL - incompatible with arp_interval
>> 5) ok, set arp_interval to zero
>> 6) set mode to ALB or TLB
>> 7) SUCCESS
>>
>> Wouldn't it be nice if the kernel handled clearing arp_interval for us,
>> since it knows that arp_interval is incompatible with ALB/TLB...
>>
>> Could be done separately.  I have no objection to Ding's patch other
>> than "life could be even better".
>>
>> Dan
> Nik was actually planning to work on a pretty significant rewrite of the code that handles setting and clearing of different config options, but I have not seen him chime in on this thread yet.
> 
> Nik, anything you can share on this or are you still a bit away from coming up with a design and implementation?
> 
ok, I will waiting for Nik's suggestion and then decide what to do next.
Ding

> -- 
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

[PATCH net v2] bonding: disable arp and enable mii monitoring when bond change to no uses arp mode

[Ding Tianhong]

Because the ARP monitoring is not support for 802.3ad, but I still
could change the mode to 802.3ad from ab mode while ARP monitoring
is running, it is incorrect.

So add a check for 802.3ad in bonding_store_mode to fix the problem,
and make a new macro BOND_NO_USES_ARP() to simplify the code.

v2: according to the Dan Williams's suggestion, bond mode is the most
    important bond option, it should override any of the other sub-options.
    So when the mode is changed, the conficting values should be cleared
    or reset, otherwise the user has to duplicate more operations to modify
    the logic. I disable the arp and enable mii monitoring when the bond mode
    is changed to AB, TB and 8023AD if the arp interval is true.

Suggested-by: Dan Williams <dcbw@redhat.com>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_options.c | 13 +++++++++----
 drivers/net/bonding/bonding.h      |  5 +++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 9a5223c..04364f7a 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -45,10 +45,15 @@ int bond_option_mode_set(struct bonding *bond, int mode)
 		return -EPERM;
 	}
 
-	if (BOND_MODE_IS_LB(mode) && bond->params.arp_interval) {
-		pr_err("%s: %s mode is incompatible with arp monitoring.\n",
-		       bond->dev->name, bond_mode_tbl[mode].modename);
-		return -EINVAL;
+	if (BOND_NO_USES_ARP(mode) && bond->params.arp_interval) {
+		pr_info("%s: %s mode is incompatible with arp monitoring, start mii monitoring\n",
+			bond->dev->name, bond_mode_tbl[mode].modename);
+		/* disable arp monitoring */
+		bond->params.arp_interval = 0;
+		/* set miimon to default value */
+		bond->params.miimon = 100;
+		pr_info("%s: Setting MII monitoring interval to %d.\n",
+			bond->dev->name, bond->params.miimon);
 	}
 
 	/* don't cache arp_validate between modes */
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index ca31286..a310fb5 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -55,6 +55,11 @@
 		 ((mode) == BOND_MODE_TLB)          ||	\
 		 ((mode) == BOND_MODE_ALB))
 
+#define BOND_NO_USES_ARP(mode)				\
+		(((mode) == BOND_MODE_8023AD)	||	\
+		 ((mode) == BOND_MODE_TLB)	||	\
+		 ((mode) == BOND_MODE_ALB))
+
 #define TX_QUEUE_OVERRIDE(mode)				\
 			(((mode) == BOND_MODE_ACTIVEBACKUP) ||	\
 			 ((mode) == BOND_MODE_ROUNDROBIN))
-- 
1.8.0

Re: [PATCH net RESEND] bonding: don't change to 802.3ad mode while ARP monitoring is running

[Nikolay Aleksandrov]

On 11/19/2013 03:31 AM, Andy Gospodarek wrote:
> On 11/18/2013 05:50 PM, Dan Williams wrote:
>> On Mon, 2013-11-18 at 15:48 -0500, David Miller wrote:
>>> From: Dan Williams <dcbw@redhat.com>
>>> Date: Mon, 18 Nov 2013 11:44:42 -0600
>>>
>>>> On Sat, 2013-11-16 at 14:30 +0800, Ding Tianhong wrote:
>>>>> Because the ARP monitoring is not support for 802.3ad, but I still
>>>>> could change the mode to 802.3ad from ab mode while ARP monitoring
>>>>> is running, it is incorrect.
>>>>>
>>>>> So add a check for 802.3ad in bonding_store_mode to fix the problem,
>>>>> and make a new macro BOND_NO_USES_ARP() to simplify the code.
>>>> Instead of failing, couldn't the code stop ARP monitoring and allow the
>>>> mode change?  This is similar to setting miimon, which disables ARP
>>>> monitoring, or setting ARP monitoring, which disables miimon.
>>>>
>>>>     if (new_value && bond->params.arp_interval) {
>>>>         pr_info("%s: MII monitoring cannot be used with ARP monitoring.
>>>> Disabling ARP monitoring...\n",
>>>>             bond->dev->name);
>>>>         bond->params.arp_interval = 0;
>>>>         if (bond->params.arp_validate)
>>>>             bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
>>>>     }
>>>>
>>>> Bond mode is the most important bond option, so it seems like it should
>>>> override any of the other sub-options.  I know the code doesn't do this
>>>> now, but maybe instead of the patch you propose, it would be nicer to
>>>> allow the mode change instead?
>>> I agree with Dan, if other mode changes behave this way (by dropping the
>>> incompatible feature) we should make 802.3ad do so as well at the very
>>> least for consistency.
>> Currently ALB and TLB modes will fail if arp_interval > 0, so Ding's
>> patch is technically correct.
>>
>> Instead, I'm proposing that 'mode' trumps all, and if the user changes
>> the mode, conflicting values should be cleared or reset.  Otherwise
>> userspace has to duplicate a lot of kernel logic/validation.  For
>> example:
>>
>> 1) set mode to ROUNDROBIN
>> 2) set arp_interval
>> 3) set mode to ALB or TLB
>> 4) FAIL - incompatible with arp_interval
>> 5) ok, set arp_interval to zero
>> 6) set mode to ALB or TLB
>> 7) SUCCESS
>>
>> Wouldn't it be nice if the kernel handled clearing arp_interval for us,
>> since it knows that arp_interval is incompatible with ALB/TLB...
>>
>> Could be done separately.  I have no objection to Ding's patch other
>> than "life could be even better".
>>
>> Dan
> Nik was actually planning to work on a pretty significant rewrite of the code
> that handles setting and clearing of different config options, but I have not
> seen him chime in on this thread yet.
> 
> Nik, anything you can share on this or are you still a bit away from coming up
> with a design and implementation?
> 
Oops sorry, it seems I've missed this thread and saw it just now. I'm working on
the new option code, but it's far from ready yet. I personally don't mind for
such patch to go in *now*, I can always re-work it once net-next opens up.

Cheers,
 Nik

Re: [PATCH net v2] bonding: disable arp and enable mii monitoring when bond change to no uses arp mode

[Nikolay Aleksandrov]

On 11/22/2013 03:12 AM, Ding Tianhong wrote:
> Because the ARP monitoring is not support for 802.3ad, but I still
> could change the mode to 802.3ad from ab mode while ARP monitoring
> is running, it is incorrect.
> 
> So add a check for 802.3ad in bonding_store_mode to fix the problem,
> and make a new macro BOND_NO_USES_ARP() to simplify the code.
> 
> v2: according to the Dan Williams's suggestion, bond mode is the most
>     important bond option, it should override any of the other sub-options.
>     So when the mode is changed, the conficting values should be cleared
>     or reset, otherwise the user has to duplicate more operations to modify
>     the logic. I disable the arp and enable mii monitoring when the bond mode
>     is changed to AB, TB and 8023AD if the arp interval is true.
> 
> Suggested-by: Dan Williams <dcbw@redhat.com>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
>  drivers/net/bonding/bond_options.c | 13 +++++++++----
>  drivers/net/bonding/bonding.h      |  5 +++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 9a5223c..04364f7a 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -45,10 +45,15 @@ int bond_option_mode_set(struct bonding *bond, int mode)
>  		return -EPERM;
>  	}
>  
> -	if (BOND_MODE_IS_LB(mode) && bond->params.arp_interval) {
> -		pr_err("%s: %s mode is incompatible with arp monitoring.\n",
> -		       bond->dev->name, bond_mode_tbl[mode].modename);
> -		return -EINVAL;
> +	if (BOND_NO_USES_ARP(mode) && bond->params.arp_interval) {
> +		pr_info("%s: %s mode is incompatible with arp monitoring, start mii monitoring\n",
> +			bond->dev->name, bond_mode_tbl[mode].modename);
> +		/* disable arp monitoring */
> +		bond->params.arp_interval = 0;
> +		/* set miimon to default value */
> +		bond->params.miimon = 100;
> +		pr_info("%s: Setting MII monitoring interval to %d.\n",
> +			bond->dev->name, bond->params.miimon);
>  	}
>  
Maybe define the "default" miimon value somewhere ? A value of 100 for miimon is
used repeatedly in bond_check_params() as well, it'd be nice to give it a name,
e.g. I had to grep around to see why you chose 100 to be that value.

>  	/* don't cache arp_validate between modes */
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index ca31286..a310fb5 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -55,6 +55,11 @@
>  		 ((mode) == BOND_MODE_TLB)          ||	\
>  		 ((mode) == BOND_MODE_ALB))
>  
> +#define BOND_NO_USES_ARP(mode)				\
> +		(((mode) == BOND_MODE_8023AD)	||	\
> +		 ((mode) == BOND_MODE_TLB)	||	\
> +		 ((mode) == BOND_MODE_ALB))
> +
>  #define TX_QUEUE_OVERRIDE(mode)				\
>  			(((mode) == BOND_MODE_ACTIVEBACKUP) ||	\
>  			 ((mode) == BOND_MODE_ROUNDROBIN))
> 
One small note, you can save a few lines in bond_sysfs.c if you switch
the check in bonding_store_arp_interval() to the new macro BOND_NO_USES_ARP()
as it's identical.

Cheers,
 Nik

Re: [PATCH net v2] bonding: disable arp and enable mii monitoring when bond change to no uses arp mode

[Ding Tianhong]

On 2013/11/22 21:55, Nikolay Aleksandrov wrote:
> On 11/22/2013 03:12 AM, Ding Tianhong wrote:
>> Because the ARP monitoring is not support for 802.3ad, but I still
>> could change the mode to 802.3ad from ab mode while ARP monitoring
>> is running, it is incorrect.
>>
>> So add a check for 802.3ad in bonding_store_mode to fix the problem,
>> and make a new macro BOND_NO_USES_ARP() to simplify the code.
>>
>> v2: according to the Dan Williams's suggestion, bond mode is the most
>>     important bond option, it should override any of the other sub-options.
>>     So when the mode is changed, the conficting values should be cleared
>>     or reset, otherwise the user has to duplicate more operations to modify
>>     the logic. I disable the arp and enable mii monitoring when the bond mode
>>     is changed to AB, TB and 8023AD if the arp interval is true.
>>
>> Suggested-by: Dan Williams <dcbw@redhat.com>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>>  drivers/net/bonding/bond_options.c | 13 +++++++++----
>>  drivers/net/bonding/bonding.h      |  5 +++++
>>  2 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>> index 9a5223c..04364f7a 100644
>> --- a/drivers/net/bonding/bond_options.c
>> +++ b/drivers/net/bonding/bond_options.c
>> @@ -45,10 +45,15 @@ int bond_option_mode_set(struct bonding *bond, int mode)
>>  		return -EPERM;
>>  	}
>>  
>> -	if (BOND_MODE_IS_LB(mode) && bond->params.arp_interval) {
>> -		pr_err("%s: %s mode is incompatible with arp monitoring.\n",
>> -		       bond->dev->name, bond_mode_tbl[mode].modename);
>> -		return -EINVAL;
>> +	if (BOND_NO_USES_ARP(mode) && bond->params.arp_interval) {
>> +		pr_info("%s: %s mode is incompatible with arp monitoring, start mii monitoring\n",
>> +			bond->dev->name, bond_mode_tbl[mode].modename);
>> +		/* disable arp monitoring */
>> +		bond->params.arp_interval = 0;
>> +		/* set miimon to default value */
>> +		bond->params.miimon = 100;
>> +		pr_info("%s: Setting MII monitoring interval to %d.\n",
>> +			bond->dev->name, bond->params.miimon);
>>  	}
>>  
> Maybe define the "default" miimon value somewhere ? A value of 100 for miimon is
> used repeatedly in bond_check_params() as well, it'd be nice to give it a name,
> e.g. I had to grep around to see why you chose 100 to be that value.
> 

yes, I get it from bond_check_params(), I think it is time to give it a new name, MIIMON_DEFAULT_VALUE = 100.

>>  	/* don't cache arp_validate between modes */
>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>> index ca31286..a310fb5 100644
>> --- a/drivers/net/bonding/bonding.h
>> +++ b/drivers/net/bonding/bonding.h
>> @@ -55,6 +55,11 @@
>>  		 ((mode) == BOND_MODE_TLB)          ||	\
>>  		 ((mode) == BOND_MODE_ALB))
>>  
>> +#define BOND_NO_USES_ARP(mode)				\
>> +		(((mode) == BOND_MODE_8023AD)	||	\
>> +		 ((mode) == BOND_MODE_TLB)	||	\
>> +		 ((mode) == BOND_MODE_ALB))
>> +
>>  #define TX_QUEUE_OVERRIDE(mode)				\
>>  			(((mode) == BOND_MODE_ACTIVEBACKUP) ||	\
>>  			 ((mode) == BOND_MODE_ROUNDROBIN))
>>
> One small note, you can save a few lines in bond_sysfs.c if you switch
> the check in bonding_store_arp_interval() to the new macro BOND_NO_USES_ARP()
> as it's identical.
> 
> Cheers,
>  Nik

good idea.

Regards.
Ding

> 
> 
> .
> 

[PATCH net v3] bonding: disable arp and enable mii monitoring when bond change to no uses arp mode

[Ding Tianhong]

Because the ARP monitoring is not support for 802.3ad, but I still
could change the mode to 802.3ad from ab mode while ARP monitoring
is running, it is incorrect.

So add a check for 802.3ad in bonding_store_mode to fix the problem,
and make a new macro BOND_NO_USES_ARP() to simplify the code.

v2: according to the Dan Williams's suggestion, bond mode is the most
    important bond option, it should override any of the other sub-options.
    So when the mode is changed, the conficting values should be cleared
    or reset, otherwise the user has to duplicate more operations to modify
    the logic. I disable the arp and enable mii monitoring when the bond mode
    is changed to AB, TB and 8023AD if the arp interval is true.

v3: according to the Nik's suggestion, the default value of miimon should need
    a name, there is several place to use it, and the bond_store_arp_interval()
    could use micro BOND_NO_USES_ARP to make the code more simpify.

Suggested-by: Dan Williams <dcbw@redhat.com>
Suggested-by: Nikolay Aleksandrov <nikolay@redhat.com>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_main.c    |  4 ++--
 drivers/net/bonding/bond_options.c | 13 +++++++++----
 drivers/net/bonding/bond_sysfs.c   |  4 +---
 drivers/net/bonding/bonding.h      |  7 +++++++
 4 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 4dd5ee2..36eab0c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4110,7 +4110,7 @@ static int bond_check_params(struct bond_params *params)
 		if (!miimon) {
 			pr_warning("Warning: miimon must be specified, otherwise bonding will not detect link failure, speed and duplex which are essential for 802.3ad operation\n");
 			pr_warning("Forcing miimon to 100msec\n");
-			miimon = 100;
+			miimon = BOND_DEFAULT_MIIMON;
 		}
 	}
 
@@ -4147,7 +4147,7 @@ static int bond_check_params(struct bond_params *params)
 		if (!miimon) {
 			pr_warning("Warning: miimon must be specified, otherwise bonding will not detect link failure and link speed which are essential for TLB/ALB load balancing\n");
 			pr_warning("Forcing miimon to 100msec\n");
-			miimon = 100;
+			miimon = BOND_DEFAULT_MIIMON;
 		}
 	}
 
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 9a5223c..ea6f640 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -45,10 +45,15 @@ int bond_option_mode_set(struct bonding *bond, int mode)
 		return -EPERM;
 	}
 
-	if (BOND_MODE_IS_LB(mode) && bond->params.arp_interval) {
-		pr_err("%s: %s mode is incompatible with arp monitoring.\n",
-		       bond->dev->name, bond_mode_tbl[mode].modename);
-		return -EINVAL;
+	if (BOND_NO_USES_ARP(mode) && bond->params.arp_interval) {
+		pr_info("%s: %s mode is incompatible with arp monitoring, start mii monitoring\n",
+			bond->dev->name, bond_mode_tbl[mode].modename);
+		/* disable arp monitoring */
+		bond->params.arp_interval = 0;
+		/* set miimon to default value */
+		bond->params.miimon = BOND_DEFAULT_MIIMON;
+		pr_info("%s: Setting MII monitoring interval to %d.\n",
+			bond->dev->name, bond->params.miimon);
 	}
 
 	/* don't cache arp_validate between modes */
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 0ec2a7e..abf5e10 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -523,9 +523,7 @@ static ssize_t bonding_store_arp_interval(struct device *d,
 		ret = -EINVAL;
 		goto out;
 	}
-	if (bond->params.mode == BOND_MODE_ALB ||
-	    bond->params.mode == BOND_MODE_TLB ||
-	    bond->params.mode == BOND_MODE_8023AD) {
+	if (BOND_NO_USES_ARP(bond->params.mode)) {
 		pr_info("%s: ARP monitoring cannot be used with ALB/TLB/802.3ad. Only MII monitoring is supported on %s.\n",
 			bond->dev->name, bond->dev->name);
 		ret = -EINVAL;
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index ca31286..a9f4f9f 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -35,6 +35,8 @@
 
 #define BOND_MAX_ARP_TARGETS	16
 
+#define BOND_DEFAULT_MIIMON	100
+
 #define IS_UP(dev)					   \
 	      ((((dev)->flags & IFF_UP) == IFF_UP)	&& \
 	       netif_running(dev)			&& \
@@ -55,6 +57,11 @@
 		 ((mode) == BOND_MODE_TLB)          ||	\
 		 ((mode) == BOND_MODE_ALB))
 
+#define BOND_NO_USES_ARP(mode)				\
+		(((mode) == BOND_MODE_8023AD)	||	\
+		 ((mode) == BOND_MODE_TLB)	||	\
+		 ((mode) == BOND_MODE_ALB))
+
 #define TX_QUEUE_OVERRIDE(mode)				\
 			(((mode) == BOND_MODE_ACTIVEBACKUP) ||	\
 			 ((mode) == BOND_MODE_ROUNDROBIN))
-- 
1.8.0

Re: [PATCH net v3] bonding: disable arp and enable mii monitoring when bond change to no uses arp mode

[Dan Williams]

On Fri, 2013-11-22 at 22:28 +0800, Ding Tianhong wrote:
> Because the ARP monitoring is not support for 802.3ad, but I still
> could change the mode to 802.3ad from ab mode while ARP monitoring
> is running, it is incorrect.
> 
> So add a check for 802.3ad in bonding_store_mode to fix the problem,
> and make a new macro BOND_NO_USES_ARP() to simplify the code.

Excellent, that's exactly what I was thinking of.  Thanks!

Dan

> v2: according to the Dan Williams's suggestion, bond mode is the most
>     important bond option, it should override any of the other sub-options.
>     So when the mode is changed, the conficting values should be cleared
>     or reset, otherwise the user has to duplicate more operations to modify
>     the logic. I disable the arp and enable mii monitoring when the bond mode
>     is changed to AB, TB and 8023AD if the arp interval is true.
> 
> v3: according to the Nik's suggestion, the default value of miimon should need
>     a name, there is several place to use it, and the bond_store_arp_interval()
>     could use micro BOND_NO_USES_ARP to make the code more simpify.
> 
> Suggested-by: Dan Williams <dcbw@redhat.com>
> Suggested-by: Nikolay Aleksandrov <nikolay@redhat.com>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
>  drivers/net/bonding/bond_main.c    |  4 ++--
>  drivers/net/bonding/bond_options.c | 13 +++++++++----
>  drivers/net/bonding/bond_sysfs.c   |  4 +---
>  drivers/net/bonding/bonding.h      |  7 +++++++
>  4 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 4dd5ee2..36eab0c 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4110,7 +4110,7 @@ static int bond_check_params(struct bond_params *params)
>  		if (!miimon) {
>  			pr_warning("Warning: miimon must be specified, otherwise bonding will not detect link failure, speed and duplex which are essential for 802.3ad operation\n");
>  			pr_warning("Forcing miimon to 100msec\n");
> -			miimon = 100;
> +			miimon = BOND_DEFAULT_MIIMON;
>  		}
>  	}
>  
> @@ -4147,7 +4147,7 @@ static int bond_check_params(struct bond_params *params)
>  		if (!miimon) {
>  			pr_warning("Warning: miimon must be specified, otherwise bonding will not detect link failure and link speed which are essential for TLB/ALB load balancing\n");
>  			pr_warning("Forcing miimon to 100msec\n");
> -			miimon = 100;
> +			miimon = BOND_DEFAULT_MIIMON;
>  		}
>  	}
>  
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 9a5223c..ea6f640 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -45,10 +45,15 @@ int bond_option_mode_set(struct bonding *bond, int mode)
>  		return -EPERM;
>  	}
>  
> -	if (BOND_MODE_IS_LB(mode) && bond->params.arp_interval) {
> -		pr_err("%s: %s mode is incompatible with arp monitoring.\n",
> -		       bond->dev->name, bond_mode_tbl[mode].modename);
> -		return -EINVAL;
> +	if (BOND_NO_USES_ARP(mode) && bond->params.arp_interval) {
> +		pr_info("%s: %s mode is incompatible with arp monitoring, start mii monitoring\n",
> +			bond->dev->name, bond_mode_tbl[mode].modename);
> +		/* disable arp monitoring */
> +		bond->params.arp_interval = 0;
> +		/* set miimon to default value */
> +		bond->params.miimon = BOND_DEFAULT_MIIMON;
> +		pr_info("%s: Setting MII monitoring interval to %d.\n",
> +			bond->dev->name, bond->params.miimon);
>  	}
>  
>  	/* don't cache arp_validate between modes */
> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
> index 0ec2a7e..abf5e10 100644
> --- a/drivers/net/bonding/bond_sysfs.c
> +++ b/drivers/net/bonding/bond_sysfs.c
> @@ -523,9 +523,7 @@ static ssize_t bonding_store_arp_interval(struct device *d,
>  		ret = -EINVAL;
>  		goto out;
>  	}
> -	if (bond->params.mode == BOND_MODE_ALB ||
> -	    bond->params.mode == BOND_MODE_TLB ||
> -	    bond->params.mode == BOND_MODE_8023AD) {
> +	if (BOND_NO_USES_ARP(bond->params.mode)) {
>  		pr_info("%s: ARP monitoring cannot be used with ALB/TLB/802.3ad. Only MII monitoring is supported on %s.\n",
>  			bond->dev->name, bond->dev->name);
>  		ret = -EINVAL;
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index ca31286..a9f4f9f 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -35,6 +35,8 @@
>  
>  #define BOND_MAX_ARP_TARGETS	16
>  
> +#define BOND_DEFAULT_MIIMON	100
> +
>  #define IS_UP(dev)					   \
>  	      ((((dev)->flags & IFF_UP) == IFF_UP)	&& \
>  	       netif_running(dev)			&& \
> @@ -55,6 +57,11 @@
>  		 ((mode) == BOND_MODE_TLB)          ||	\
>  		 ((mode) == BOND_MODE_ALB))
>  
> +#define BOND_NO_USES_ARP(mode)				\
> +		(((mode) == BOND_MODE_8023AD)	||	\
> +		 ((mode) == BOND_MODE_TLB)	||	\
> +		 ((mode) == BOND_MODE_ALB))
> +
>  #define TX_QUEUE_OVERRIDE(mode)				\
>  			(((mode) == BOND_MODE_ACTIVEBACKUP) ||	\
>  			 ((mode) == BOND_MODE_ROUNDROBIN))

Re: [PATCH net v3] bonding: disable arp and enable mii monitoring when bond change to no uses arp mode

[Nikolay Aleksandrov]

On 11/22/2013 03:28 PM, Ding Tianhong wrote:
> Because the ARP monitoring is not support for 802.3ad, but I still
> could change the mode to 802.3ad from ab mode while ARP monitoring
> is running, it is incorrect.
> 
> So add a check for 802.3ad in bonding_store_mode to fix the problem,
> and make a new macro BOND_NO_USES_ARP() to simplify the code.
> 
> v2: according to the Dan Williams's suggestion, bond mode is the most
>     important bond option, it should override any of the other sub-options.
>     So when the mode is changed, the conficting values should be cleared
>     or reset, otherwise the user has to duplicate more operations to modify
>     the logic. I disable the arp and enable mii monitoring when the bond mode
>     is changed to AB, TB and 8023AD if the arp interval is true.
> 
> v3: according to the Nik's suggestion, the default value of miimon should need
>     a name, there is several place to use it, and the bond_store_arp_interval()
>     could use micro BOND_NO_USES_ARP to make the code more simpify.
> 
> Suggested-by: Dan Williams <dcbw@redhat.com>
> Suggested-by: Nikolay Aleksandrov <nikolay@redhat.com>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---

Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>

Re: [PATCH net v3] bonding: disable arp and enable mii monitoring when bond change to no uses arp mode

[David Miller]

From: Ding Tianhong <dingtianhong@huawei.com>
Date: Fri, 22 Nov 2013 22:28:43 +0800

> Because the ARP monitoring is not support for 802.3ad, but I still
> could change the mode to 802.3ad from ab mode while ARP monitoring
> is running, it is incorrect.
> 
> So add a check for 802.3ad in bonding_store_mode to fix the problem,
> and make a new macro BOND_NO_USES_ARP() to simplify the code.
> 
> v2: according to the Dan Williams's suggestion, bond mode is the most
>     important bond option, it should override any of the other sub-options.
>     So when the mode is changed, the conficting values should be cleared
>     or reset, otherwise the user has to duplicate more operations to modify
>     the logic. I disable the arp and enable mii monitoring when the bond mode
>     is changed to AB, TB and 8023AD if the arp interval is true.
> 
> v3: according to the Nik's suggestion, the default value of miimon should need
>     a name, there is several place to use it, and the bond_store_arp_interval()
>     could use micro BOND_NO_USES_ARP to make the code more simpify.
> 
> Suggested-by: Dan Williams <dcbw@redhat.com>
> Suggested-by: Nikolay Aleksandrov <nikolay@redhat.com>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

Applied, thanks for following up on this Ding.