Conversation
GNU/Hurd has the cpuset types and macros, but no pthread_getaffinity_np yet.
|
I maintain the Debian packages for jemalloc, and @sthibaul is also with the Debian project, and one of the GNU Hurd porters. Samuel also had to submit a couple of changes (1, 2) to glibc for jemalloc to work on Hurd; he's definitely the subject-matter expert here! I've included the changes in this PR in our jemalloc (5.3.0) Debian package, and it has built successfully (build log) on Hurd, and is passing the test suite with no warnings. I doubt Hurd is a priority for jemalloc, but given this works and the changes are trivial and not invasive at all, nor hurt any other ports, I was hoping you'd just merge it, so that we don't have to carry any downstream patches. Thanks! |
| #if defined(JEMALLOC_HAVE_SCHED_SETAFFINITY) | ||
| cpu_set_t cpuset; | ||
| #else | ||
| #elif defined(JEMALLOC_HAVE_PTHREAD_GETAFFINITY_NP) |
There was a problem hiding this comment.
Question: does Hurd have pthread_getaffinity_np but not pthread_setaffinity_np? Asking because L116 have the defined(JEMALLOC_HAVE_PTHREAD_SETAFFINITY_NP) check.
This section of the code is assuming background thread is supported, so we should not have unsupported cases workaround here. See L86 around #ifndef JEMALLOC_BACKGROUND_THREAD. If it's not possible to support bg thds on hurd, let's make it explicitly so. Maybe through:
Line 2575 in 72cfdce
There was a problem hiding this comment.
does Hurd have pthread_getaffinity_np but not pthread_setaffinity_np?
No, it has none of the two (and not sched_setaffinity either)
This section of the code is assuming background thread is supported
Mmm, ok, but is thread binding really necessary for background thread support?
| # elif defined(JEMALLOC_HAVE_PTHREAD_GETAFFINITY_NP) | ||
| pthread_getaffinity_np(pthread_self(), sizeof(set), &set); | ||
| # else | ||
| CPU_ZERO(&set); |
There was a problem hiding this comment.
This will cause jemalloc to detect wrong number of CPUs, and likely will not perform well, even defeating the purpose of using jemalloc. Is there no way to do so on hurd?
There was a problem hiding this comment.
SMP support is still WIP, so it's actually the correct number of CPUs :)
That said, sysconf(_SC_NPROCESSORS_ONLN) will properly report 1, so perhaps it's the #elif defined(CPU_COUNT) above that should rather be #elif defined(CPU_COUNT) && (defined(JEMALLOC_HAVE_SCHED_SETAFFINITY) || defined(JEMALLOC_HAVE_PTHREAD_GETAFFINITY_NP)
|
Hi! Any news here? |
No description provided.