Skip to contentSkip to conversation

eal/windows: fix invalid thread handle

PATCH Superseded Tasnim Bashar 4 participants 17 messages May 22, 2020 – Jun 29, 2020

lore
Share & tools
lore

Tasnim Bashar

author
#0 lore raw

PATCH 1/1 eal/windows: fix invalid thread handle

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 <[email protected]>
---
 lib/librte_eal/windows/include/pthread.h | 40 +++++++++++++++++++++---
 1 file changed, 35 insertions(+), 5 deletions(-)
lib/librte_eal/windows/include/pthread.h +35 -5
index 0bbed5c3b8..d2a986f8fd 100644
@@ -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;
 }
Signature · 1 line
2.19.1.windows.1
#1 lore raw
Quoted 5 lines · On Thu, 21 May 2020 17:11:12 -0700 Tasnim Bashar <[email protected]> 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.

Quoted 11 lines
Signed-off-by Tasnim Bashar <[email protected]>
---
 lib/librte_eal/windows/include/pthread.h | 40 +++++++++++++++++++++---
 1 file changed, 35 insertions(+), 5 deletions(-)
lib/librte_eal/windows/include/pthread.h +0 -0
index 0bbed5c3b8..d2a986f8fd 100644

lib/librte_eal/windows/include/pthread.h extern "C" { · line 21

Quoted 4 lines
lib/librte_eal/windows/include/pthread.h +1 -0
@@ -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.

Quoted 5 lines
lib/librte_eal/windows/include/pthread.h +0 -0
 #define PTHREAD_BARRIER_SERIAL_THREAD TRUE
 

@@ -50,7 +51,19 @@ typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
 static inline int

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 60

Quoted 10 lines
lib/librte_eal/windows/include/pthread.h +7 -1
 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).

Quoted 10 lines
lib/librte_eal/windows/include/pthread.h +6 -0
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		CloseHandle(thread_handle);
+		return -1;
+	}
+	CloseHandle(thread_handle);
 	return 0;
 }

lib/librte_eal/windows/include/pthread.h eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset) · line 65

Quoted 6 lines
lib/librte_eal/windows/include/pthread.h +0 -3
@@ -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.

Quoted 17 lines
lib/librte_eal/windows/include/pthread.h +15 -1
+	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);

lib/librte_eal/windows/include/pthread.h eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset) · line 101

Quoted 10 lines
lib/librte_eal/windows/include/pthread.h +6 -0
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		CloseHandle(thread_handle);
+		return -1;
+	}
 	*cpuset = dwprevaffinitymask;
+	CloseHandle(thread_handle);
 	return 0;
 }

--
Dmitry Kozlyuk

author
#2 lore raw

PATCH v3 1/1 eal/windows: fix invalid thread handle

In reply to Tasnim Bashar

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 <[email protected]>

---
v3: WA to remove warning(-Wmaybe-uninitialized)

---
 lib/librte_eal/windows/include/pthread.h     | 60 ++++++++++++++++----
 lib/librte_eal/windows/include/rte_windows.h |  1 +
 2 files changed, 50 insertions(+), 11 deletions(-)
lib/librte_eal/windows/include/pthread.h +49 -11
index 0bbed5c3b8..7b2c355443 100644
@@ -6,6 +6,7 @@
 #define _PTHREAD_H_
 
 #include <stdint.h>
+#include <sched.h>
 
 /**
  * This file is required to support the common code in eal_common_proc.c,

@@ -16,8 +17,8 @@
 extern "C" {
 #endif
 
-#include <windows.h>
 #include <rte_common.h>
+#include <rte_windows.h>
 
 #define PTHREAD_BARRIER_SERIAL_THREAD TRUE
 

@@ -41,31 +42,68 @@ typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
 #define pthread_self() \
 	((pthread_t)GetCurrentThreadId())
 #define pthread_setaffinity_np(thread, size, cpuset) \
-	eal_set_thread_affinity_mask(thread, (unsigned long *) cpuset)
+	eal_set_thread_affinity_mask(thread, cpuset)
 #define pthread_getaffinity_np(thread, size, cpuset) \
-	eal_get_thread_affinity_mask(thread, (unsigned long *) cpuset)
+	eal_get_thread_affinity_mask(thread, cpuset)
 #define pthread_create(threadid, threadattr, threadfunc, args) \
 	eal_create_thread(threadid, threadfunc, args)
 
 static inline int
-eal_set_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+eal_set_thread_affinity_mask(pthread_t threadid, rte_cpuset_t *cpuset)
 {
-	SetThreadAffinityMask((HANDLE) threadid, *cpuset);
+	DWORD_PTR ret;
+	HANDLE thread_handle;
+
+	thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
+	if (thread_handle == NULL) {
+		RTE_LOG_WIN32_ERR("OpenThread()");
+		return -1;
+	}
+
+	ret = SetThreadAffinityMask(thread_handle, *cpuset->_bits);
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		CloseHandle(thread_handle);
+		return -1;
+	}
+	CloseHandle(thread_handle);
 	return 0;
 }
 
 static inline int
-eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+eal_get_thread_affinity_mask(pthread_t threadid, rte_cpuset_t *cpuset)
 {
 	/* Workaround for the lack of a GetThreadAffinityMask()
 	 *API in Windows
 	 */
