pytorch / cpuinfo

CPU INFOrmation library (x86/x86-64/ARM/ARM64, Linux/Windows/Android/macOS/iOS)
BSD 2-Clause "Simplified" License
996 stars 314 forks source link

Fix CPU_SET dynamic allocation and leak #205

Closed prashanthswami closed 10 months ago

prashanthswami commented 10 months ago

The initial implementation had a number of issues:

Reviewer: @malfet CC: @GlassOfWhiskey @markdryan

markdryan commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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