Skip to content
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

feature request: add support for nvmlDeviceGetGraphicsRunningProcesses_v2 #33

Closed
KisaragiEffective opened this issue Aug 17, 2022 · 5 comments · Fixed by #39
Closed
Labels
backwards-compatibility Related to compatibility with older versions of NVML

Comments

@KisaragiEffective
Copy link
Contributor

KisaragiEffective commented Aug 17, 2022

Hello! I'm one of the users, and found out that this crate does not support nvmlDeviceGetGraphicsRunningProcesses_v2. My personal computer runs on debian 11 (and there's managed package called nvidia-driver), which is v470 and lacks support of nvmlDeviceGetGraphicsRunningProcesses_v3:

$ nm -D /usr/lib/x86_64-linux-gnu/libnvidia-ml.so.1 | grep nvmlDeviceGetGraphicsRunningProcesses
0000000000051370 T nvmlDeviceGetGraphicsRunningProcesses
0000000000051570 T nvmlDeviceGetGraphicsRunningProcesses_v2

So, I purpose add two method: running_graphics_processes_v2 and running_graphics_processes_count_v2. They'll have same return type as running_graphics_processes and running_graphics_processes_count as well.

Here's my current code (you can use this: I wrote this based on your code, so I'll leave this as MIT OR Apache-2.0 license)

Would you mind if I submit PR for this? Thank you.

Code

Code
trait GetRunningGraphicsProcessesV2 {
    fn running_graphics_processes_v2(&self) -> Result<Vec<ProcessInfo>, NvmlError>;

    fn running_graphics_processes_count_v2(&self) -> Result<u32, NvmlError>;
}

type GetProcessV2Sig = unsafe extern "C" fn(
    device: nvmlDevice_t,
    #[allow(non_snake_case)]
    infoCount: *mut c_uint,
    infos: *mut nvmlProcessInfo_t,
) -> nvmlReturn_t;

static GET_PS_V2: Lazy<GetProcessV2Sig> = Lazy::new(|| {
    unsafe {
        let lib = Library::new("libnvidia-ml.so").unwrap();
        let f: GetProcessV2Sig = lib.get(b"nvmlDeviceGetGraphicsRunningProcesses_v2\0").map(|a| *a).unwrap();
        f
    }
});

impl GetRunningGraphicsProcessesV2 for Device<'_> {
    fn running_graphics_processes_v2(&self) -> Result<Vec<ProcessInfo>, NvmlError> {
        let sym = *GET_PS_V2;

        let mut count: c_uint = match self.running_graphics_processes_count_v2()? {
            0 => return Ok(vec![]),
            value => value,
        };
        // Add a bit of headroom in case more processes are launched in
        // between the above call to get the expected count and the time we
        // actually make the call to get data below.
        count += 5;
        let mem = unsafe { mem::zeroed() };
        let mut processes: Vec<nvmlProcessInfo_t> = vec![mem; count as usize];

        let device = unsafe { self.handle() };
        nvml_try(sym(device, &mut count, processes.as_mut_ptr()))?;
        processes.truncate(count as usize);

        Ok(processes.into_iter().map(ProcessInfo::from).collect())
    }

    fn running_graphics_processes_count_v2(&self) -> Result<u32, NvmlError> {
        let sym = *GET_PS_V2;


        // Indicates that we want the count
        let mut count: c_uint = 0;

        let device = unsafe { self.handle() };
        // Passing null doesn't indicate that we want the count. It's just allowed.
        match sym(device, &mut count, null_mut()) {
            nvmlReturn_enum_NVML_ERROR_INSUFFICIENT_SIZE => Ok(count),
            // If success, return 0; otherwise, return error
            other => nvml_try(other).map(|_| 0),
        }
    }
}
@Cldfire Cldfire added the backwards-compatibility Related to compatibility with older versions of NVML label Aug 19, 2022
@Cldfire
Copy link
Collaborator

Cldfire commented Aug 19, 2022

Hey! I have seen this and I will give it some thought, but I won't have time to reply for another couple of days 🙂.

@KisaragiEffective
Copy link
Contributor Author

Thank you for your reply! Please feel free to tell me it when you're free.

@KisaragiEffective
Copy link
Contributor Author

@Cldfire How is it going? May I submit one?

@Cldfire
Copy link
Collaborator

Cldfire commented Nov 26, 2022

Sorry for taking so long to reply! I got busy with life stuff.

So, I've put some thought into this and I have two main concerns here:

  • In order for me to accept a PR for this we would need to end up with nvmlDeviceGetGraphicsRunningProcesses_v2 being generated and present as a symbol in nvml-wrapper-sys/src/bindings.rs. I don't want to mess around with opening and loading a second library definition to support legacy function calls. Doing this will require making sure bindgen generates bindings for the functions in this ifdef, and making sure that's accomplished without modifying nvml.h in the source tree:

    https://github.com/Cldfire/nvml-wrapper/blob/1e943a42d2362f6aa1ad84d41f6e54af2fa61187/nvml-wrapper-sys/nvml.h#L9106-L9129

  • I don't want to clog editor autocomplete with the names of legacy methods. This means avoiding a situation where the default autocomplete experience when using this library is seeing the following:

    device.running_graphics_<|>
    
        ------------------------------------------
        device.running_graphics_processes()
        device.running_graphics_processes_v1()
        device.running_graphics_processes_v2()
        ------------------------------------------
    

    This excludes a trait-based approach for bringing these into scope because rust-analyzer is smart enough to discover method calls that could be made available by importing a trait and suggest those for autocomplete. The only way to preserve a good default autocomplete experience here would be to have a crate feature such as "legacy-functions" that could be enabled to compile support for older function calls in the wrapper.

@Cldfire
Copy link
Collaborator

Cldfire commented Nov 26, 2022

@KisaragiEffective I put together a branch meeting both of the constraints I laid out above. It's in a PR here: #39

Please give that a look and let me know if it would satisfy your needs 🙂.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-compatibility Related to compatibility with older versions of NVML
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants