Conversation
FreeMasen
force-pushed
the
feat/some-vgpu-apis
branch
4 times, most recently
from
2b5367f to
f3f0229
Compare
FreeMasen
force-pushed
the
feat/some-vgpu-apis
branch
from
f3f0229 to
970027b
Compare
FreeMasen
left a comment
FreeMasen
left a comment
There was a problem hiding this comment.
I believe this is ready for an initial review to make sure I am not doing anything that goes against the goals of this project. I need to dig a little deeper into how testing works for this project to add additional test for these additions.
I am going to leave the Draft status until I can get through tests, any feedback would be welcome!
| #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] | ||
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
| #[repr(u32)] | ||
| pub enum VgpuLicenseState { |
There was a problem hiding this comment.
This is a bit of an odd scenario since it is effectively a C enum in NVML but no c enum is actually defined for it. Let me know if you'd prefer this live in src/vgpu.rs instead of here
| use wrapcenum_derive::EnumWrapper; | ||
|
|
||
| #[derive(Debug, Clone, Eq, PartialEq, Hash)] | ||
| pub enum VmId { |
There was a problem hiding this comment.
This may be a misinterpretation of the *_wrappers modules, let me know if you'd prefer this is moved to a different location
| Licensed = NVML_GRID_LICENSE_STATE_LICENSED, | ||
| } | ||
|
|
||
| impl From<u32> for VgpuLicenseState { |
There was a problem hiding this comment.
Since one of the cases was NVML_GRID_LICENSE_STATE_UNKNOWN, it seemed like we wouldn't need TryFrom here but let me know if you'd prefer to use the UnexpectedVariant error instead
| pub hour: u8, | ||
| pub min: u8, | ||
| pub sec: u8, | ||
| pub status: u8, |
There was a problem hiding this comment.
I couldn't find any documentation about why are expected values here and left the type effectively unchanged. Let me know if there is something I missed and I can add another enum to capture the status.
| year: u16::try_from(value.year).unwrap_or(u16::MAX), | ||
| month: u8::try_from(value.month).unwrap_or(u8::MAX), | ||
| day: u8::try_from(value.day).unwrap_or(u8::MAX), | ||
| hour: u8::try_from(value.hour).unwrap_or(u8::MAX), | ||
| min: u8::try_from(value.min).unwrap_or(u8::MAX), | ||
| sec: u8::try_from(value.sec).unwrap_or(u8::MAX), |
There was a problem hiding this comment.
These all seem like there would be little risk to fail and the TryFromIntError these return wasn't already included in the error variants, let me know if you'd prefer another variant is added there instead of using these defaults
| Ok(device | ||
| .active_vgpus()? | ||
| .into_iter() | ||
| .map(|v| v.instance) | ||
| .collect::<Vec<_>>()) | ||
| }) |
There was a problem hiding this comment.
Using a lifetime generic argument in the return value here made these test helpers fail due to the lifetime requirements, if there is a better way to handle this please let me know
| impl TryFrom<nvmlVgpuMetadata_t> for VgpuMetadata { | ||
| type Error = NvmlError; | ||
| fn try_from(value: nvmlVgpuMetadata_t) -> Result<Self, Self::Error> { | ||
| let convert_c_str = |c_str: &[c_char]| { |
There was a problem hiding this comment.
This one is a bit odd, I wanted to avoid using any unsafe and so this will manually copy the string byte by byte which may not be the most efficient. Let me know if you'd prefer a different method for this or even if lossy conversion is acceptable here
|
I'm happy to review this once it is ready. Once you're happy with it lmk. |
FreeMasen
force-pushed
the
feat/some-vgpu-apis
branch
from
afce533 to
c6b1acb
Compare
| let mut count = 0; | ||
| unsafe { | ||
| nvml_try_count(sym(self.instance, std::ptr::null_mut(), &mut count))?; | ||
| metadata = vec![std::mem::zeroed(); count as usize]; |
There was a problem hiding this comment.
I am not sure about this one, I tried running this function on a machine with an a16 and count was a number that was much larger than the expected buffer size (468) but not a factor of std::mem::size_of::<nvmlVgpuMetadata_t>(); which is 212 according to rust analyzer.
|
I believe this is ready for review. I have tried to comment on any of the places where I wasn't quite sure about how something should be implemented. |
2 participants
This PR is an attempt to fully cover the vGPU APIs defined by nvml. To achieve this I've followed the patterns defined for
DeviceandVgpuTypeto cover all functions in theunwrapped_functions.txtfile that are namednvmlVgpu*The one exception here was
nvmlVgpuInstanceGetLicenseStatuswhich is deprecated and not documented at this time, if you'd like me to dig through older versions of the docs, I can find the documentation for how that API works.Please let me know if I've misunderstood any of the patterns I've tried to emulate and/or the API documentation, I would be happy to follow up with additional changes as needed.
Note: this is currently marked as a draft because I based these changes on #129 and will rebase once that merges or is declined