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

Add Windows support #1

Open
klueska opened this issue Dec 4, 2020 · 5 comments
Open

Add Windows support #1

klueska opened this issue Dec 4, 2020 · 5 comments

Comments

@klueska
Copy link
Contributor

klueska commented Dec 4, 2020

The "old" version of the NVML bindings (which were a subfolder of gpu-monitoring-tools and not really "bindings" in the true sense of the word) were usable on both Linux and Windows.

Currently these new, comprehensive bindings are only usable on Linux.
We should add windows support as well.

I envision changes in 3 places to support it:

  1. Change the pgk/dl abstraction to be more generic and usable on windows
  2. Update gen/nvml/init.go to consume the updated API from pkg/dl so that it works on both linux and windows
  3. Update nvml.yml so that all generated code is usable on both linux and windows (currently we add linux specific cgo directives to the generated code).

/cc @Drauthius who added windows support to:
https://github.com/NVIDIA/gpu-monitoring-tools/tree/master/bindings/go/nvml

@rubu
Copy link

rubu commented Feb 15, 2021

Hi @klueska,

since gpu-monitoring-tools cannot be used as a standlone bindings module (do to the fact it pulls in kubernetes, issue #123), I had a look a this repo. I need it for win exclusively so I wanted to patch it, went through the points you listed here, and well the bindings part is a bit tricky. I don't think that there exists functionality on Windows that mimics what RTLD_GLOBAL does, and gpu-monitoring-tools avoids RTLD_GLOBAL as well. Is a solution that simply loads the symbols in variables, in contrast to the clever but unportable exposure via the same name be considered as well?

@rubu
Copy link

rubu commented Feb 15, 2021

Also a small weird thing I noticed:

func SystemGetProcessName(Pid int) (string, Return) {
	Name := make([]byte, C.PR_SET_NAME)
	ret := nvmlSystemGetProcessName(uint32(Pid), &Name[0], C.PR_SET_NAME)
	return string(Name[:clen(Name)]), ret
}

PR_SET_NAME is treated as the maximum length of the name, even though it is a flag. Shouldn't 16 be used here according to:

PR_SET_NAME (since Linux 2.6.9)

Set the name of the calling thread, using the value in the location pointed to by (char *) arg2. The name can be up to 16 bytes long, and should be null-terminated if it contains fewer bytes.

Since the actual value of the flag seems to be 15:

#define PR_SET_NAME    15		/* Set process name */
#define PR_GET_NAME    16		/* Get process name */

@klueska
Copy link
Contributor Author

klueska commented Feb 15, 2021

Hi @rubu

I will take a look more closely at your first comment later this week.
For your second comment -- yes, you are correct. That was an oversight.

Since this value is just used to set the maximum length of the buffer we pass in, we are not in danger of overunning any memory with the current implementation -- it's just a little weird to be using the arbitrary value of PR_SET_NAME as the length.

I am likely to just create my own constant called PROC_NAME_MAX and set it to 256. This will then work on both linux and windows once the time comes.

@klueska
Copy link
Contributor Author

klueska commented Feb 16, 2021

Regarding your second comment: #10

@clarkmcc
Copy link

Curious if there's an update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants