[v2] 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 | 56 ++++++++++++++++----
lib/librte_eal/windows/include/rte_windows.h | 1 +
2 files changed, 47 insertions(+), 10 deletions(-)
Comments
On Sat, 23 May 2020 00:25:56 -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.
>
> Signed-off-by: Tasnim Bashar <tbashar@mellanox.com>
> ---
Please use --in-reply-to=Message-ID-of-previous-version to send v2, etc.
If you're using Outlook, you can find message ID as follows:
https://support.office.com/en-us/article/view-internet-message-headers-in-outlook-cd039382-dc6e-4264-ac74-c048563d212c
For example, this is how this patch should have been sent:
git send-email --in-reply-to=20200522001112.48932-1-tbashar@mellanox.com ...
[snip]
> +
> /* set it back! */
> - SetThreadAffinityMask((HANDLE) threadid, dwprevaffinitymask);
> + ret = SetThreadAffinityMask(thread_handle, dwprevaffinitymask);
> + if (ret == 0) {
> + RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
> + CloseHandle(thread_handle);
> + return -1;
> + }
> *cpuset = dwprevaffinitymask;
Getting a warning with MinGW-w64:
In file included from ../../../../../lib/librte_eal/include/rte_eal.h:15,
from ../../../../../lib/librte_eal/common/eal_common_options.c:24:
../../../../../lib/librte_eal/common/eal_common_options.c: In function 'eal_adjust_config':
../../../../../lib/librte_eal/windows/include/sched.h:63:40: warning: 'default_set' may be used uninitialized in this function [-Wmaybe-uninitialized]
63 | (dst)->_bits[_i] = (src1)->_bits[_i] & (src2)->_bits[_i]; \
| ^
../../../../../lib/librte_eal/common/eal_common_options.c:1624:15: note: 'default_set' was declared here
1624 | rte_cpuset_t default_set;
| ^~~~~~~~~~~
This is correct, because you only initialize first "long long" in
rte_cpuset_t and not the second. Since we're trying not to introduce new
warnings, the fix is as follows:
memset(cpuset, 0, sizeof(rte_cpuset_t));
"sizeof(rte_cpuset_t)" requires <sched.h> and then it's probably worth using
"rte_cpuset_t *" for instead of "long long *" for the parameter.
25/05/2020 03:02, Dmitry Kozlyuk:
> On Sat, 23 May 2020 00:25:56 -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.
> >
> > Signed-off-by: Tasnim Bashar <tbashar@mellanox.com>
> > ---
>
> Please use --in-reply-to=Message-ID-of-previous-version to send v2, etc.
I should update the doc to ask replying to the first patch.
> If you're using Outlook, you can find message ID as follows:
>
> https://support.office.com/en-us/article/view-internet-message-headers-in-outlook-cd039382-dc6e-4264-ac74-c048563d212c
You can also find the message ID in patchwork or inbox.dpdk.org.
> For example, this is how this patch should have been sent:
>
> git send-email --in-reply-to=20200522001112.48932-1-tbashar@mellanox.com ...
@@ -16,8 +16,8 @@
extern "C" {
#endif
-#include <windows.h>
#include <rte_common.h>
+#include <rte_windows.h>
#define PTHREAD_BARRIER_SERIAL_THREAD TRUE
@@ -41,31 +41,67 @@ 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, (long long *) cpuset)
#define pthread_getaffinity_np(thread, size, cpuset) \
- eal_get_thread_affinity_mask(thread, (unsigned long *) cpuset)
+ eal_get_thread_affinity_mask(thread, (long long *) 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, long long *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);
+ 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, long 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);
+ 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);
+ 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;
}
@@ -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.