-
Notifications
You must be signed in to change notification settings - Fork 146
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
Treat vhost-vdpa devices as DPDK devices #163
Conversation
@@ -1,3 +1,4 @@ | |||
github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= |
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.
I notice this project is unmaintained.
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.
as govdpa offers CLI, thats an indirect dependency introduced by github.com/urfave/cli
which as far as i see is maintained so if any issues arise, i guess they will either remove this dependency/fork/use something else.
also the code here invokes govdpa directly and does not use the cli
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.
@martinkennelly how did you check the project is unmaintained, is it via some tools? just curious since I don't know how this could be done.
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 spot on - its not a problem for this patch.
@zshi-redhat No tool unfortunately. I manually went through the added repositories to check.
@@ -204,6 +206,15 @@ func HasDpdkDriver(pciAddr string) (bool, error) { | |||
return true, nil | |||
} | |||
} | |||
|
|||
/* If there is a vdpa device associated with the device and it is bound to |
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.
so i guess eventually we would want a VDPA code path here right ? (e.g HasVdpaDriver() ?)
or possibly some clever renaming to cover both cases :)
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.
Technically, a vhost-vdpa driver is a dpdkDriver, so wouldn't HasDpdkDriver
cover both cases?
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 vdpa drivers(virtio_vdpa
vhost_vdpa
) need to be added in UserspaceDrivers
slice for HasDpdkDriver
to detect them, no?
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.
@ahalim-intel. The vdpa drivers cannot be added to UserspaceDrivers
because they are not pci drivers. They are vdpa-bus drivers. The driver that is used at the pci-bus level is independent of whether a vdpa device is created or not and also independent of what specific vdpa driver is used.
/* If there is a vdpa device associated with the device and it is bound to | ||
vhost-vdpa driver, it shall be treated as a dpdk device | ||
*/ | ||
if vdpaDev, err := kvdpa.GetVdpaDeviceByPci(pciAddr); err != nil && |
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.
Is it 1:1 mapping between VF and vDPA dev? I remembered there was discussion about 1 pd => n vdev
where pd is anchor device like mlx5_core.vdpa
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.
currently i think its 1:1 (and is statically created for mlx5 vdpa) but:
- there is work to make it one to many
- ongoing work to transition to dynamic creation, adding uapi to kernel, vdpa device creation using vdpa tool (iproute2)
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.
Right. It is unclear to me how 1:N mapping would be handled by the SR-IOV Device Plugin.
If it does, it could save the detail information (since the PCIAddress is no longer unique) in the device-info-spec file which the CNI could read.
But I guess that's work for the future?
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.
yes, this will be for future work.
PR looks good to me. |
Use the govdpa library to inspect the device and determine if it's a vdpa device and its driver. If it's a vhost device, the CNI should not try to do any netdevice configuration. Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
Use the govdpa library to inspect the device and determine if it's a
vdpa device and its driver.
If it's a vhost device, the CNI should not try to do any netdevice
configuration.
Fixes: #162
Signed-off-by: Adrian Moreno amorenoz@redhat.com