Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 36 additions & 7 deletions src/cpu.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1373,11 +1373,11 @@ static ncnn::CpuSet get_smt_cpu_mask()
if (ptr->Relationship == RelationProcessorCore)
{
ncnn::CpuSet smt_set;
smt_set.mask = ptr->ProcessorMask;
smt_set.mask[0] = ptr->ProcessorMask;
if (smt_set.num_enabled() > 1)
{
// this core is smt
smt_cpu_mask.mask |= smt_set.mask;
smt_cpu_mask.mask[0] |= smt_set.mask[0];
}
}

Expand Down Expand Up @@ -1432,14 +1432,43 @@ static std::vector<int> get_max_freq_mhz()

static int set_sched_affinity(const ncnn::CpuSet& thread_affinity_mask)
{
DWORD_PTR prev_mask = SetThreadAffinityMask(GetCurrentThread(), thread_affinity_mask.mask);
if (prev_mask == 0)
// Find the first enabled CPU group and bind this thread to it.
// A thread can only belong to one processor group at a time.
// For multi-group masks, set_cpu_thread_affinity() calls this
// repeatedly per-thread, each bound to a specific group.
for (int g = 0; g < NCNN_MAX_PROCESSOR_GROUPS; g++)
{
NCNN_LOGE("SetThreadAffinityMask failed %d", GetLastError());
return -1;
if (thread_affinity_mask.mask[g] == 0)
continue;

#if _WIN32_WINNT >= 0x0600 // Win7+

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Gate group affinity on Win7+

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 👍 / 👎.

GROUP_AFFINITY ga;
ga.Group = (WORD)g;
ga.Mask = thread_affinity_mask.mask[g];
ga.Reserved[0] = 0;
ga.Reserved[1] = 0;
ga.Reserved[2] = 0;

GROUP_AFFINITY prev;
if (!SetThreadGroupAffinity(GetCurrentThread(), &ga, &prev))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve Windows XP affinity builds

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 👍 / 👎.

{
NCNN_LOGE("SetThreadGroupAffinity failed for group %d: %d", g, GetLastError());
return -1;
}
#else // XP: only group 0 supported, use legacy API
if (g != 0)
continue; // XP cannot bind beyond group 0
DWORD_PTR prev = SetThreadAffinityMask(GetCurrentThread(), thread_affinity_mask.mask[0]);
if (prev == 0)
{
NCNN_LOGE("SetThreadAffinityMask failed %d", GetLastError());
return -1;
}
#endif
return 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not return after the first processor group

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 👍 / 👎.

}

return 0;
return 0; // no CPUs enabled
}
#endif // defined _WIN32

Expand Down
6 changes: 5 additions & 1 deletion src/cpu.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

#include "platform.h"

// Windows processor groups each contain up to 64 logical processors.
// Support up to NCNN_MAX_PROCESSOR_GROUPS groups (512 CPUs max at 8 groups).
#define NCNN_MAX_PROCESSOR_GROUPS 8

namespace ncnn {

class NCNN_EXPORT CpuSet
Expand All @@ -30,7 +34,7 @@ class NCNN_EXPORT CpuSet

public:
#if defined _WIN32
ULONG_PTR mask;
KAFFINITY mask[NCNN_MAX_PROCESSOR_GROUPS];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fix Windows CpuSet array users

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 👍 / 👎.

#endif
#if defined __ANDROID__ || defined __linux__
cpu_set_t cpu_set;
Expand Down