Intel switchdev #757

zeeke · 2024-08-20T10:45:20Z

Ice driver supports creating VFs after the eSwitcMode is
set to switchdev. This is different than the preferred way
for mlx5 driver.

Add specific functions to configure eSwitchMode and SriovNumVFs
for each driver.

Add a basic test to loop over all available
devices and test if setting eSwitchMode: switchdev works.

github-actions · 2024-08-20T10:45:32Z

Thanks for your PR,
To run vendors CIs, Maintainers can use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

coveralls · 2024-08-20T17:09:59Z

Pull Request Test Coverage Report for Build 10612389872

Details

32 of 56 (57.14%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.03%) to 45.055%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/host/internal/sriov/sriov.go	32	56	57.14%

Totals
Change from base Build 10594418576:	0.03%
Covered Lines:	6628
Relevant Lines:	14711

💛 - Coveralls

ykulazhenkov

LGTM

Eoghan1232 · 2024-08-26T13:53:01Z

I will do a test of this

Eoghan1232 · 2024-08-26T16:08:03Z

worked like a charm @zeeke ;)

let me give the PR a review now then we should be good to merge

Eoghan1232

Changes LGTM!

once this is merged, we should also update the support HW list.
I will do this

SchSeba · 2024-08-27T17:20:09Z

pkg/host/internal/sriov/sriov.go

-		log.Log.Error(err, "setEswitchMode(): failed to set mode", "device", pciAddr, "mode", eswitchMode)
-		return err
+
+	type setEswitchModeAndNumVFsFn func(string, string, int) error


nit: can we move this one outside the function?

ok, moving. though that type is not supposed to be used out of this function

SchSeba · 2024-08-27T17:20:34Z

pkg/host/internal/sriov/sriov.go

+
+	type setEswitchModeAndNumVFsFn func(string, string, int) error
+
+	var setEswitchModeAndNumVFsByDriverName map[string]setEswitchModeAndNumVFsFn = map[string]setEswitchModeAndNumVFsFn{


nit: can we do something like

setEswitchModeAndNumVFsByDriverName map[string]setEswitchModeAndNumVFsFn := map[string]setEswitchModeAndNumVFsFn{...}

pkg/host/internal/sriov/sriov.go

SchSeba · 2024-08-27T17:22:42Z

pkg/host/internal/sriov/sriov.go

 }