-		/* obtain previous mask by setting dummy mask */
-	DWORD dwprevaffinitymask =
-		SetThreadAffinityMask((HANDLE) threadid, 0x1);
+	DWORD_PTR dwprevaffinitymask;
+	HANDLE thread_handle;
+	DWORD_PTR ret;
+
+	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 */
+	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);
-	*cpuset = dwprevaffinitymask;
+	ret = SetThreadAffinityMask(thread_handle, dwprevaffinitymask);
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		CloseHandle(thread_handle);
+		return -1;
+	}
+	memset(cpuset, 0, sizeof(rte_cpuset_t));
+	*cpuset->_bits = dwprevaffinitymask;
+	CloseHandle(thread_handle);
 	return 0;
 }
lib/librte_eal/windows/include/rte_windows.h +1 -0
index ed6e4c1485..677b63c42d 100644
@@ -29,6 +29,7 @@
 #define INITGUID
 #endif
 #include <initguid.h>
+#include <rte_log.h>
 
 /**
  * Log GetLastError() with context, usually a Win32 API function and arguments.
Signature · 1 line
2.19.1.windows.1
#3 lore raw

02/06/2020 04:00, Tasnim Bashar:

lib/librte_eal/windows/include/rte_windows.h line 32

Quoted 7 lines
lib/librte_eal/windows/include/rte_windows.h +1 -0
@@ -29,6 +29,7 @@
 #define INITGUID
 #endif
 #include <initguid.h>
+#include <rte_log.h>

Why do you need adding rte_log in rte_windows?

author
#4 lore raw

In rte_windows, rte_log is used.
Therefore, rte_log is included to use rte_windows independently.
- Tasnim

Quoted 22 lines

-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Thursday, June 11, 2020 7:35 AM
To: Tasnim Bashar <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Tal Shnaiderman <[email protected]>; Fady Bader <[email protected]>; Ophir Munk <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v3] eal/windows: fix invalid thread handle

02/06/2020 04:00, Tasnim Bashar:

lib/librte_eal/windows/include/rte_windows.h line 32

Quoted 7 lines
lib/librte_eal/windows/include/rte_windows.h +1 -0
@@ -29,6 +29,7 @@
 #define INITGUID
 #endif
 #include <initguid.h>
+#include <rte_log.h>

Why do you need adding rte_log in rte_windows?

#5 lore raw

12/06/2020 18:22, Tasnim Bashar:

Quoted 12 lines

From: Thomas Monjalon <[email protected]>

Quoted 10 lines

02/06/2020 04:00, Tasnim Bashar:

lib/librte_eal/windows/include/rte_windows.h line 32

Quoted 7 lines
lib/librte_eal/windows/include/rte_windows.h +1 -0
@@ -29,6 +29,7 @@
 #define INITGUID
 #endif
 #include <initguid.h>
+#include <rte_log.h>

Why do you need adding rte_log in rte_windows?

Quoted 2 lines

In rte_windows, rte_log is used.
Therefore, rte_log is included to use rte_windows independently.

OK

#6 lore raw
In reply to Tasnim Bashar

02/06/2020 04:00, Tasnim Bashar:

Quoted 9 lines

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 <[email protected]>

---
v3: WA to remove warning(-Wmaybe-uninitialized)

The -Wmaybe-uninitialized warning was there before this patch. Shouldn't it be a separate patch before this one?

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 79

Quoted 11 lines

static inline int

lib/librte_eal/windows/include/pthread.h +2 -4
-eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+eal_get_thread_affinity_mask(pthread_t threadid, rte_cpuset_t *cpuset)
 {
 	/* Workaround for the lack of a GetThreadAffinityMask()
 	 *API in Windows
 	 */
-		/* obtain previous mask by setting dummy mask */
-	DWORD dwprevaffinitymask =
-		SetThreadAffinityMask((HANDLE) threadid, 0x1);
+	DWORD_PTR dwprevaffinitymask;

Please use underscores to separate parts in names.

Quoted 17 lines
lib/librte_eal/windows/include/pthread.h +17 -0
+	HANDLE thread_handle;
+	DWORD_PTR ret;
+
+	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 */
+	dwprevaffinitymask = SetThreadAffinityMask(thread_handle, 0x1);
+	if (dwprevaffinitymask == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		CloseHandle(thread_handle);
+		return -1;
+	}
+

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 104

Quoted 10 lines
lib/librte_eal/windows/include/pthread.h +7 -2
 	/* set it back! */
