Create VGPU changes with VFIO Framework#139
Conversation
fffa26f to
cbe892c
Compare
Signed-off-by: Arjun <agadiyar@nvidia.com>
cbe892c to
3f31828
Compare
Signed-off-by: Arjun <agadiyar@nvidia.com>
1682d72 to
4a52cbe
Compare
cdesiniotis
left a comment
There was a problem hiding this comment.
Thanks @JunAr7112, this is a good start. As we make iterations on this and get more familiar with the internals here, it may be valuable to create a new internal/vgpu package that hides away the vfio vs mdev framework complexity. We need to think through what the right interface would be, but I imagine we will need methods for 1) getting all vGPU devices, 2) getting all parent devices (of which you can create a vGPU device on top of), 3) creating a vGPU device. The pkg/vgpu/config.go file, which is concerned with getting / setting a particular vGPU config, can invoke these methods without having to know what vfio / mdev is.
8846795 to
5daf473
Compare
5daf473 to
89a1e62
Compare
15a9586 to
b1fd32d
Compare
Signed-off-by: Arjun <agadiyar@nvidia.com>
ae21d52 to
50e2185
Compare
Signed-off-by: Arjun <agadiyar@nvidia.com>
50e2185 to
58c7cc6
Compare
6bc8da3 to
d134551
Compare
d134551 to
d49bf31
Compare
| if ret != nvml.SUCCESS { | ||
| continue | ||
| } | ||
| vgpuConfig[typeName]++ |
There was a problem hiding this comment.
Question -- does the name reported by NVML, e.g. vgpuTypeId.GetName(), align exactly with the name we were using before? (the names stored in vgpuDev.MDEVType)
There was a problem hiding this comment.
I added a helper function to ensure they exactly align
There was a problem hiding this comment.
So the name reported by NVML contains the product prefix? For example, is NVML returning NVIDIA A100-4C as the type name for the A100-4C device?
There was a problem hiding this comment.
Yes I checked this manually earlier and added the parseVGPUTypeName(rawName string) to verify that we would only be getting A100-4C. The name reported by NVML included a prefix.
internal/vfio/vfio.go
Outdated
| vfnum := 0 | ||
| numVF := int(device.SriovInfo.PhysicalFunction.NumVFs) | ||
| for vfnum < numVF { | ||
| vfAddr := filepath.Join(HostPCIDevicesRoot, device.Address, "virtfn"+strconv.Itoa(vfnum), "nvidia") |
There was a problem hiding this comment.
I don't think the nvidia directory should be included in this path. The "path to the VF" is simply /sys/bus/pci/devices/<BDF>/virtfn<N>. Other parts of the code are not intuitive to me because vfAddr includes the nvidia directory.
There was a problem hiding this comment.
local-agadiyar@ipp1-2284:/sys/bus/pci/devices/0000:41:00.0/virtfn0/nvidia$ cat current_vgpu_type
687
local-agadiyar@ipp1-2284:/sys/bus/pci/devices/0000:41:00.0/virtfn0/nvidia$ cat creatable_vgpu_types
ID : vGPU Name
The current_vgpu_type and creatable_vgpu_types files are located in the nvidia folder. This way we don't need to append nvidia onto another address variable
internal/vfio/vfio.go
Outdated
| return nil, fmt.Errorf("virtual function %d at address %s does not exist", vfnum, vfAddr) | ||
| } | ||
| parentDevices = append(parentDevices, &ParentDevice{ | ||
| NvidiaPCIDevice: device, |
There was a problem hiding this comment.
Question: Shouldn't NvidiaPCIDevice represent the VF (IIUC device is currently a PF)? If so, then I don't see the need to have VirtualFunctionaPath as a separate field. If we instead used the VF here, I think that would simplify the code in a few places and make this easier to read. Note, the nvpci.NvidiaPCIDevice type allows you to go from the VF to the backing PF via device.SriovInfo.VirtualFunction.PhysicalFunction.
There was a problem hiding this comment.
I think the issue here is that nvpci doesn't have a built in way to get virtual functions from the physical function. That is why I am storing the physical device ( via nvdevices, err := m.nvlib.Nvpci.GetGPUs() ) as well as the path to the virtual function.
internal/vfio/vfio.go
Outdated
| } | ||
| devices := []*Device{} | ||
| for _, parentDevice := range parentDevices { | ||
| vgpuTypeNumberBytes, err := os.ReadFile(filepath.Join(parentDevice.VirtualFunctionPath, "current_vgpu_type")) |
There was a problem hiding this comment.
As indicated in a prior comment, if parentDevice was just of type nvpci.NvidiaPCIDevice (and represented the VF), this code would be replaced by:
vgpuTypeNumberBytes, err := os.ReadFile(filepath.Join(parentDevice.NvidiaPCIDevice.Path, "current_vgpu_type"))
There was a problem hiding this comment.
See above comment.
internal/vfio/vfio.go
Outdated
| // ParentDevice represents an NVIDIA parent PCI device. | ||
| type ParentDevice struct { | ||
| *nvpci.NvidiaPCIDevice | ||
| VirtualFunctionPath string |
There was a problem hiding this comment.
Why is this field needed? Shouldn't NvidiaPCIDevice.Path represent the path to the virtual function?
There was a problem hiding this comment.
See above comment. nvpci.NvidiaPCIDevice is storing the physical function
3754bb7 to
cfffa9f
Compare
cfffa9f to
6eaa198
Compare
| } | ||
|
|
||
| type nvlibVGPUConfigManager struct { | ||
| nvlib nvlib.Interface |
There was a problem hiding this comment.
Question -- does nvlib.Interface need to exist anymore?
There was a problem hiding this comment.
No we are no longer using the nvlib.Interface. I don't think we need it for any of the other projects either
| if ret != nvml.SUCCESS { | ||
| continue | ||
| } | ||
| vgpuConfig[typeName]++ |
There was a problem hiding this comment.
So the name reported by NVML contains the product prefix? For example, is NVML returning NVIDIA A100-4C as the type name for the A100-4C device?
pkg/vgpu/config.go
Outdated
| found := false | ||
| for _, vgpuTypeId := range supportedVGPUs { | ||
| rawName, ret := vgpuTypeId.GetName() | ||
| if ret != nvml.SUCCESS { | ||
| continue | ||
| } | ||
| name := parseVGPUTypeName(rawName) | ||
| if name == key { | ||
| found = true | ||
| sanitizedConfig[key] = val | ||
| break | ||
| } | ||
| if name == strippedKey { | ||
| found = true | ||
| sanitizedConfig[strippedKey] = val | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
To improve readability, what if we constructed a map, named supportedVgpuTypes, prior as such:
supportedVgpuTypes := map[string]bool{}
for _, vgpu := range supportedVGPUs {
name, ret := vgpu.GetName()
if ret != nvml.SUCCESS {
continue
}
name = parseVGPUTypeName(name)
supportedVgpuNames[name] = true
}
Then this for loop would simplify to
| found := false | |
| for _, vgpuTypeId := range supportedVGPUs { | |
| rawName, ret := vgpuTypeId.GetName() | |
| if ret != nvml.SUCCESS { | |
| continue | |
| } | |
| name := parseVGPUTypeName(rawName) | |
| if name == key { | |
| found = true | |
| sanitizedConfig[key] = val | |
| break | |
| } | |
| if name == strippedKey { | |
| found = true | |
| sanitizedConfig[strippedKey] = val | |
| break | |
| } | |
| } | |
| if _, ok := supportedVgpuTypes[key]; ok { | |
| sanitizedConfig[key] = val | |
| } else if _, ok := supportedVgpuTypes[strippedKey]; ok { | |
| sanitizedConfig[strippedKey] = val | |
| } else { | |
| return fmt.Errorf("vGPU type %s is not supported on GPU (index=%d, address=%s)", key, gpu, device.Address) | |
| } |
There was a problem hiding this comment.
Ok I broke this into two for loops.
Signed-off-by: Arjun <agadiyar@nvidia.com>
No description provided.