Re: [PATCH v2 net] bonding: don't use stale speed and duplex information

[zhuyj <zyjzyj2000@gmail.com>]

On 02/29/2016 01:39 PM, Jay Vosburgh wrote:
> zhuyj <zyjzyj2000@gmail.com> wrote:
>
>> On 02/25/2016 09:33 PM, Jay Vosburgh wrote:
>>> zhuyj <zyjzyj2000@gmail.com> wrote:
>>> [...]
>>>> I delved into the source code and Emil's tests. I think that the problem
>>>> that this patch expects to fix occurs very unusually.
>>>>
>>>> Do you agree with me?
>>>>
>>>> If so, maybe the following patch can reduce the performance loss.
>>>> Please comment on it. Thanks a lot.
>>>>
>>>>
>>>> diff --git a/drivers/net/bonding/bond_main.c
>>>> b/drivers/net/bonding/bond_main.c
>>>> index b7f1a99..c4c511a 100644
>>>> --- a/drivers/net/bonding/bond_main.c
>>>> +++ b/drivers/net/bonding/bond_main.c
>>>> @@ -2129,7 +2129,9 @@ static void bond_miimon_commit(struct bonding *bond)
>>>>                          continue;
>>>>
>>>>                  case BOND_LINK_UP:
>>>> -                       bond_update_speed_duplex(slave);
>>>> +                       if (slave->speed == SPEED_UNKNOWN)
>>>> +                               bond_update_speed_duplex(slave);
>>>> +
>>>>                          bond_set_slave_link_state(slave, BOND_LINK_UP,
>>>> BOND_SLAVE_NOTIFY_NOW);
>>>>                          slave->last_link_up = jiffies;
>>> 	I don't believe the speed is necessarily SPEED_UNKNOWN coming in
>>> here.  If the race occurs at a time later than the initial enslavement,
>>> speed may already be set (and the race manifests if the new speed
>>> changes, i.e., the link changes from 1 Gb/sec to 10 Gb/sec), so I don't
>>> think this is functionally correct.
>> Hi, Jay
>>
>> Thanks for your reply.
>>
>> IMHO, "If the race occurs at a time later than the initial enslavement,
>> speed may already be set (and the race manifests if the new speed
>> changes, i.e., the link changes from 1 Gb/sec to 10 Gb/sec)", from my test,
>> this will not happen because the previous source code make the speed
>> correct.
> 	How, exactly, will "the previous source code make the speed
> correct"?
>
>> This "bond_update_speed_duplex" repeats to get the correct speed.
>>
>> That is, this patch is to fix the error in initial enslavement. The
>> mentioned scenario will not occur.
> 	I see nothing in the code that limits the race to happening only
> at enslavement time.
>
> 	If the bond_mii_monitor call executes between the device going
> link up and the arrival of the NETDEV_CHANGE or NETDEV_UP callback, the
> stored speed and duplex are stale.  The stale speed value is not
> guaranteed to be SPEED_UNKNOWN, so your patch is not functionally
> correct.

Hi, Jay

In this function bond_slave_netdev_event, the speed is updated.

Best Regards!
Zhu Yanjun

>
> 	-J
>
>> Even though the performance impact is minimal, if we can avoid this
>> performance
>> impact, why not ?
>>
>> Best Regards!
>> Zhu Yanjun
>>
>>> 	Also, the call to bond_miimon_commit itself is already gated by
>>> bond_miimon_inspect finding a link state change.  The performance impact
>>> here should be minimal.
>>>
>>> 	-J
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com