-	SetThreadAffinityMask((HANDLE) threadid, dwprevaffinitymask);
-	*cpuset = dwprevaffinitymask;
+	ret = SetThreadAffinityMask(thread_handle, dwprevaffinitymask);
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		CloseHandle(thread_handle);
+		return -1;
+	}
+	memset(cpuset, 0, sizeof(rte_cpuset_t));

Shouldn't we use RTE_CPU_ZERO instead of memset?

Quoted 4 lines
lib/librte_eal/windows/include/pthread.h +2 -0
+	*cpuset->_bits = dwprevaffinitymask;
+	CloseHandle(thread_handle);
 	return 0;
 }
author
#7 lore raw
Quoted 8 lines

From: Thomas Monjalon <[email protected]>
02/06/2020 04:00, Tasnim Bashar:

Quoted 6 lines

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.

Quoted 6 lines
Quoted 3 lines
Signed-off-by Tasnim Bashar <[email protected]>

---
v3: WA to remove warning(-Wmaybe-uninitialized)

The -Wmaybe-uninitialized warning was there before this patch. Shouldn't it be a separate patch before this one?

The warning appeared only on this patch, so we don't need to separate it

Quoted 38 lines
Quoted 4 lines

static inline int
-eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 79

Quoted 10 lines
lib/librte_eal/windows/include/pthread.h +3 -3
+eal_get_thread_affinity_mask(pthread_t threadid, rte_cpuset_t
+*cpuset)
 {
 	/* Workaround for the lack of a GetThreadAffinityMask()
 	 *API in Windows
 	 */
-		/* obtain previous mask by setting dummy mask */
-	DWORD dwprevaffinitymask =
-		SetThreadAffinityMask((HANDLE) threadid, 0x1);
+	DWORD_PTR dwprevaffinitymask;

Please use underscores to separate parts in names.

Quoted 21 lines
lib/librte_eal/windows/include/pthread.h +18 -2
+	HANDLE thread_handle;
+	DWORD_PTR ret;
+
+	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 */
+	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);
-	*cpuset = dwprevaffinitymask;
+	ret = SetThreadAffinityMask(thread_handle, dwprevaffinitymask);
Quoted 8 lines

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 104

Quoted 6 lines
lib/librte_eal/windows/include/pthread.h +6 -0
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		CloseHandle(thread_handle);
+		return -1;
+	}
+	memset(cpuset, 0, sizeof(rte_cpuset_t));

Shouldn't we use RTE_CPU_ZERO instead of memset?

If we use CPU_ZERO or CPU_SET, we still get the same warning!

Quoted 7 lines
Quoted 5 lines
lib/librte_eal/windows/include/pthread.h +2 -0
+	*cpuset->_bits = dwprevaffinitymask;
+	CloseHandle(thread_handle);
 	return 0;
 }
#8 lore raw

16/06/2020 20:53, Tasnim Bashar:

Quoted 8 lines
Quoted 8 lines

From: Thomas Monjalon <[email protected]>
02/06/2020 04:00, Tasnim Bashar:

Quoted 6 lines

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.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Signed-off-by Tasnim Bashar <[email protected]>

---
v3: WA to remove warning(-Wmaybe-uninitialized)

The -Wmaybe-uninitialized warning was there before this patch. Shouldn't it be a separate patch before this one?

The warning appeared only on this patch, so we don't need to separate it

I can see the warning on the main repo when cross-compiling with MinGW on Linux.

[...]

Quoted 5 lines
Quoted 3 lines

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 104

Quoted 1 line
lib/librte_eal/windows/include/pthread.h +1 -0
+	memset(cpuset, 0, sizeof(rte_cpuset_t));

Shouldn't we use RTE_CPU_ZERO instead of memset?

If we use CPU_ZERO or CPU_SET, we still get the same warning!

That's strange. Does it mean CPU_ZERO is broken in lib/librte_eal/windows/include/sched.h ?

author
#9 lore raw
Quoted 22 lines

From: Thomas Monjalon <[email protected]>
Sent: Tuesday, June 16, 2020 12:17 PM
To: [email protected]; Tasnim Bashar <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Tal Shnaiderman <[email protected]>; Fady Bader <[email protected]>; Ophir Munk <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v3] eal/windows: fix invalid thread handle

16/06/2020 20:53, Tasnim Bashar:

Quoted 12 lines
Quoted 12 lines

From: Thomas Monjalon <[email protected]>
02/06/2020 04:00, Tasnim Bashar:

Quoted 9 lines

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 <[email protected]>

---
v3: WA to remove warning(-Wmaybe-uninitialized)

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

The -Wmaybe-uninitialized warning was there before this patch. Shouldn't it be a separate patch before this one?

The warning appeared only on this patch, so we don't need to separate it

I can see the warning on the main repo when cross-compiling with MinGW on Linux.

I didn't test with cross compilation. In that case we can separate the warning fix from this patch.

Quoted 11 lines

[...]

Quoted 5 lines
Quoted 3 lines

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 104

Quoted 1 line
lib/librte_eal/windows/include/pthread.h +1 -0
+	memset(cpuset, 0, sizeof(rte_cpuset_t));

