-
Notifications
You must be signed in to change notification settings - Fork 114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add switchdev-configuration-after-NM service #202
Add switchdev-configuration-after-NM service #202
Conversation
f85c531
to
dcefeab
Compare
Is this a temporary workaround before we fix the issue in the kernel? |
Not a temporary fix, this is required for VF LAG feature to function properly. |
fi | ||
|
||
# Required for NetworkManager configuration(e.g. bond) to settle down | ||
sleep 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use nmcli
or ip
command to check the link state, instead of using sleep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not be easy since we don't know what configuration the nmcli or ip should wait. there could be cases that additional bond or other NM configuration is not configured by user.
Description=Binds SRIOV VFs into switchdev driver | ||
# Removal of this file signals firstboot completion | ||
ConditionPathExists=!/etc/ignition-machine-config-encapsulated.json | ||
# This service is used to move a SRIOV NIC into switchdev mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is not accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -0,0 +1,35 @@ | |||
mode: 0755 | |||
overwrite: true | |||
path: "/usr/local/bin/bind-switchdev.sh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name looks unclear to me. Since we split the switchdev-configuration.service into two portion. Shall we use the name to switchdev-configuration-before-NM.service
and switchdev-configuration-after-NM.service
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
dcefeab
to
156b9bd
Compare
In the commit message, shall it be |
updated |
/lgtm |
/cc @adrianchiris @e0ne |
fi | ||
|
||
# Required for NetworkManager configuration(e.g. bond) to settle down | ||
sleep 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if we dont have this sleep ? do you know what configurations need to settle down ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened was that VF LAG didn't take effect (there is the dmesg indicating the error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrianchiris any follow-up questions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no additional questions. VF Lag has some quirks :(
@@ -67,12 +67,5 @@ contents: | |||
|
|||
# turn hw-tc-offload on | |||
/usr/sbin/ethtool -K ${name} hw-tc-offload on | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to this PR, but we could probably save some time during boot if we set sriov_drivers_autoprobe
to 0
then there is no need to unload drivers in line 57
- echo 0 > /sys/bus/pci/devices/PCI_DBDF_OF_PF/sriov_drivers_autoprobe
- echo XXX > /sys/bus/pci/devices/PCI_DBDF_OF_PF/sriov_numvfs
- Restore drivers autoprobe
echo 1 > /sys/bus/pci/devices/PCI_DBDF_OF_PF/sriov_drivers_autoprobe
if this sounds like something we like, i can open an enhancement on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. Can echo 1 > /sys/bus/pci/devices/PCI_DBDF_OF_PF/sriov_drivers_autoprobe
load the driver to VFs as we do in line 75?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to re-enable sriov_drivers_autoprobe before binding driver in L75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done #214
Switchdev-configuration-before-NM service rebinds VF to its kernel driver and executes after NetworkManager service, this is required for features such as VF LAG to take effect when bond or other network configurtion are configured through NetworkManager service. Signed-off-by: Zenghui Shi <[email protected]>
156b9bd
to
65f4cf9
Compare
resolved merge conflict. |
Thanks for your PR,
To skip the vendors CIs use one of:
|
Switchdev-configuration-after-NM service rebinds VF to its
driver and executes after NetworkManager service, this is
required for features such as VF LAG to take effect when
bond or other network configurtion are configured through
NetworkManager service.
Signed-off-by: Zenghui Shi [email protected]