zhuyj <[email protected]> wrote:
>On 02/25/2016 09:33 PM, Jay Vosburgh wrote:
>> zhuyj <[email protected]> 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.
-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, [email protected]