Shouldn't we use RTE_CPU_ZERO instead of memset?

If we use CPU_ZERO or CPU_SET, we still get the same warning!

That's strange. Does it mean CPU_ZERO is broken in lib/librte_eal/windows/include/sched.h ?

I don't see any issues in CPU_ZERO.
I thinks the issue with compiler interpretation.
I also notice if we check the cpuset is null or not, also removes the warning. But the strange thing is, it removes the warning only if we check like this -> if(!cpuset) If we check like this -> if(cpuset != NULL), we still get the warning

#10 lore raw
In reply to Tasnim Bashar

02/06/2020 04:00, Tasnim Bashar:

Quoted 12 lines

#define pthread_setaffinity_np(thread, size, cpuset) \

lib/librte_eal/windows/include/pthread.h +3 -3
-	eal_set_thread_affinity_mask(thread, (unsigned long *) cpuset)
+	eal_set_thread_affinity_mask(thread, cpuset)
 #define pthread_getaffinity_np(thread, size, cpuset) \
-	eal_get_thread_affinity_mask(thread, (unsigned long *) cpuset)
+	eal_get_thread_affinity_mask(thread, cpuset)
 #define pthread_create(threadid, threadattr, threadfunc, args) \
 	eal_create_thread(threadid, threadfunc, args)
 
 static inline int
-eal_set_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+eal_set_thread_affinity_mask(pthread_t threadid, rte_cpuset_t *cpuset)

[...]

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 74

Quoted 3 lines

static inline int

lib/librte_eal/windows/include/pthread.h +1 -1
-eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+eal_get_thread_affinity_mask(pthread_t threadid, rte_cpuset_t *cpuset)

I don't understand the need for the #define.
Why not naming the functions with the final pthread standard name?

author
#11 lore raw
Quoted 15 lines

From: Thomas Monjalon <[email protected]>
02/06/2020 04:00, Tasnim Bashar:

Quoted 12 lines

#define pthread_setaffinity_np(thread, size, cpuset) \

lib/librte_eal/windows/include/pthread.h +3 -3
-	eal_set_thread_affinity_mask(thread, (unsigned long *) cpuset)
+	eal_set_thread_affinity_mask(thread, cpuset)
 #define pthread_getaffinity_np(thread, size, cpuset) \
-	eal_get_thread_affinity_mask(thread, (unsigned long *) cpuset)
+	eal_get_thread_affinity_mask(thread, cpuset)
 #define pthread_create(threadid, threadattr, threadfunc, args) \
 	eal_create_thread(threadid, threadfunc, args)
 
 static inline int
-eal_set_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+eal_set_thread_affinity_mask(pthread_t threadid, rte_cpuset_t *cpuset)

[...]

Quoted 6 lines

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 74

Quoted 3 lines

static inline int

lib/librte_eal/windows/include/pthread.h +1 -1
-eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+eal_get_thread_affinity_mask(pthread_t threadid, rte_cpuset_t *cpuset)

I don't understand the need for the #define.
Why not naming the functions with the final pthread standard name?

Some functions take more inputs and some take less, that's why we used #define.
But it is possible to implement them without #define, naming the functions with the final pthread standard name. However, it will be better to change it in a different commit.

author
#12 lore raw

PATCH v4 1/1 eal/windows: fix thread handle

In reply to Tasnim Bashar

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 <[email protected]>

---
v3: WA to remove warning(-Wmaybe-uninitialized)
v4: Directly used function name instead of #define

---
 lib/librte_eal/windows/include/pthread.h     | 84 +++++++++++++++-----
 lib/librte_eal/windows/include/rte_windows.h |  1 +
 2 files changed, 67 insertions(+), 18 deletions(-)
lib/librte_eal/windows/include/pthread.h +66 -18
index e2274cf4e9..2553c0890a 100644
@@ -6,6 +6,7 @@
 #define _PTHREAD_H_
 
 #include <stdint.h>
+#include <sched.h>
 
 /**
  * This file is required to support the common code in eal_common_proc.c,

@@ -16,8 +17,8 @@
 extern "C" {
 #endif
 
-#include <windows.h>
 #include <rte_common.h>
+#include <rte_windows.h>
 
 #define PTHREAD_BARRIER_SERIAL_THREAD TRUE
 

@@ -40,37 +41,84 @@ typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
 /* pthread function overrides */
 #define pthread_self() \
 	((pthread_t)GetCurrentThreadId())
-#define pthread_setaffinity_np(thread, size, cpuset) \
-	eal_set_thread_affinity_mask(thread, (unsigned long *) cpuset)
-#define pthread_getaffinity_np(thread, size, cpuset) \
-	eal_get_thread_affinity_mask(thread, (unsigned long *) cpuset)
-#define pthread_create(threadid, threadattr, threadfunc, args) \
-	eal_create_thread(threadid, threadattr, threadfunc, args)
 
 static inline int
