fix: support >64 CPUs on Windows via processor groups (#6142)#6823
fix: support >64 CPUs on Windows via processor groups (#6142)#6823xyaohubery wants to merge 2 commits into
Conversation
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0e107e155
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| #if __APPLE__ | ||
| unsigned int policy; | ||
| #endif | ||
| }; |
There was a problem hiding this comment.
The header now ends immediately after CpuSet, removing the namespace close, include-guard #endif, and all exported CPU API declarations such as get_cpu_count() and set_cpu_thread_affinity(). Any translation unit that includes src/cpu.h will fail before reaching this Windows processor-group code, and external users also lose the public declarations.
Useful? React with 👍 / 👎.
| NCNN_LOGE("SetThreadGroupAffinity failed for group %d: %d", g, GetLastError()); | ||
| return -1; | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
When the affinity mask spans multiple Windows processor groups, this early return binds every caller to only the first non-empty group. set_cpu_thread_affinity() still passes the same full CpuSet to each OpenMP worker, so on >64-CPU machines all workers land in group 0 and CPUs in later groups are never used, defeating the intended processor-group support.
Useful? React with 👍 / 👎.
c0e107e to
4e3dc5a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e3dc5a4c8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| public: | ||
| #if defined _WIN32 | ||
| ULONG_PTR mask; | ||
| KAFFINITY mask[NCNN_MAX_PROCESSOR_GROUPS]; |
There was a problem hiding this comment.
In the current tree, changing CpuSet::mask to an array leaves the Windows-only code still treating it as a scalar: get_smt_cpu_mask() assigns/ORs smt_set.mask, and CpuSet::enable/disable/disable_all/is_enabled() still use operations like mask |= and mask = 0. Any _WIN32 build that compiles cpu.cpp therefore fails before the processor-group support can be used; the helper methods and SMT mask construction need to index/iterate the group array consistently.
Useful? React with 👍 / 👎.
| ga.Reserved[2] = 0; | ||
|
|
||
| GROUP_AFFINITY prev; | ||
| if (!SetThreadGroupAffinity(GetCurrentThread(), &ga, &prev)) |
There was a problem hiding this comment.
When building the repository's Windows XP configurations (.github/workflows/windows-xp.yml uses -DNCNN_WINXP=ON and the XP toolchains set _WIN32_WINNT=0x0501), this unguarded call is not available: Microsoft documents SetThreadGroupAffinity as requiring _WIN32_WINNT >= 0x0601 / Windows 7 (https://learn.microsoft.com/en-us/windows/win32/api/processtopologyapi/nf-processtopologyapi-setthreadgroupaffinity). Because the whole _WIN32 affinity path now depends on it, those supported XP-targeted builds fail instead of falling back to SetThreadAffinityMask.
Useful? React with 👍 / 👎.
Replace single ULONG_PTR mask with KAFFINITY array (NCNN_MAX_PROCESSOR_GROUPS=8) supporting up to 512 logical processors. - CpuSet enable/disable/is_enabled: compute group and bit from cpu index - CpuSet num_enabled: count POPCNT across all groups - set_sched_affinity: bind thread to first enabled processor group - Preserve all existing declarations (no header truncation) - Backward compatible for single-group machines Fixes Tencent#6142 Assisted-by: claude-code:deepseek-v4-pro
4e3dc5a to
14a70a9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14a70a92bf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (thread_affinity_mask.mask[g] == 0) | ||
| continue; | ||
|
|
||
| #if _WIN32_WINNT >= 0x0600 // Win7+ |
There was a problem hiding this comment.
Fresh evidence beyond the prior XP concern: this revision added a guard, but it still takes this branch when _WIN32_WINNT == 0x0600 (Vista), while SetThreadGroupAffinity is only available for _WIN32_WINNT >= 0x0601 / Windows 7. A Vista-targeted non-XP build therefore compiles or links against a missing API instead of using the legacy SetThreadAffinityMask fallback; use the Win7 threshold or a runtime lookup here.
Useful? React with 👍 / 👎.
2 participants
Summary
Replace single
ULONG_PTR maskwithKAFFINITYarray supporting up to 512 logical processors (8 groups × 64 CPUs).Changes
ULONG_PTR mask→KAFFINITY mask[NCNN_MAX_PROCESSOR_GROUPS]SetThreadGroupAffinityfor proper group bindingVerification
Fixes #6142
Assisted-by: claude-code:deepseek-v4-pro