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

Fix CPU_SET dynamic allocation and leak #205

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

prashanthswami
Copy link
Collaborator

The initial implementation had a number of issues:

  • The allocation of the CPU_SET should be checked for a NULL return.
  • The CPU_*_S macros should be used when working with dynamic sets.
  • The CPU_SET needs to be cleared via CPU_ZERO_S before use.
  • Dynamic CPU_SETs need to be freed after use.
  • The __riscv_hwprobe syscall is expecting a set size not a count.

Reviewer: @malfet
CC: @GlassOfWhiskey @markdryan

The initial implementation had a number of issues:
- The allocation of the CPU_SET should be checked for a NULL return.
- The CPU_*_S macros should be used when working with dynamic sets.
- The CPU_SET needs to be cleared via CPU_ZERO_S before use.
- Dynamic CPU_SETs need to be freed after use.
- The __riscv_hwprobe syscall is expecting a set *size* not a *count*.
@markdryan
Copy link
Contributor

@prashanthswami I tried building cpuinfo today an a RISC-V VM (Ubuntu 23.10 with glibc 2.38) and the build failed with the following error

cpuinfo/src/riscv/linux/riscv-hw.c:1:10: fatal error: sys/hwprobe.h: No such file or directory
    1 | #include <sys/hwprobe.h>
      |          ^~~~~~~~~~~~~~~
compilation terminated.

This was the main branch without this PR, so it looks like the RISC-V builds are currently broken (they don't seem to be tested by the CI). It looks to me like you're relying on glibc's implementation of hwprobe, but AFAIK this isn't merged yet. How did you get it to build? Did you build your own glibc with the hwprobe patch applied? In any case, we can't really assume that this header will always be available, not for a good few years at least.

Other projects that are using hwprobe right now are making the syscall directly, which is probably the safest thing to do for now. OpenJDK duplicates the constants from the kernel headers needed for the sys call and then does a direct syscall. A nicer solution would be to first check to see if the <asm/hwprobe.h> exists either using a build system check or perhaps using

#ifdef __has_include
# if __has_include (<asm/hwprobe.h>)
#  include <asm/hwprobe.h>
# endif
#endif

and manually defining the constants if asm/hwprobe is not available before making the syscall.

@GlassOfWhiskey
Copy link
Contributor

What I was trying to do in #148 is to check the kernel version at compile time.
If you prefer, I can do a different check, or move it directly at runtime. Please let me know what you prefer, but please do not split again the discussion over multiple PRs otherwise it becomes impossible to follow the flow.

@markdryan
Copy link
Contributor

markdryan commented Nov 24, 2023

What I was trying to do in #148 is to check the kernel version at compile time. If you prefer, I can do a different check, or move it directly at runtime. Please let me know what you prefer, but please do not split again the discussion over multiple PRs otherwise it becomes impossible to follow the flow.

Where would you like to have the discussions? Over at #148 or perhaps #124?

I've answered my own question. Looks like #148 is the place to be. I'll comment there.

@prashanthswami
Copy link
Collaborator Author

+1 Let's keep discussions about swapping hwprobe includes over in #148 - this PR is purely to resolve the CPU_SET usage error.

@malfet malfet merged commit 9d80992 into pytorch:main Nov 28, 2023
8 checks passed
@prashanthswami prashanthswami deleted the fix-riscv-hwprobe branch January 11, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants