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

[Nikolay Aleksandrov <nikolay@redhat.com>]

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