nbi_impl: set speed/duplex before setting link up #451
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KanjiMonster
force-pushed
the
jogo_set_speed_while_down
branch
from
50dc3cf
to
c54a3ee
Compare
|
This should at least fix the spurious test failures with LACP bonds. |
|
Ran the link-aggr-lacp-systemd test for a few hours in a loop, and it didn't fail, while it failed after a while without the changes. |
Link speed/duplex changes do not trigger any notifications, only link state changes do. Therefore we need to make sure that speed/duplex is set correctly before setting the link state to up for ports. When setting the link state first, it leaves a window where a process reacting to the link state event may read out old speed/duplex before we had a change to update them, and the default speed/duplex is 10mbit/half for TAP. This can for example lead to breakage in LACP, where it uses the link speed to calculate the actor key. If it uses the wrong/old speed, it calculates a different key than the other side, and LACP will then fail to synchronize, breaking the link. So make sure to set the speed and duplex first on link up. On link down, we do not need to set any speed or duplex values, since their values are undefined. Fixes: b5fe46c ("tap_manager: set port speed on tap interfaces") Signed-off-by: Jonas Gorski <[email protected]>
KanjiMonster
force-pushed
the
jogo_set_speed_while_down
branch
from
c54a3ee
to
eac8d30
Compare
|
Simplified the change a bit - we don't need to set speed/duplex when there is no link (values are more or less undefined). |
rubensfig
approved these changes
Sep 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link speed/duplex changes do not trigger any notifications, only link state changes do. Therefore we need to make sure that speed/duplex is set correctly before setting the link state to up for ports.
When setting the link state first, it leaves a window where a process reacting to the link state event may read out old speed/duplex before we had a change to update them, and the default speed/duplex is 10mbit/half for TAP.
This can for example lead to breakage in LACP, where it uses the link speed to calculate the actor key. If it uses the wrong/old speed, it calculates a different key than the other side, and LACP will then fail to synchronize, breaking the link.
So make sure to set the speed and duplex first on link up, and reverse on link down.
Fixes: b5fe46c ("tap_manager: set port speed on tap interfaces")