[Openmp-commits] [PATCH] D72058: [OpenMP] Enabling CPU affinity on Darwin platform proposal

[Andrey Churbanov via Phabricator via Openmp-commits]

AndreyChurbanov added inline comments.


================
Comment at: openmp/runtime/src/kmp_affinity.h:313
+#elif KMP_OS_DARWIN
+      boolean_t def = true;
+      mach_msg_type_number_t cnt = __kmp_affin_mask_size;
----------------
IINM, if the def is true, then thread_policy_get will always return default policy, which is 0.  Is it the intention here?


================
Comment at: openmp/runtime/src/kmp_affinity.h:321
+        for (size_t i = 0; i < __kmp_affin_mask_size; i++)
+          set(tmask[i].affinity_tag);
+      }
----------------
The affinity_tag is treated as kind of bit mask here.  But actually the affinity_tag is just an ID which hints the system to keep closer threads with same tags, and keep spread threads with different tags.
Of cause it can be used as kind of mask anyway, if set appropriately in set_system_affinity.

And I am not sure the thread_policy_get can work with array of policies.  From what I saw in public examples, it works with single policy, and cnt parameter is just the number of integers in the corresponding structure, not the size of array. 



================
Comment at: openmp/runtime/src/kmp_affinity.h:352
+
+      kern_return_t r = thread_policy_set(
+          mach_thread_self(), THREAD_AFFINITY_POLICY,
----------------
Same as for thread_policy_get, I think the thread_policy_set works with single policy, not with array of them. Thus the third parameter should be thread_affinity_policy_data_t*, not **.  At least this is how public examples on thread_policy_set/get are written (though I can mistake here as I am not an expert in Darwin affinity).


================
Comment at: openmp/runtime/src/z_Linux_util.cpp:130
+#elif KMP_OS_DARWIN
+#define KMP_CPU_SET_SIZE_LIMIT (THREAD_AFFINITY_POLICY_COUNT)
 #endif
----------------
If I read thread_policy.h correctly, the THREAD_AFFINITY_POLICY_COUNT is the number of integers in the thread_affinity_policy_data_t structure, which is 1.  This does not relate with the size of CPU_SET.


================
Comment at: openmp/runtime/src/z_Linux_util.cpp:301
+  mach_msg_type_number_t cnt = KMP_CPU_SET_SIZE_LIMIT;
+  buf = (unsigned char *)KMP_INTERNAL_MALLOC(KMP_CPU_SET_SIZE_LIMIT);
+  gCode = thread_policy_get(mach_thread_self(), THREAD_AFFINITY_POLICY,
----------------
The buf is array of size 1 byte here. Whereas the thread_policy_get writes 4 bytes (sizeof thread_affinity_policy_data_t structure) into buf.



================
Comment at: openmp/runtime/src/z_Linux_util.cpp:308
+  if (gCode == KERN_SUCCESS) {
+    KMP_AFFINITY_ENABLE(KMP_CPU_SET_SIZE_LIMIT);
+    KA_TRACE(10, ("__kmp_affinity_determine_capable: "
----------------
Parameter to KMP_AFFINITY_ENABLE is the size of affinity mask.  It can be 1 if __kmp_affin_mask_size is only used to indicate affinity capable, and the mask is not used as a bit mask.  Or if there is single proc available on teh system that is unlikely nowadays.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72058/new/

https://reviews.llvm.org/D72058