-eal_set_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+pthread_setaffinity_np(pthread_t threadid, size_t cpuset_size,
+			rte_cpuset_t *cpuset)
 {
-	SetThreadAffinityMask((HANDLE) threadid, *cpuset);
-	return 0;
+	DWORD_PTR ret;
+	HANDLE thread_handle;
+
+	if (cpuset == NULL || cpuset_size == 0)
+		return -1;
+
+	thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
+	if (thread_handle == NULL) {
+		RTE_LOG_WIN32_ERR("OpenThread()");
+		return -1;
+	}
+
+	ret = SetThreadAffinityMask(thread_handle, *cpuset->_bits);
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		goto close_handle;
+	}
+
+close_handle:
+	if (CloseHandle(thread_handle) == 0) {
+		RTE_LOG_WIN32_ERR("CloseHandle()");
+		return -1;
+	}
+	return (ret == 0) ? -1 : 0;
 }
 
 static inline int
-eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+pthread_getaffinity_np(pthread_t threadid, size_t cpuset_size,
+			rte_cpuset_t *cpuset)
 {
 	/* Workaround for the lack of a GetThreadAffinityMask()
 	 *API in Windows
 	 */
-		/* obtain previous mask by setting dummy mask */
-	DWORD dwprevaffinitymask =
-		SetThreadAffinityMask((HANDLE) threadid, 0x1);
+	DWORD_PTR prev_affinity_mask;
+	HANDLE thread_handle;
+	DWORD_PTR ret;
+
+	if (cpuset == NULL || cpuset_size == 0)
+		return -1;
+
+	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 */
+	prev_affinity_mask = SetThreadAffinityMask(thread_handle, 0x1);
+	if (prev_affinity_mask == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		goto close_handle;
+	}
+
 	/* set it back! */
-	SetThreadAffinityMask((HANDLE) threadid, dwprevaffinitymask);
-	*cpuset = dwprevaffinitymask;
-	return 0;
+	ret = SetThreadAffinityMask(thread_handle, prev_affinity_mask);
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		goto close_handle;
+	}
+
+	memset(cpuset, 0, cpuset_size);
+	*cpuset->_bits = prev_affinity_mask;
+
+close_handle:
+	if (CloseHandle(thread_handle) == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		return -1;
+	}
+	return (ret == 0) ? -1 : 0;
 }
 
 static inline int
