vhost-vdpa support #467

lmilleri · 2023-07-10T13:50:50Z

This PR implements the changes for vhost-vdpa support.

SriovNetworkNodePolicy API: VDPA device type can take now the following values: "virtio" and "vhost"
if vhost is specified, the vhost-vdpa driver is loaded into the kernel and bound to the vdpa device

github-actions · 2023-07-10T13:51:04Z

Thanks for your PR,
To run vendors CIs 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 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 · 2023-07-10T13:58:07Z

Pull Request Test Coverage Report for Build 5948839312

56 of 77 (72.73%) changed or added relevant lines in 2 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.4%) to 24.801%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/plugins/generic/generic_plugin.go	50	71	70.42%

Files with Coverage Reduction	New Missed Lines	%
pkg/plugins/generic/generic_plugin.go	3	50.53%

Totals
Change from base Build 5939485088:	0.4%
Covered Lines:	2122
Relevant Lines:	8556

💛 - Coveralls

github-actions · 2023-07-13T12:54:28Z

Thanks for your PR,
To run vendors CIs 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 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.

github-actions · 2023-07-14T15:46:08Z

Thanks for your PR,
To run vendors CIs 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 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.

lmilleri · 2023-07-17T07:20:22Z

@bn222 @SchSeba @adrianchiris @zshi-redhat Please review and approve if looks good

bn222 · 2023-07-17T08:55:48Z

pkg/plugins/generic/generic_plugin.go

