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(-) 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> #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; } -- 2.19.1.windows.1
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
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(-) diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h index 0bbed5c3b8..7b2c355443 100644 --- a/lib/librte_eal/windows/include/pthread.h +++ b/lib/librte_eal/windows/include/pthread.h @@ -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; } diff --git a/lib/librte_eal/windows/include/rte_windows.h b/lib/librte_eal/windows/include/rte_windows.h index ed6e4c1485..677b63c42d 100644 --- 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> /** * Log GetLastError() with context, usually a Win32 API function and arguments. -- 2.19.1.windows.1
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
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) 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(-) diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h index e2274cf4e9..2553c0890a 100644 --- a/lib/librte_eal/windows/include/pthread.h +++ b/lib/librte_eal/windows/include/pthread.h @@ -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); diff --git a/lib/librte_eal/windows/include/rte_windows.h b/lib/librte_eal/windows/include/rte_windows.h index 899ed7d874..d013b50241 100644 --- a/lib/librte_eal/windows/include/rte_windows.h +++ b/lib/librte_eal/windows/include/rte_windows.h @@ -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. -- 2.19.1.windows.1
Hi, Tasnim... On 6/24/2020 4:10 PM, Tasnim Bashar 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> > --- > 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(-) > > diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h > index e2274cf4e9..2553c0890a 100644 > --- a/lib/librte_eal/windows/include/pthread.h > +++ b/lib/librte_eal/windows/include/pthread.h > @@ -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; Initialize ret to 0 here, otherwise... (see below) > + > + 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; ...after this jump, ret is uninitialized and will produce random results at the check at the end of the function. > + } > + > /* 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); > diff --git a/lib/librte_eal/windows/include/rte_windows.h b/lib/librte_eal/windows/include/rte_windows.h > index 899ed7d874..d013b50241 100644 > --- a/lib/librte_eal/windows/include/rte_windows.h > +++ b/lib/librte_eal/windows/include/rte_windows.h > @@ -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.
> From: Ranjit Menon <ranjit.menon@intel.com> > Sent: Wednesday, June 24, 2020 5:39 PM > To: Tasnim Bashar <tbashar@mellanox.com>; dev@dpdk.org > Cc: harini.ramakrishnan@microsoft.com; pallavi.kadam@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>; Thomas > Monjalon <thomas@monjalon.net> > Subject: Re: [PATCH v4] eal/windows: fix thread handle > > Hi, Tasnim... > > On 6/24/2020 4:10 PM, Tasnim Bashar 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> > > --- > > 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(-) > > > > diff --git a/lib/librte_eal/windows/include/pthread.h > > b/lib/librte_eal/windows/include/pthread.h > > index e2274cf4e9..2553c0890a 100644 > > --- a/lib/librte_eal/windows/include/pthread.h > > +++ b/lib/librte_eal/windows/include/pthread.h > > @@ -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; > Initialize ret to 0 here, otherwise... (see below) > > + > > + 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; > ...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. > > + } > > + > > /* 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); > > diff --git a/lib/librte_eal/windows/include/rte_windows.h > > b/lib/librte_eal/windows/include/rte_windows.h > > index 899ed7d874..d013b50241 100644 > > --- a/lib/librte_eal/windows/include/rte_windows.h > > +++ b/lib/librte_eal/windows/include/rte_windows.h > > @@ -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.
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 | 84 +++++++++++++++----- lib/librte_eal/windows/include/rte_windows.h | 1 + 2 files changed, 67 insertions(+), 18 deletions(-) diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h index e2274cf4e9..99013dc94d 100644 --- a/lib/librte_eal/windows/include/pthread.h +++ b/lib/librte_eal/windows/include/pthread.h @@ -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); diff --git a/lib/librte_eal/windows/include/rte_windows.h b/lib/librte_eal/windows/include/rte_windows.h index 899ed7d874..d013b50241 100644 --- a/lib/librte_eal/windows/include/rte_windows.h +++ b/lib/librte_eal/windows/include/rte_windows.h @@ -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. -- 2.19.1.windows.1
25/06/2020 21:25, 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>
Applied, thanks