zhuyj <zyjzyj2...@gmail.com> wrote: >On 02/25/2016 09:33 PM, Jay Vosburgh wrote: >> zhuyj <zyjzyj2...@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. -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.vosbu...@canonical.com