eal/windows: fix invalid thread handle
Checks
Commit Message
Casting thread ID to handle is not accurate way to get thread handle.
Need to use OpenThread function to get thread handle from thread ID.
pthread_setaffinity_np and pthread_getaffinity_np functions
for Windows are affected because of it.
Signed-off-by: Tasnim Bashar <tbashar@mellanox.com>
---
lib/librte_eal/windows/include/pthread.h | 40 +++++++++++++++++++++---
1 file changed, 35 insertions(+), 5 deletions(-)
Comments
On Thu, 21 May 2020 17:11:12 -0700
Tasnim Bashar <tbashar@mellanox.com> wrote:
> Casting thread ID to handle is not accurate way to get thread handle.
> Need to use OpenThread function to get thread handle from thread ID.
>
> pthread_setaffinity_np and pthread_getaffinity_np functions
> for Windows are affected because of it.
Good catch.
Side note:
The sooner we move to a mature third-party pthread implementation, the better.
>
> Signed-off-by: Tasnim Bashar <tbashar@mellanox.com>
> ---
> lib/librte_eal/windows/include/pthread.h | 40 +++++++++++++++++++++---
> 1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h
> index 0bbed5c3b8..d2a986f8fd 100644
> --- a/lib/librte_eal/windows/include/pthread.h
> +++ b/lib/librte_eal/windows/include/pthread.h
> @@ -18,6 +18,7 @@ extern "C" {
>
> #include <windows.h>
> #include <rte_common.h>
> +#include <rte_windows.h>
Build fails, because <rte_windows.h> doesn't include <rte_log.h>, macros from
which it uses. I'd rather include it there, so that <rte_windows.h> could be
used independently, but you can also add an include here.
If you include <rte_windows.h>, you don't need <windows.h> anymore.
>
> #define PTHREAD_BARRIER_SERIAL_THREAD TRUE
>
> @@ -50,7 +51,19 @@ typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
> static inline int
> eal_set_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
> {
> - SetThreadAffinityMask((HANDLE) threadid, *cpuset);
> + HANDLE thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
> + if (thread_handle == NULL) {
> + RTE_LOG_WIN32_ERR("OpenThread()");
> + return -1;
> + }
> +
> + DWORD ret = SetThreadAffinityMask(thread_handle, *cpuset);
GetThreadAffinityMask() and SetThreadAffinityMask() operate on DWORD_PTR (8
bytes), not DWORD (4 byte). This applies to all usages of these functions in
the file and also to the type of "cpuset" parameter: it should be either "long
long *" (as rte_cpuset_t is declared) or just "rte_cpuset_t *".
Also, DPDK coding style suggests declaring variables at the start of the block
(i.e. function here and below).
> + if (ret == 0) {
> + RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
> + CloseHandle(thread_handle);
> + return -1;
> + }
> + CloseHandle(thread_handle);
> return 0;
> }
>
> @@ -60,12 +73,29 @@ eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
> /* Workaround for the lack of a GetThreadAffinityMask()
> *API in Windows
> */
> - /* obtain previous mask by setting dummy mask */
> - DWORD dwprevaffinitymask =
> - SetThreadAffinityMask((HANDLE) threadid, 0x1);
Loss of data for the reason described above: here a 64-bit mask is
stored in a 32-bit variable, so later the wrong value of affinity will be
restored for cores 32 and on.
> + HANDLE thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
> + if (thread_handle == NULL) {
> + RTE_LOG_WIN32_ERR("OpenThread()");
> + return -1;
> + }
> +
> + /* obtain previous mask by setting dummy mask */
> + DWORD dwprevaffinitymask = SetThreadAffinityMask(thread_handle, 0x1);
> + if (dwprevaffinitymask == 0) {
> + RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
> + CloseHandle(thread_handle);
> + return -1;
> + }
> +
> /* set it back! */
> - SetThreadAffinityMask((HANDLE) threadid, dwprevaffinitymask);
> + DWORD ret = SetThreadAffinityMask(thread_handle, dwprevaffinitymask);
> + if (ret == 0) {
> + RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
> + CloseHandle(thread_handle);
> + return -1;
> + }
> *cpuset = dwprevaffinitymask;
> + CloseHandle(thread_handle);
> return 0;
> }
>
--
Dmitry Kozlyuk
@@ -18,6 +18,7 @@ extern "C" {
#include <windows.h>
#include <rte_common.h>
+#include <rte_windows.h>
#define PTHREAD_BARRIER_SERIAL_THREAD TRUE
@@ -50,7 +51,19 @@ typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
static inline int
eal_set_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
{
- SetThreadAffinityMask((HANDLE) threadid, *cpuset);
+ HANDLE thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
+ if (thread_handle == NULL) {
+ RTE_LOG_WIN32_ERR("OpenThread()");
+ return -1;
+ }
+
+ DWORD ret = SetThreadAffinityMask(thread_handle, *cpuset);
+ if (ret == 0) {
+ RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+ CloseHandle(thread_handle);
+ return -1;
+ }
+ CloseHandle(thread_handle);
return 0;
}
@@ -60,12 +73,29 @@ eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
/* Workaround for the lack of a GetThreadAffinityMask()
*API in Windows
*/
- /* obtain previous mask by setting dummy mask */
- DWORD dwprevaffinitymask =
- SetThreadAffinityMask((HANDLE) threadid, 0x1);
+ HANDLE thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
+ if (thread_handle == NULL) {
+ RTE_LOG_WIN32_ERR("OpenThread()");
+ return -1;
+ }
+
+ /* obtain previous mask by setting dummy mask */
+ DWORD dwprevaffinitymask = SetThreadAffinityMask(thread_handle, 0x1);
+ if (dwprevaffinitymask == 0) {
+ RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+ CloseHandle(thread_handle);
+ return -1;
+ }
+
/* set it back! */
- SetThreadAffinityMask((HANDLE) threadid, dwprevaffinitymask);
+ DWORD ret = SetThreadAffinityMask(thread_handle, dwprevaffinitymask);
+ if (ret == 0) {
+ RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+ CloseHandle(thread_handle);
+ return -1;
+ }
*cpuset = dwprevaffinitymask;
+ CloseHandle(thread_handle);
return 0;
}