Skip to content

Commit 958c87c

Browse files
authored
Fix incorrect CPU topology initialization (#3304) (#3487)
The error handling in the global object that defines CPU topology in oneDAL/DAAL and in global daal::services::Environment object was improved. Several related bugs were fixed. - cpp/daal/src/services/service_topo.h and cpp/daal/src/services/service_topo.cpp files re-worked to make it less error prone. - The logic in setChkProcessAffinityConsistency() was changed to allow affinity masks that contain zeros. Previously having the process affinity mask with zeros lead to undefined behavior because the global object that defines CPU topology ended up in non-initialized state in that case. - Out-of-bound memory access issues in CPU topology initialization were fixed. - The constructor of daal::services::Environment class was updated to always call getCpuId() method which initializes the Environment instance. Without this call an uninitialized version of Environment might be used. - The behavior of daal::services::Environment::getInstance() function that returns the pointer to a global DAAL's Environment object was changed. Now the function returns nullptr in case the global Environment object is uninitialized. The respective null pointer checks were added into the calling code. - The behavior of daal::services::Environment::initNumberOfThreads() was changed to handle the case of hyper-threading correctly, setting the maximal number of threads available to oneDAL equal to the number of physical CPU cores.
1 parent 77eab40 commit 958c87c

15 files changed

Lines changed: 1306 additions & 1838 deletions

File tree

cpp/daal/include/algorithms/algorithm_base_common.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,9 @@ class AlgorithmIfaceImpl : public AlgorithmIface
116116
protected:
117117
services::Status getEnvironment()
118118
{
119-
int cpuid = (int)daal::services::Environment::getInstance()->getCpuId();
119+
daal::services::Environment * env = daal::services::Environment::getInstance();
120+
if (!env) return services::Status(services::ErrorCpuNotSupported);
121+
int cpuid = (int)env->getCpuId();
120122
if (cpuid < 0) return services::Status(services::ErrorCpuNotSupported);
121123
_env.cpuid = cpuid;
122124
_env.cpuid_init_flag = true;

cpp/daal/include/services/daal_defines.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,12 @@
4141
#define TARGET_RISCV64
4242
#endif
4343

44+
#if !defined(TARGET_X86_64)
45+
#ifndef DAAL_CPU_TOPO_DISABLED
46+
#define DAAL_CPU_TOPO_DISABLED
47+
#endif
48+
#endif
49+
4450
#if (defined(__INTEL_COMPILER) || defined(__INTEL_LLVM_COMPILER)) && !defined(SYCL_LANGUAGE_VERSION)
4551
#define DAAL_INTEL_CPP_COMPILER
4652
#endif

cpp/daal/include/services/env_detect.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,12 @@ class DAAL_EXPORT Environment : public Base
6565
*/
6666
static Environment * getInstance();
6767

68+
/**
69+
* Returns the status of the environment instance initialization
70+
* \return Status of the environment instance initialization
71+
*/
72+
static int getStatus();
73+
6874
/**
6975
* Decreases the instance counter
7076
* \return The return code

cpp/daal/src/data_management/data_conversion.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,9 @@ static bool tryToCopyFuncAVX512(const size_t nrows, const size_t ncols, void * d
4242

4343
if (!ptr)
4444
{
45-
int cpuid = (int)daal::services::Environment::getInstance()->getCpuId();
45+
daal::services::Environment * env = daal::services::Environment::getInstance();
46+
if (!env) return false; // Environment not initialized
47+
int cpuid = (int)env->getCpuId();
4648

4749
switch (cpuid)
4850
{

cpp/daal/src/externals/service_service_mkl.h

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,19 +62,40 @@ struct MklService
6262
// return memmove_s(dest, destSize, src, smax);
6363
}
6464

65-
static int serv_get_ht() { return (serv_get_ncorespercpu() > 1 ? 1 : 0); }
65+
static int serv_get_ht()
66+
{
67+
const int ncorespercpu = serv_get_ncorespercpu();
68+
if (ncorespercpu < 0)
69+
{
70+
// Failed to get the number of cores per CPU.
71+
return -1;
72+
}
73+
return (ncorespercpu > 1 ? 1 : 0);
74+
}
6675

6776
static int serv_get_ncpus()
6877
{
69-
unsigned int ncores = daal::services::internal::_internal_daal_GetProcessorCoreCount();
78+
if (daal::services::internal::_internal_daal_GetStatus() != 0)
79+
{
80+
// CPU topology initialization failed;
81+
return -1;
82+
}
83+
const unsigned int ncores = daal::services::internal::_internal_daal_GetProcessorCoreCount();
7084
return (ncores ? ncores : 1);
7185
}
7286

7387
static int serv_get_ncorespercpu()
7488
{
75-
unsigned int nlogicalcpu = daal::services::internal::_internal_daal_GetProcessorCoreCount();
76-
unsigned int ncpus = serv_get_ncpus();
77-
return (ncpus > 0 && nlogicalcpu > 0 && nlogicalcpu > ncpus ? nlogicalcpu / ncpus : 1);
89+
if (daal::services::internal::_internal_daal_GetStatus() != 0)
90+
{
91+
// CPU topology initialization failed;
92+
return -1;
93+
}
94+
const unsigned int nlogicalcpu = daal::services::internal::_internal_daal_GetSysLogicalProcessorCount();
95+
const unsigned int ncpus = serv_get_ncpus();
96+
// On hybrid systems, return 2 in case of hyper-threading enabled at least on some cores,
97+
// 1 otherwise
98+
return (ncpus > 0 && nlogicalcpu > 0 && nlogicalcpu > ncpus ? (nlogicalcpu - ncpus + 1) / ncpus : 1);
7899
}
79100

80101
// TODO: The real call should be delegated to a backend library if the option is supported

cpp/daal/src/services/compiler/generic/env_detect_features.cpp

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,11 @@
2727

2828
#if defined(TARGET_X86_64)
2929
#include <immintrin.h>
30+
31+
#if defined(__GNUC__) || defined(__clang__)
32+
#include <cpuid.h> // __cpuidex
33+
#endif
34+
3035
#elif defined(TARGET_ARM)
3136
#include <sys/auxv.h>
3237
#include <asm/hwcap.h>
@@ -54,10 +59,11 @@ void __daal_serv_CPUHasAVX512f_enable_it_mac();
5459
#if defined(TARGET_X86_64)
5560
void run_cpuid(uint32_t eax, uint32_t ecx, uint32_t * abcd)
5661
{
57-
#if defined(_MSC_VER)
58-
__cpuidex((int *)abcd, eax, ecx);
62+
#if defined(_MSC_VER) || (defined(__clang__) && __clang_major__ > 18) || (defined(__GNUC__) && !defined(__clang__))
63+
__cpuidex(reinterpret_cast<int *>(abcd), eax, ecx);
5964
#else
60-
uint32_t ebx, edx;
65+
66+
uint32_t ebx = 0, edx = 0;
6167
#if defined(__i386__) && defined(__PIC__)
6268
/* in case of PIC under 32-bit EBX cannot be clobbered */
6369
__asm__("movl %%ebx, %%edi \n\t cpuid \n\t xchgl %%ebx, %%edi" : "=D"(ebx), "+a"(eax), "+c"(ecx), "=d"(edx));

cpp/daal/src/services/env_detect.cpp

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,15 @@ void daal_free_buffers();
5454
DAAL_EXPORT daal::services::Environment * daal::services::Environment::getInstance()
5555
{
5656
static daal::services::Environment instance;
57-
return &instance;
57+
if (isInit)
58+
return &instance;
59+
else
60+
return nullptr;
61+
}
62+
63+
DAAL_EXPORT int daal::services::Environment::getStatus()
64+
{
65+
return daal::services::internal::_internal_daal_GetStatus();
5866
}
5967

6068
DAAL_EXPORT int daal::services::Environment::freeInstance()
@@ -129,6 +137,8 @@ DAAL_EXPORT daal::services::Environment::Environment() : _schedulerHandle {}, _g
129137
{
130138
_env.cpuid_init_flag = false;
131139
_env.cpuid = -1;
140+
// Initialize the Environment to prevent from using it uninitialized
141+
getCpuId();
132142
}
133143

134144
DAAL_EXPORT daal::services::Environment::Environment(const Environment & e) : daal::services::Environment::Environment() {}
@@ -145,15 +155,25 @@ DAAL_EXPORT void daal::services::Environment::initNumberOfThreads()
145155
daal::setSchedulerHandle(&_schedulerHandle);
146156
#endif
147157
/* if HT enabled - set _numThreads to physical cores num */
148-
if (daal::internal::ServiceInst::serv_get_ht())
158+
const int htStatus = daal::internal::ServiceInst::serv_get_ht();
159+
if (htStatus < 0)
149160
{
150-
/* Number of cores = number of cpu packages * number of cores per cpu package */
151-
int ncores = daal::internal::ServiceInst::serv_get_ncpus() * daal::internal::ServiceInst::serv_get_ncorespercpu();
161+
// Failed to get the number of CPUs. Environment cannot be initialized
162+
return;
163+
}
164+
else if (htStatus > 0)
165+
{
166+
const int ncpus = daal::internal::ServiceInst::serv_get_ncpus();
167+
if (ncpus < 0)
168+
{
169+
// Failed to get the number of CPUs. Environment cannot be initialized
170+
return;
171+
}
152172

153173
/* Re-set number of threads if ncores is valid and different to _numThreads */
154-
if ((ncores > 0) && (ncores < _daal_threader_get_max_threads()))
174+
if ((ncpus > 0) && (ncpus < _daal_threader_get_max_threads()))
155175
{
156-
daal::services::Environment::setNumberOfThreads(ncores);
176+
daal::services::Environment::setNumberOfThreads(ncpus);
157177
}
158178
}
159179
isInit = true;

cpp/daal/src/services/library_version_info.cpp

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,12 +53,25 @@ DAAL_EXPORT daal::services::LibraryVersionInfo::LibraryVersionInfo()
5353
productStatus(PRODUCTSTATUS),
5454
build(BUILD),
5555
build_rev(BUILD_REV),
56-
name(PRODUCT_NAME_STR),
56+
name(PRODUCT_NAME_STR)
57+
{
5758
#ifndef DAAL_REF
58-
processor(cpu_long_names[daal::services::Environment::getInstance()->getCpuId()])
59+
daal::services::Environment * env = daal::services::Environment::getInstance();
60+
if (env)
61+
{
62+
processor = cpu_long_names[env->getCpuId()];
63+
}
64+
else
65+
{
66+
#if (!defined(DAAL_NOTHROW_EXCEPTIONS))
67+
int error = daal::services::Environment::getStatus();
68+
throw std::runtime_error("Environment not initialized, cannot get processor info, error code: " + std::to_string(error));
69+
#else
70+
processor = cpu_long_names[0];
71+
#endif
72+
}
5973
#else
60-
processor(cpu_long_names[0])
74+
processor = cpu_long_names[0];
6175
#endif
62-
{}
63-
76+
}
6477
DAAL_EXPORT daal::services::LibraryVersionInfo::~LibraryVersionInfo() {}

0 commit comments

Comments
 (0)