Extend cpu affinity mask to support up to 64 cores#286
Open
shegazyy wants to merge 3 commits into
Open
Conversation
shegazyy
temporarily deployed
to
workflow-approval
GitHub Actions
Inactive
shegazyy
temporarily deployed
to
workflow-approval
GitHub Actions
Inactive
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
Contributor
Author
|
Hey @NicolasFussberger 🙏 ready for review whenever you have a moment! |
Contributor
Thanks! I requested @SimonKozik to do a review, he already had a closer look at this some time in the past. |
|
The created documentation from the pull request is available at: docu-html |
shegazyy
temporarily deployed
to
workflow-approval
GitHub Actions
Inactive
shegazyy
temporarily deployed
to
workflow-approval
GitHub Actions
Inactive
Contributor
|
The qnx8 pipeline fails with I also could not find a reference to _NTO_TCTL_RUNMASK64_GET_AND_SET. But I think also the runmask support in qnx8 changed significantly with the introduction of cpu clusters. |
shegazyy
requested a deployment
to
workflow-approval
GitHub Actions
Waiting
shegazyy
requested a deployment
to
workflow-approval
GitHub Actions
Waiting
Contributor
Author
|
Thanks for catching this @NicolasFussberger 🙏 turns out |
The cpu_mask_ field in OsalConfig and the setaffinity() function used uint32_t, limiting process affinity to at most 32 cores. Systems with more than 32 cores would silently use only the lower 32 bits. Change the mask type to uint64_t throughout the affected chain: - OsalConfig::cpu_mask_ (iprocess.hpp) - setaffinity() signature (set_affinity.hpp and both platform impls) - kDefaultProcessorAffinityMask() return type (configuration_manager) - kDefaultNumCores cap raised from 32 to 64 (num_cores.hpp) On Linux the loop variable and shift literal are updated to uint64_t / 1ULL to avoid the undefined behaviour for i >= 32 noted in the original review discussion (PR eclipse-score#113). On QNX the 32-bit ThreadCtl(_NTO_TCTL_RUNMASK_GET_AND_SET) is replaced with _NTO_TCTL_RUNMASK64_GET_AND_SET which accepts a uint64_t mask. kDefaultProcessorAffinityMask() guards against the n == 64 edge case (where 1ULL << 64 would itself be UB) by returning UINT64_MAX instead. The XML configuration reader is updated to use std::stoull so the full 64-bit coremask value is parsed without truncation. Closes eclipse-score#122
_NTO_TCTL_RUNMASK_GET_AND_SET returns EINVAL on systems with more than 32 CPUs. Use _NTO_TCTL_RUNMASK_GET_AND_SET_INHERIT instead, which accepts a struct _thread_runmask with RMSK_SIZE/RMSK_SET macros to support up to 64 CPUs across multiple CPU clusters. Issue: eclipse-score#122
shegazyy
force-pushed
the
fix/extend_runmask_to_64_cores
branch
from
3966871 to
195e81f
Compare
shegazyy
temporarily deployed
to
workflow-approval
GitHub Actions
Inactive
shegazyy
temporarily deployed
to
workflow-approval
GitHub Actions
Inactive
Contributor
Author
|
Hey @NicolasFussberger 🙏 reworked it to use |
shegazyy
requested a deployment
to
workflow-approval
GitHub Actions
Waiting
shegazyy
requested a deployment
to
workflow-approval
GitHub Actions
Waiting
| { | ||
| if (cpumask & (1ULL << i)) | ||
| { | ||
| RMSK_SET(static_cast<int>(i), tm.runmask); |
Contributor
There was a problem hiding this comment.
I tried this out and the code was working for > 32 cpus 👍
I wonder if we should also set the inheritmask here, otherwise only the main thread of the child process gets the affinity. Any further threads created by the child process will not get this affinity.
Suggested change
| RMSK_SET(static_cast<int>(i), tm.runmask); | |
| RMSK_SET(static_cast<int>(i), tm.runmask); | |
| RMSK_SET(static_cast<int>(i), tm.inherit_mask); |
This is what I assume is the wanted behaviour from an integrators point or view and I think this is also the behavior of the linux implementation of this method
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The cpu_mask_ field in OsalConfig and the setaffinity() function used uint32_t, limiting process affinity to at most 32 cores. Systems with more than 32 cores would silently use only the lower 32 bits.
Change the mask type to uint64_t throughout the affected chain:
On Linux the loop variable and shift literal are updated to uint64_t / 1ULL to avoid the undefined behaviour for i >= 32 noted in the original review discussion (PR #113).
On QNX the 32-bit ThreadCtl(_NTO_TCTL_RUNMASK_GET_AND_SET) is replaced with _NTO_TCTL_RUNMASK64_GET_AND_SET which accepts a uint64_t mask.
kDefaultProcessorAffinityMask() guards against the n == 64 edge case (where 1ULL << 64 would itself be UB) by returning UINT64_MAX instead.
The XML configuration reader is updated to use std::stoull so the full 64-bit coremask value is parsed without truncation.
Closes #122