@@ -69,7 +71,7 @@ func (p *GenericPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeSt
 	p.DesireState = new

 	needDrain = needDrainNode(new.Spec.Interfaces, new.Status.Interfaces)
-	needReboot = needRebootNode(new, &p.LoadVfioDriver, &p.LoadVirtioVdpaDriver)
+	needReboot = needRebootNode(new, &p.LoadVfioDriver, &p.LoadVirtioVdpaDriver, &p.LoadVhostVdpaDriver)


This now starts to look cumbersome. Can we start this PR with a refactor of p.LoadVfioDriver and vdpa driver? What's happening here is that the needRebootNode actually returns a bunch of things through its arguments. That function is now doing much more than "just checking if we need to reboot".

We also have an enum and a small state machine to load drivers. This could be simplified much further.

@bn222 pushing the refactoring of driver loading logic

github-actions · 2023-07-18T13:33:47Z

Thanks for your PR,
To run vendors CIs 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 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.

github-actions · 2023-07-18T13:36:44Z

Thanks for your PR,
To run vendors CIs 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 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.

pkg/plugins/generic/generic_plugin.go

github-actions · 2023-07-19T08:41:18Z

Thanks for your PR,
To run vendors CIs 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 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.

adrianchiris · 2023-07-23T14:03:40Z

pkg/plugins/generic/generic_plugin.go

+	return p.DriverStateMap
+}
+
+func (p *GenericPlugin) TestDriverLoading(state *sriovnetworkv1.SriovNetworkNodeState) {


Nit: LoadDriverForTests ?

pkg/plugins/generic/generic_plugin.go

adrianchiris · 2023-07-23T14:28:28Z

pkg/plugins/generic/generic_plugin.go

-		if needVfioDriver(state) {
-			*loadVfioDriver = loading
-			update, err := tryEnableIommuInKernelArgs()
+func nodeToRebootIfVfio(state *sriovnetworkv1.SriovNetworkNodeState, driverMap DriverStateMapType) (updateNode bool) {


nodeToRebootIfVfio ? maybe needRebootIfVfio ?

also lets use "needReboot" as return val as thats what it means.

adrianchiris · 2023-07-23T14:31:56Z

pkg/plugins/generic/generic_plugin.go

-			update, err := tryEnableIommuInKernelArgs()
+func nodeToRebootIfVfio(state *sriovnetworkv1.SriovNetworkNodeState, driverMap DriverStateMapType) (updateNode bool) {
+	updateNode = false
+	for driverID, driverState := range driverMap {


driverMap is a .... map.

no need to iterate over it. instead just index it by Vfio

adrianchiris · 2023-07-23T14:34:29Z

pkg/plugins/generic/generic_plugin.go

-			*loadVfioDriver = loading
-			update, err := tryEnableIommuInKernelArgs()
+func nodeToRebootIfVfio(state *sriovnetworkv1.SriovNetworkNodeState, driverMap DriverStateMapType) (updateNode bool) {
+	updateNode = false


this is by default false. so can be removed.

adrianchiris · 2023-07-23T14:34:34Z

pkg/plugins/generic/generic_plugin.go

-		}
-	}
+func needRebootNode(state *sriovnetworkv1.SriovNetworkNodeState, driverMap DriverStateMapType) (needReboot bool) {
+	needReboot = false


this is by default false. so can be removed.

adrianchiris · 2023-07-23T14:37:48Z

pkg/plugins/generic/generic_plugin.go


 	if needReboot {
 		needDrain = true
 	}
 	return
 }

+// ////////////// for testing purposes only ///////////////////////


move those to the end of the file ?

github-actions · 2023-07-24T09:28:51Z

Thanks for your PR,
To run vendors CIs 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 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.

github-actions · 2023-07-24T09:44:12Z

Thanks for your PR,
To run vendors CIs 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 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.

SchSeba · 2023-08-01T15:01:27Z

bindata/manifests/switchdev-config/files/switchdev-configuration-after-nm.sh.yaml

-            if [ $vfid -le $maxId ] && [ $vfid -ge $minId ] && [ $vdpaType == "virtio" ]; then
-              vdpa_cmd="vdpa dev add name vdpa:"${VfPciAddr}" mgmtdev pci/"${VfPciAddr}
+            if [ $vfid -le $maxId ] && [ $vfid -ge $minId ] && [ $vdpaType != "" ]; then
+              vdpa_cmd="vdpa dev add name vdpa:"${VfPciAddr}" mgmtdev pci/"${VfPciAddr}" max_vqp 32"


what this means? max_vqp 32 it will always be this number? should it be configurable?

Yes, you're right, this will be configurable in the future by extending the API? For the time being, the idea is to enable the multiqueue support by providing by default 32 queues. We think it is the best value, considering that the virtio-net driver will use the number of cores as a maximum. The reason for enabling multiqueue is to increase the performances of the card and be ready to demonstrate it to Verizon

SchSeba · 2023-08-01T15:04:21Z

pkg/plugins/generic/generic_plugin.go

-		if err := p.HostManager.LoadKernelModule("vfio_pci"); err != nil {
-			glog.Errorf("generic-plugin Apply(): fail to load vfio_pci kmod: %v", err)
-			return err
+func syncDriverState(p *GenericPlugin) error {


please have this function as part of the struct no need to pass it

SchSeba · 2023-08-01T15:16:52Z

pkg/plugins/generic/generic_plugin.go

+	for _, driverState := range p.DriverStateMap {
+		if !driverState.DriverLoaded && driverState.NeedDriverFunc(p.DesireState, driverState) {
+			if err := p.HostManager.LoadKernelModule(driverState.DriverName); err != nil {
+				glog.Errorf("generic-plugin Apply(): fail to load %s kmod: %v", driverState.DriverName, err)


please change the function on the log generic-plugin Apply()

SchSeba · 2023-08-01T15:17:16Z

pkg/plugins/generic/generic_plugin.go

+func syncDriverState(p *GenericPlugin) error {
+	for _, driverState := range p.DriverStateMap {
+		if !driverState.DriverLoaded && driverState.NeedDriverFunc(p.DesireState, driverState) {
+			if err := p.HostManager.LoadKernelModule(driverState.DriverName); err != nil {


please add a V(2) log say what driver it's loading (good for debug)

SchSeba · 2023-08-01T15:18:23Z

pkg/plugins/generic/generic_plugin.go

@@ -104,6 +144,11 @@ func (p *GenericPlugin) Apply() error {
 		}
 	}

+	err := syncDriverState(p)


please just simply this like

if err := p.syncDrvierStates(); err != nil { return err }

SchSeba · 2023-08-01T15:20:33Z

pkg/plugins/generic/generic_plugin.go

 	return
 }
+
+// ////////////// for testing purposes only ///////////////////////


nit: you have a space here :)

github-actions · 2023-08-02T07:59:08Z

Thanks for your PR,
To run vendors CIs 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 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.

github-actions · 2023-08-04T08:13:53Z

Thanks for your PR,
To run vendors CIs 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 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.

github-actions · 2023-08-08T08:08:36Z

Thanks for your PR,
To run vendors CIs 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 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.

adrianchiris

lgtm from my side.

one small comment RE log verbosity

adrianchiris · 2023-08-16T14:59:09Z

pkg/plugins/generic/generic_plugin.go

+func (p *GenericPlugin) syncDriverState() error {
+	for _, driverState := range p.DriverStateMap {
+		if !driverState.DriverLoaded && driverState.NeedDriverFunc(p.DesireState, driverState) {
+			glog.Infof("loading driver %s", driverState.DriverName)


nit: use glog.V(2).Infof as @SchSeba suggested below.

github-actions · 2023-08-16T15:20:06Z

Thanks for your PR,
To run vendors CIs 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 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.

adrianchiris · 2023-08-16T15:29:15Z

thanks @lmilleri !

adrianchiris · 2023-08-21T12:02:23Z

@SchSeba @zeeke shall we proceeed with merging this one ?

zeeke

Only one nit comment

Overall, LGTM and good testing coverage. Thanks

pkg/plugins/generic/generic_plugin.go

Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>

Max number of queues is set to 32 by default. This setting should help with the performance of NVIDIA network cards. Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>

github-actions · 2023-08-23T08:11:44Z

Thanks for your PR,
To run vendors CIs 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 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.

SriovNetworkNodePolicy API: VDPA device type can take now the following values: "virtio" and "vhost" Vhost/vdpa: Same behaviour of virtio/vdpa apart for the drivers that are loaded in the kernel (vhost-vdpa instead of virtio-vdpa) Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>

github-actions · 2023-08-23T08:21:32Z

Thanks for your PR,
To run vendors CIs 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 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.

lmilleri force-pushed the host-vdpa branch from f7c9317 to 4175d3b Compare July 13, 2023 12:54

lmilleri force-pushed the host-vdpa branch from 4175d3b to 79f227b Compare July 14, 2023 15:45

bn222 reviewed Jul 17, 2023

View reviewed changes

lmilleri force-pushed the host-vdpa branch from 79f227b to 97c0775 Compare July 18, 2023 13:33

lmilleri force-pushed the host-vdpa branch from 97c0775 to 25222d7 Compare July 18, 2023 13:36

bn222 reviewed Jul 18, 2023

View reviewed changes

pkg/plugins/generic/generic_plugin.go Show resolved Hide resolved

lmilleri force-pushed the host-vdpa branch from 25222d7 to 18ba319 Compare July 19, 2023 08:41

adrianchiris requested a review from SchSeba July 23, 2023 13:31

adrianchiris reviewed Jul 23, 2023

View reviewed changes

lmilleri force-pushed the host-vdpa branch from 18ba319 to e8d47cd Compare July 24, 2023 09:28

lmilleri force-pushed the host-vdpa branch from e8d47cd to 5a4fa02 Compare July 24, 2023 09:43

SchSeba reviewed Aug 1, 2023

View reviewed changes

lmilleri force-pushed the host-vdpa branch from 5a4fa02 to 70f916c Compare August 2, 2023 07:58

lmilleri force-pushed the host-vdpa branch from 70f916c to f3ebe10 Compare August 4, 2023 08:13

lmilleri force-pushed the host-vdpa branch from f3ebe10 to b033857 Compare August 8, 2023 08:08

adrianchiris approved these changes Aug 16, 2023

View reviewed changes

lmilleri force-pushed the host-vdpa branch from b033857 to 6a976e8 Compare August 16, 2023 15:19

adrianchiris approved these changes Aug 16, 2023

View reviewed changes

zeeke approved these changes Aug 21, 2023

View reviewed changes

pkg/plugins/generic/generic_plugin.go Outdated Show resolved Hide resolved

lmilleri added 2 commits August 23, 2023 09:11

Driver loading refactoring

75e7d15

Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>

vdpa multiqueue enabled

1a5faae

Max number of queues is set to 32 by default. This setting should help with the performance of NVIDIA network cards. Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>

lmilleri force-pushed the host-vdpa branch from 6a976e8 to b418d6f Compare August 23, 2023 08:11

lmilleri force-pushed the host-vdpa branch from b418d6f to 688979d Compare August 23, 2023 08:21

zeeke merged commit ee0e017 into k8snetworkplumbingwg:master Aug 23, 2023
9 checks passed

vhost-vdpa support #467

vhost-vdpa support #467

Conversation

lmilleri commented Jul 10, 2023

github-actions bot commented Jul 10, 2023

coveralls commented Jul 10, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5948839312

💛 - Coveralls

github-actions bot commented Jul 13, 2023

github-actions bot commented Jul 14, 2023

lmilleri commented Jul 17, 2023

bn222 Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 18, 2023

github-actions bot commented Jul 18, 2023

github-actions bot commented Jul 19, 2023

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

github-actions bot commented Jul 24, 2023

github-actions bot commented Jul 24, 2023

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

github-actions bot commented Aug 2, 2023

github-actions bot commented Aug 4, 2023

github-actions bot commented Aug 8, 2023

adrianchiris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 16, 2023

adrianchiris commented Aug 16, 2023

adrianchiris commented Aug 21, 2023

zeeke left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 23, 2023

github-actions bot commented Aug 23, 2023

coveralls commented Jul 10, 2023 •

edited

Loading

bn222 Jul 17, 2023 •

edited

Loading