[v3] 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>
---
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(-)
Comments
02/06/2020 04:00, Tasnim Bashar:
> --- a/lib/librte_eal/windows/include/rte_windows.h
> +++ b/lib/librte_eal/windows/include/rte_windows.h
> @@ -29,6 +29,7 @@
> #define INITGUID
> #endif
> #include <initguid.h>
> +#include <rte_log.h>
Why do you need adding rte_log in rte_windows?
In rte_windows, rte_log is used.
Therefore, rte_log is included to use rte_windows independently.
- Tasnim
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, June 11, 2020 7:35 AM
> To: Tasnim Bashar <tbashar@mellanox.com>
> Cc: dev@dpdk.org; harini.ramakrishnan@microsoft.com;
> pallavi.kadam@intel.com; ranjit.menon@intel.com; ocardona@microsoft.com;
> navasile@linux.microsoft.com; dmitry.kozliuk@gmail.com; Tal Shnaiderman
> <talshn@mellanox.com>; Fady Bader <fady@mellanox.com>; Ophir Munk
> <ophirmu@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v3] eal/windows: fix invalid thread handle
>
> 02/06/2020 04:00, Tasnim Bashar:
> > --- a/lib/librte_eal/windows/include/rte_windows.h
> > +++ b/lib/librte_eal/windows/include/rte_windows.h
> > @@ -29,6 +29,7 @@
> > #define INITGUID
> > #endif
> > #include <initguid.h>
> > +#include <rte_log.h>
>
> Why do you need adding rte_log in rte_windows?
>
12/06/2020 18:22, Tasnim Bashar:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 02/06/2020 04:00, Tasnim Bashar:
> > > --- a/lib/librte_eal/windows/include/rte_windows.h
> > > +++ b/lib/librte_eal/windows/include/rte_windows.h
> > > @@ -29,6 +29,7 @@
> > > #define INITGUID
> > > #endif
> > > #include <initguid.h>
> > > +#include <rte_log.h>
> >
> > Why do you need adding rte_log in rte_windows?
>
> In rte_windows, rte_log is used.
> Therefore, rte_log is included to use rte_windows independently.
OK
02/06/2020 04:00, 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 <tbashar@mellanox.com>
> ---
> 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?
> 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;
Please use underscores to separate parts in names.
> + 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));
Shouldn't we use RTE_CPU_ZERO instead of memset?
> + *cpuset->_bits = dwprevaffinitymask;
> + CloseHandle(thread_handle);
> return 0;
> }
02/06/2020 04:00, Tasnim Bashar:
> #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)
[...]
> 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)
I don't understand the need for the #define.
Why not naming the functions with the final pthread standard name?
> From: Thomas Monjalon <thomas@monjalon.net>
> 02/06/2020 04:00, 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 <tbashar@mellanox.com>
> > ---
> > 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
>
> > 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;
>
> Please use underscores to separate parts in names.
>
> > + 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));
>
> Shouldn't we use RTE_CPU_ZERO instead of memset?
If we use CPU_ZERO or CPU_SET, we still get the same warning!
>
> > + *cpuset->_bits = dwprevaffinitymask;
> > + CloseHandle(thread_handle);
> > return 0;
> > }
>
>
> From: Thomas Monjalon <thomas@monjalon.net>
> 02/06/2020 04:00, Tasnim Bashar:
> > #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)
> [...]
> > 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)
>
> 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.
16/06/2020 20:53, Tasnim Bashar:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > 02/06/2020 04:00, 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 <tbashar@mellanox.com>
> > > ---
> > > 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.
[...]
> > > + 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 ?
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, June 16, 2020 12:17 PM
> To: dmitry.kozliuk@gmail.com; Tasnim Bashar <tbashar@mellanox.com>
> Cc: dev@dpdk.org; harini.ramakrishnan@microsoft.com;
> pallavi.kadam@intel.com; ranjit.menon@intel.com; ocardona@microsoft.com;
> navasile@linux.microsoft.com; Tal Shnaiderman <talshn@mellanox.com>; Fady
> Bader <fady@mellanox.com>; Ophir Munk <ophirmu@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v3] eal/windows: fix invalid thread handle
>
> 16/06/2020 20:53, Tasnim Bashar:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > 02/06/2020 04:00, 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 <tbashar@mellanox.com>
> > > > ---
> > > > 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.
I didn't test with cross compilation. In that case we can separate the warning fix from this patch.
>
> [...]
> > > > + 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
@@ -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;
}
@@ -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.