-eal_create_thread(void *threadid, const void *threadattr, void *threadfunc,
+pthread_create(void *threadid, const void *threadattr, void *threadfunc,
 		void *args)
 {
 	RTE_SET_USED(threadattr);
lib/librte_eal/windows/include/rte_windows.h +1 -0
index 899ed7d874..d013b50241 100644
@@ -31,6 +31,7 @@
 #define INITGUID
 #endif
 #include <initguid.h>
+#include <rte_log.h>
 
 /**
  * Log GetLastError() with context, usually a Win32 API function and arguments.
Signature · 1 line
2.19.1.windows.1
#13 lore raw

Re: [dpdk-dev] [PATCH v4] eal/windows: fix thread handle

Hi, Tasnim...

Quoted 85 lines · Tasnim Bashar

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 <[email protected]>

---
v3: WA to remove warning(-Wmaybe-uninitialized)
v4: Directly used function name instead of #define

---
  lib/librte_eal/windows/include/pthread.h     | 84 +++++++++++++++-----
  lib/librte_eal/windows/include/rte_windows.h |  1 +
  2 files changed, 67 insertions(+), 18 deletions(-)
lib/librte_eal/windows/include/pthread.h +30 -11
index e2274cf4e9..2553c0890a 100644
@@ -6,6 +6,7 @@
  #define _PTHREAD_H_
 
  #include <stdint.h>
+#include <sched.h>
 
  /**
   * This file is required to support the common code in eal_common_proc.c,

@@ -16,8 +17,8 @@
  extern "C" {
  #endif
 
-#include <windows.h>
  #include <rte_common.h>
+#include <rte_windows.h>
 
  #define PTHREAD_BARRIER_SERIAL_THREAD TRUE
 

@@ -40,37 +41,84 @@ typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
  /* pthread function overrides */
  #define pthread_self() \
  	((pthread_t)GetCurrentThreadId())
-#define pthread_setaffinity_np(thread, size, cpuset) \
-	eal_set_thread_affinity_mask(thread, (unsigned long *) cpuset)
-#define pthread_getaffinity_np(thread, size, cpuset) \
-	eal_get_thread_affinity_mask(thread, (unsigned long *) cpuset)
-#define pthread_create(threadid, threadattr, threadfunc, args) \
-	eal_create_thread(threadid, threadattr, threadfunc, args)
 
  static inline int
-eal_set_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+pthread_setaffinity_np(pthread_t threadid, size_t cpuset_size,
+			rte_cpuset_t *cpuset)
  {
-	SetThreadAffinityMask((HANDLE) threadid, *cpuset);
-	return 0;
+	DWORD_PTR ret;
+	HANDLE thread_handle;
+
+	if (cpuset == NULL || cpuset_size == 0)
+		return -1;
+
+	thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
+	if (thread_handle == NULL) {
+		RTE_LOG_WIN32_ERR("OpenThread()");
+		return -1;
+	}
+
+	ret = SetThreadAffinityMask(thread_handle, *cpuset->_bits);
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		goto close_handle;
+	}
+
+close_handle:
+	if (CloseHandle(thread_handle) == 0) {
+		RTE_LOG_WIN32_ERR("CloseHandle()");
+		return -1;
+	}
+	return (ret == 0) ? -1 : 0;
  }
 
  static inline int
-eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+pthread_getaffinity_np(pthread_t threadid, size_t cpuset_size,
+			rte_cpuset_t *cpuset)

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 84

Quoted 10 lines
lib/librte_eal/windows/include/pthread.h +3 -3
  {
  	/* Workaround for the lack of a GetThreadAffinityMask()
  	 *API in Windows
  	 */
-		/* obtain previous mask by setting dummy mask */
-	DWORD dwprevaffinitymask =
-		SetThreadAffinityMask((HANDLE) threadid, 0x1);
+	DWORD_PTR prev_affinity_mask;
+	HANDLE thread_handle;
+	DWORD_PTR ret;

Initialize ret to 0 here, otherwise... (see below)

Quoted 5 lines
lib/librte_eal/windows/include/pthread.h +5 -0
+
+	if (cpuset == NULL || cpuset_size == 0)
+		return -1;
+
+	thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 99

Quoted 10 lines
lib/librte_eal/windows/include/pthread.h +10 -0
+	if (thread_handle == NULL) {
+		RTE_LOG_WIN32_ERR("OpenThread()");
+		return -1;
+	}
+
+	/* obtain previous mask by setting dummy mask */
+	prev_affinity_mask = SetThreadAffinityMask(thread_handle, 0x1);
+	if (prev_affinity_mask == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		goto close_handle;

...after this jump, ret is uninitialized and will produce random results at the check at the end of the function.

Quoted 34 lines
lib/librte_eal/windows/include/pthread.h +18 -4
+	}
+
  	/* set it back! */
-	SetThreadAffinityMask((HANDLE) threadid, dwprevaffinitymask);
-	*cpuset = dwprevaffinitymask;
-	return 0;
+	ret = SetThreadAffinityMask(thread_handle, prev_affinity_mask);
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		goto close_handle;
+	}
+
+	memset(cpuset, 0, cpuset_size);
+	*cpuset->_bits = prev_affinity_mask;
+
+close_handle:
+	if (CloseHandle(thread_handle) == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		return -1;
+	}
+	return (ret == 0) ? -1 : 0;
  }
 
  static inline int
-eal_create_thread(void *threadid, const void *threadattr, void *threadfunc,
+pthread_create(void *threadid, const void *threadattr, void *threadfunc,
  		void *args)
  {
  	RTE_SET_USED(threadattr);
lib/librte_eal/windows/include/rte_windows.h +0 -0
index 899ed7d874..d013b50241 100644

lib/librte_eal/windows/include/rte_windows.h line 37

Quoted 7 lines
lib/librte_eal/windows/include/rte_windows.h +1 -0
@@ -31,6 +31,7 @@
  #define INITGUID
  #endif
  #include <initguid.h>
+#include <rte_log.h>
 
  /**
   * Log GetLastError() with context, usually a Win32 API function and arguments.

ranjit m.

author
#14 lore raw

Re: [dpdk-dev] [PATCH v4] eal/windows: fix thread handle

Quoted 123 lines

From: Ranjit Menon <[email protected]>
Sent: Wednesday, June 24, 2020 5:39 PM
To: Tasnim Bashar <[email protected]>; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Tal Shnaiderman <[email protected]>; Fady Bader <[email protected]>; Ophir Munk <[email protected]>; Thomas Monjalon <[email protected]>
Subject: Re: [PATCH v4] eal/windows: fix thread handle

Hi, Tasnim...

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER

Quoted 39 lines · Tasnim Bashar

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 <[email protected]>

---
v3: WA to remove warning(-Wmaybe-uninitialized)
v4: Directly used function name instead of #define

---
  lib/librte_eal/windows/include/pthread.h     | 84 +++++++++++++++-----
  lib/librte_eal/windows/include/rte_windows.h |  1 +
  2 files changed, 67 insertions(+), 18 deletions(-)
lib/librte_eal/windows/include/pthread.h +1 -0
index e2274cf4e9..2553c0890a 100644
@@ -6,6 +6,7 @@
  #define _PTHREAD_H_
 
  #include <stdint.h>
+#include <sched.h>
 
  /**
   * This file is required to support the common code in

eal_common_proc.c, @@ -16,8 +17,8 @@
extern "C" {
#endif

lib/librte_eal/windows/include/pthread.h +1 -1
-#include <windows.h>
  #include <rte_common.h>
+#include <rte_windows.h>
 
  #define PTHREAD_BARRIER_SERIAL_THREAD TRUE

@@ -40,37 +41,84 @@ typedef SYNCHRONIZATION_BARRIER

pthread_barrier_t;

Quoted 49 lines
lib/librte_eal/windows/include/pthread.h +28 -10
  /* pthread function overrides */
  #define pthread_self() \
  	((pthread_t)GetCurrentThreadId())
-#define pthread_setaffinity_np(thread, size, cpuset) \
-	eal_set_thread_affinity_mask(thread, (unsigned long *) cpuset)
-#define pthread_getaffinity_np(thread, size, cpuset) \
-	eal_get_thread_affinity_mask(thread, (unsigned long *) cpuset)
-#define pthread_create(threadid, threadattr, threadfunc, args) \
-	eal_create_thread(threadid, threadattr, threadfunc, args)
 
  static inline int
-eal_set_thread_affinity_mask(pthread_t threadid, unsigned long
 *cpuset)
+pthread_setaffinity_np(pthread_t threadid, size_t cpuset_size,
+			rte_cpuset_t *cpuset)
  {
-	SetThreadAffinityMask((HANDLE) threadid, *cpuset);
-	return 0;
+	DWORD_PTR ret;
+	HANDLE thread_handle;
+
+	if (cpuset == NULL || cpuset_size == 0)
+		return -1;
+
+	thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
+	if (thread_handle == NULL) {
+		RTE_LOG_WIN32_ERR("OpenThread()");
+		return -1;
+	}
+
+	ret = SetThreadAffinityMask(thread_handle, *cpuset->_bits);
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		goto close_handle;
+	}
+
+close_handle:
+	if (CloseHandle(thread_handle) == 0) {
+		RTE_LOG_WIN32_ERR("CloseHandle()");
+		return -1;
+	}
+	return (ret == 0) ? -1 : 0;
  }
 
  static inline int
-eal_get_thread_affinity_mask(pthread_t threadid, unsigned long
 *cpuset)
+pthread_getaffinity_np(pthread_t threadid, size_t cpuset_size,
+			rte_cpuset_t *cpuset)

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 84

Quoted 10 lines
lib/librte_eal/windows/include/pthread.h +3 -3
  {
  	/* Workaround for the lack of a GetThreadAffinityMask()
  	 *API in Windows
  	 */
-		/* obtain previous mask by setting dummy mask */
-	DWORD dwprevaffinitymask =
-		SetThreadAffinityMask((HANDLE) threadid, 0x1);
+	DWORD_PTR prev_affinity_mask;
+	HANDLE thread_handle;
+	DWORD_PTR ret;

Initialize ret to 0 here, otherwise... (see below)

Quoted 9 lines
lib/librte_eal/windows/include/pthread.h +9 -0
+
+	if (cpuset == NULL || cpuset_size == 0)
+		return -1;
+
+	thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
+	if (thread_handle == NULL) {
+		RTE_LOG_WIN32_ERR("OpenThread()");
+		return -1;
+	}
Quoted 8 lines

lib/librte_eal/windows/include/pthread.h typedef SYNCHRONIZATION_BARRIER pthread_barrier_t; · line 99

Quoted 6 lines
lib/librte_eal/windows/include/pthread.h +6 -0
+
+	/* obtain previous mask by setting dummy mask */
+	prev_affinity_mask = SetThreadAffinityMask(thread_handle, 0x1);
+	if (prev_affinity_mask == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		goto close_handle;

...after this jump, ret is uninitialized and will produce random results at the check at the end of the function.

Good observation Ranjit. I will send new patch with the fix.

Quoted 48 lines
Quoted 37 lines
lib/librte_eal/windows/include/pthread.h +19 -4
+	}
+
  	/* set it back! */
-	SetThreadAffinityMask((HANDLE) threadid, dwprevaffinitymask);
-	*cpuset = dwprevaffinitymask;
-	return 0;
+	ret = SetThreadAffinityMask(thread_handle, prev_affinity_mask);
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		goto close_handle;
+	}
+
+	memset(cpuset, 0, cpuset_size);
+	*cpuset->_bits = prev_affinity_mask;
+
+close_handle:
+	if (CloseHandle(thread_handle) == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		return -1;
+	}
+	return (ret == 0) ? -1 : 0;
  }
 
  static inline int