-func (s *sriov) setEswitchModeAndNumVFs(pciAddr string, desiredEswitchMode string, numVFs int) error {
-	log.Log.V(2).Info("setEswitchModeAndNumVFs(): configure VFs for device",
+func (s *sriov) setEswitchModeAndNumVFsMlx(pciAddr string, desiredEswitchMode string, numVFs int) error {


please add a function comment to explain the process of how we configure switchDev virtual functions for this case

nit: between b. and c. we also unbind all VFs on PF.
can you add this to the comment ?

note: rebind happens after part of configSriovVFDevices

also what about configureHWOptionsForSwitchdev ? is it the same for intel ice ?

if we need to split that as well, maybe its worth moving switchdev related things to switchdev to switchdev pkg under host folder to avoid bloating sriov pkg.

Adding the comment about "driver unbinding".

Regarding configureHWOptionsForSwitchdev, current implementation is about setting flow_steering_mode=smfs, which is Mellanox only (IIUC).

Ice driver doesn't need this step [1] . @Eoghan1232 Can you confirm that?

[1] https://edc.intel.com/content/www/us/en/design/products/ethernet/appnote-e810-eswitch-switchdev-mode-config-guide/script-a-switchdev-mode-with-linux-bridge-configuration/

switchdev and sriov general configurations are tightly coupled. I don't know if moving the switchdev part in a specific package simplifies the code

I don't think we need to unbind driver, since we need to reset back to 0 vf's anyways

Got it. Then better move the unbindAllVFsOnPF(...) call outside of setEswitchMode(...)

Regarding configureHWOptionsForSwitchdev, current implementation is about setting flow_steering_mode=smfs, which is Mellanox only (IIUC).

this also enables hw-tc-offload via ethtool.

Regarding configureHWOptionsForSwitchdev, current implementation is about setting flow_steering_mode=smfs, which is Mellanox only (IIUC).

this also enables hw-tc-offload via ethtool.

right! I miss that. I checked and it should not be a problem for the ice driver:

from a local test:

2024-08-29T08:21:49.27421628Z LEVEL(-2) sriov/sriov.go:365 EnableHwTcOffload(): enable offloading {"device": "eno12409"} 2024-08-29T08:21:49.274526639Z LEVEL(-2) sriov/sriov.go:365 EnableHwTcOffload(): already enabled {"device": "eno12409"}

and flow_steering_mode setting is guarded: if it's not supported, it doesn't hurt.

pkg/host/internal/sriov/sriov.go

SchSeba · 2024-08-27T17:26:58Z

test/conformance/tests/test_switchdev.go

+
+	BeforeAll(func() {
+		if cluster.VirtualCluster() {
+			Skip("IGB driver does not support VF statistics")


nit: please update the skip message here :)

SchSeba · 2024-08-27T17:33:11Z

test/conformance/tests/test_switchdev.go

+
+		By(fmt.Sprintf("Testing on node %s, %d devices found", testNode, len(interfaces)))
+
+		mainDevice := findMainSriovDevice(getConfigDaemonPod(testNode), interfaces)


you can use findUnusedSriovDevices to have the list of non main device :)

cool! thnks

adrianchiris · 2024-08-28T09:57:52Z

Please hold off with merging this one id like to go over this pr by end of week

Eoghan1232 · 2024-08-28T10:07:03Z

pkg/host/internal/sriov/sriov.go

@@ -1029,6 +1047,40 @@ func (s *sriov) setEswitchModeAndNumVFs(pciAddr string, desiredEswitchMode strin
 	return nil
 }

+// setEswitchModeAndNumVFsIce configures PF eSwitch and sriov_numvfs in the following order:
+// a. set eSwitchMode to `switchdev` if requested


we need to reset vf's to 0 before setting switchdev mode, is this handled?

No, I'll fix it.
sriov_numvfs needs to be set to 0 every time we change between eSwitchMode, right?
Both legacy -> switchdev and switchdev -> legacy.

I believe so, otherwise driver will throw an error I believe.
I can double check this tomorrow to confirm

I can see in some docs that we must have 0 VF's before switching to switchdev mode.

some changes were pushed to the PR, need to take another look on it

adrianchiris

overall lgtm, left one small comment

Add a basic test to loop over all available devices and test if setting `eSwitchMode: switchdev` works. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>

Ice driver supports creating VFs after the eSwitcMode is set to switchdev. This is different than the preferred way for `mlx5` driver. Add specific functions to configure eSwitchMode and SriovNumVFs for each driver. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>

SchSeba

LGTM

nice work!

zeeke · 2024-09-17T12:58:20Z

Got approval from multiple sides, merging.

zeeke force-pushed the intel-switchdev branch 4 times, most recently from 44b682b to 9486f37 Compare August 20, 2024 17:00

zeeke force-pushed the intel-switchdev branch from 9486f37 to 58cbb96 Compare August 21, 2024 06:36

ykulazhenkov previously approved these changes Aug 22, 2024

View reviewed changes

Eoghan1232 approved these changes Aug 26, 2024

View reviewed changes

SchSeba reviewed Aug 27, 2024

View reviewed changes

zeeke force-pushed the intel-switchdev branch 3 times, most recently from bbad045 to ea50542 Compare August 28, 2024 09:56

Eoghan1232 reviewed Aug 28, 2024

View reviewed changes

ykulazhenkov self-requested a review August 28, 2024 10:16

adrianchiris reviewed Aug 28, 2024

View reviewed changes

zeeke force-pushed the intel-switchdev branch from ea50542 to 3daf254 Compare August 28, 2024 17:30

eSwitch: e2e test for Switchdev mode

1beef18

Add a basic test to loop over all available devices and test if setting `eSwitchMode: switchdev` works. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>

zeeke force-pushed the intel-switchdev branch 2 times, most recently from a6508fe to 06d6c52 Compare August 29, 2024 08:33

zeeke force-pushed the intel-switchdev branch from 06d6c52 to 976e657 Compare August 29, 2024 09:25

SchSeba approved these changes Sep 2, 2024

View reviewed changes

e0ne approved these changes Sep 9, 2024

View reviewed changes

zeeke merged commit e812d35 into k8snetworkplumbingwg:master Sep 17, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intel switchdev #757

Intel switchdev #757

zeeke commented Aug 20, 2024

github-actions bot commented Aug 20, 2024

coveralls commented Aug 20, 2024 •

edited

Loading

ykulazhenkov left a comment

Eoghan1232 commented Aug 26, 2024

Eoghan1232 commented Aug 26, 2024

Eoghan1232 left a comment

SchSeba Aug 27, 2024

zeeke Aug 28, 2024

SchSeba Aug 27, 2024

SchSeba Aug 27, 2024

adrianchiris Aug 28, 2024 •

edited

Loading

zeeke Aug 28, 2024

Eoghan1232 Aug 28, 2024

zeeke Aug 29, 2024

adrianchiris Aug 29, 2024

zeeke Aug 29, 2024

SchSeba Aug 27, 2024

SchSeba Aug 27, 2024

zeeke Aug 28, 2024

adrianchiris commented Aug 28, 2024

Eoghan1232 Aug 28, 2024

zeeke Aug 28, 2024

Eoghan1232 Aug 28, 2024

Eoghan1232 Aug 28, 2024

adrianchiris left a comment

SchSeba left a comment

zeeke commented Sep 17, 2024


		type setEswitchModeAndNumVFsFn func(string, string, int) error

		var setEswitchModeAndNumVFsByDriverName map[string]setEswitchModeAndNumVFsFn = map[string]setEswitchModeAndNumVFsFn{


		By(fmt.Sprintf("Testing on node %s, %d devices found", testNode, len(interfaces)))

		mainDevice := findMainSriovDevice(getConfigDaemonPod(testNode), interfaces)

Intel switchdev #757

Intel switchdev #757

Conversation

zeeke commented Aug 20, 2024

github-actions bot commented Aug 20, 2024

coveralls commented Aug 20, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10612389872

Details

💛 - Coveralls

ykulazhenkov left a comment

Choose a reason for hiding this comment

Eoghan1232 commented Aug 26, 2024

Eoghan1232 commented Aug 26, 2024

Eoghan1232 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris left a comment

Choose a reason for hiding this comment

SchSeba left a comment

Choose a reason for hiding this comment

zeeke commented Sep 17, 2024

coveralls commented Aug 20, 2024 •

edited

Loading

adrianchiris Aug 28, 2024 •

edited

Loading