-eal_create_thread(void *threadid, const void *threadattr, void
 *threadfunc,
+pthread_create(void *threadid, const void *threadattr, void
+*threadfunc,
  		void *args)
  {
  	RTE_SET_USED(threadattr);
lib/librte_eal/windows/include/rte_windows.h +0 -0
index 899ed7d874..d013b50241 100644

lib/librte_eal/windows/include/rte_windows.h line 37

Quoted 7 lines
lib/librte_eal/windows/include/rte_windows.h +1 -0
@@ -31,6 +31,7 @@
  #define INITGUID
  #endif
  #include <initguid.h>
+#include <rte_log.h>
 
  /**
   * Log GetLastError() with context, usually a Win32 API function and

arguments.

ranjit m.

author
#15 lore raw

PATCH v5 1/1 eal/windows: fix thread handle

In reply to Tasnim Bashar

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 <[email protected]>
---
 lib/librte_eal/windows/include/pthread.h     | 84 +++++++++++++++-----
 lib/librte_eal/windows/include/rte_windows.h |  1 +
 2 files changed, 67 insertions(+), 18 deletions(-)
lib/librte_eal/windows/include/pthread.h +66 -18
index e2274cf4e9..99013dc94d 100644
@@ -6,6 +6,7 @@
 #define _PTHREAD_H_
 
 #include <stdint.h>
+#include <sched.h>
 
 /**
  * This file is required to support the common code in eal_common_proc.c,

@@ -16,8 +17,8 @@
 extern "C" {
 #endif
 
-#include <windows.h>
 #include <rte_common.h>
+#include <rte_windows.h>
 
 #define PTHREAD_BARRIER_SERIAL_THREAD TRUE
 

@@ -40,37 +41,84 @@ typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
 /* pthread function overrides */
 #define pthread_self() \
 	((pthread_t)GetCurrentThreadId())
-#define pthread_setaffinity_np(thread, size, cpuset) \
-	eal_set_thread_affinity_mask(thread, (unsigned long *) cpuset)
-#define pthread_getaffinity_np(thread, size, cpuset) \
-	eal_get_thread_affinity_mask(thread, (unsigned long *) cpuset)
-#define pthread_create(threadid, threadattr, threadfunc, args) \
-	eal_create_thread(threadid, threadattr, threadfunc, args)
 
 static inline int
-eal_set_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+pthread_setaffinity_np(pthread_t threadid, size_t cpuset_size,
+			rte_cpuset_t *cpuset)
 {
-	SetThreadAffinityMask((HANDLE) threadid, *cpuset);
-	return 0;
+	DWORD_PTR ret = 0;
+	HANDLE thread_handle;
+
+	if (cpuset == NULL || cpuset_size == 0)
+		return -1;
+
+	thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
+	if (thread_handle == NULL) {
+		RTE_LOG_WIN32_ERR("OpenThread()");
+		return -1;
+	}
+
+	ret = SetThreadAffinityMask(thread_handle, *cpuset->_bits);
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		goto close_handle;
+	}
+
+close_handle:
+	if (CloseHandle(thread_handle) == 0) {
+		RTE_LOG_WIN32_ERR("CloseHandle()");
+		return -1;
+	}
+	return (ret == 0) ? -1 : 0;
 }
 
 static inline int
-eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
+pthread_getaffinity_np(pthread_t threadid, size_t cpuset_size,
+			rte_cpuset_t *cpuset)
 {
 	/* Workaround for the lack of a GetThreadAffinityMask()
 	 *API in Windows
 	 */
-		/* obtain previous mask by setting dummy mask */
-	DWORD dwprevaffinitymask =
-		SetThreadAffinityMask((HANDLE) threadid, 0x1);
+	DWORD_PTR prev_affinity_mask;
+	HANDLE thread_handle;
+	DWORD_PTR ret = 0;
+
+	if (cpuset == NULL || cpuset_size == 0)
+		return -1;
+
+	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 */
+	prev_affinity_mask = SetThreadAffinityMask(thread_handle, 0x1);
+	if (prev_affinity_mask == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		goto close_handle;
+	}
+
 	/* set it back! */
-	SetThreadAffinityMask((HANDLE) threadid, dwprevaffinitymask);
-	*cpuset = dwprevaffinitymask;
-	return 0;
+	ret = SetThreadAffinityMask(thread_handle, prev_affinity_mask);
+	if (ret == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		goto close_handle;
+	}
+
+	memset(cpuset, 0, cpuset_size);
+	*cpuset->_bits = prev_affinity_mask;
+
+close_handle:
+	if (CloseHandle(thread_handle) == 0) {
+		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
+		return -1;
+	}
+	return (ret == 0) ? -1 : 0;
 }
 
 static inline int
-eal_create_thread(void *threadid, const void *threadattr, void *threadfunc,
+pthread_create(void *threadid, const void *threadattr, void *threadfunc,
 		void *args)
 {
 	RTE_SET_USED(threadattr);
lib/librte_eal/windows/include/rte_windows.h +1 -0
index 899ed7d874..d013b50241 100644
@@ -31,6 +31,7 @@
 #define INITGUID
 #endif
 #include <initguid.h>
+#include <rte_log.h>
 
 /**
  * Log GetLastError() with context, usually a Win32 API function and arguments.
Signature · 1 line
2.19.1.windows.1
Applied
#16 lore raw

Re: [dpdk-dev] [PATCH v5] eal/windows: fix thread handle

25/06/2020 21:25, Tasnim Bashar:

Quoted 7 lines

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 <[email protected]>

Applied, thanks