[dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values

[dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values

[Tal Shnaiderman]

The macro definitions of the following pthread functions
return incorrect values from the inner function return code.

while pthread_barrier_init, pthread_barrier_destroy and
pthread_cancel return 0 in a case of success and non zero (errno) value
otherwise the shimming functions InitializeSynchronizationBarrier,
DeleteSynchronizationBarrier and TerminateThread return FALSE (0)
in a case of failure and TRUE(1) in a case of success.

This issue was undetected as none of the functions return codes was
checked until such check was added in commit 34cc55cce6b1 ("eal: fix
race in control thread creation") exposing the issue by failing
pthread_barrier_init and rte_eal_init on Windows as a result.

The fix aligned the return value of the 3 function with the expected
pthread API return values.

Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and macros")
Fixes: 34cc55cce6b1 ("eal: fix race in control thread creation")
Cc: stable@dpdk.org

Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
---
 lib/librte_eal/windows/include/pthread.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h
index 9aeab1fa70..1939b0121c 100644
--- a/lib/librte_eal/windows/include/pthread.h
+++ b/lib/librte_eal/windows/include/pthread.h
@@ -35,12 +35,12 @@ typedef CRITICAL_SECTION pthread_mutex_t;
 typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
 
 #define pthread_barrier_init(barrier, attr, count) \
-	InitializeSynchronizationBarrier(barrier, count, -1)
+	!InitializeSynchronizationBarrier(barrier, count, -1)
 #define pthread_barrier_wait(barrier) EnterSynchronizationBarrier(barrier, \
 	SYNCHRONIZATION_BARRIER_FLAGS_BLOCK_ONLY)
 #define pthread_barrier_destroy(barrier) \
-	DeleteSynchronizationBarrier(barrier)
-#define pthread_cancel(thread) TerminateThread((HANDLE) thread, 0)
+	!DeleteSynchronizationBarrier(barrier)
+#define pthread_cancel(thread) !TerminateThread((HANDLE) thread, 0)
 
 /* pthread function overrides */
 #define pthread_self() \
-- 
2.16.1.windows.4

Re: [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values

[Dmitry Kozlyuk]

Hi Tal,

Getting warnings from x86_64-w64-mingw32-gcc (GCC) 9.3.0:

../../../lib/librte_eal/common/eal_common_thread.c: In function ‘ctrl_params_free’:
../../../lib/librte_eal/windows/include/pthread.h:42:2: warning: value computed is not used [-Wunused-value]
   42 |  !DeleteSynchronizationBarrier(barrier)

Probably applies to other functions and may fire in combination with future
backported patches. Hopefully since 21.05 there will be new threading API.

Re: [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values

[Tal Shnaiderman]

> Subject: Re: [PATCH] eal/windows: fix pthreads macros return values
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Tal,
> 
> Getting warnings from x86_64-w64-mingw32-gcc (GCC) 9.3.0:
> 
> ../../../lib/librte_eal/common/eal_common_thread.c: In function
> ‘ctrl_params_free’:
> ../../../lib/librte_eal/windows/include/pthread.h:42:2: warning: value
> computed is not used [-Wunused-value]
>    42 |  !DeleteSynchronizationBarrier(barrier)
> 
> Probably applies to other functions and may fire in combination with future
> backported patches. Hopefully since 21.05 there will be new threading API.

Thanks Dmitry, it's odd that the compiler complains only now, I'd expect to see this warning even before the change.

Do we know if the new threading API will be in 21.05? API changes close in 3 days and I didn't see it get reviewed/acked.

I can change only pthread_barrier_init for now, since currently without this change Windows runtime is broken, what do you think? 

Re: [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values

[Thomas Monjalon]

12/04/2021 09:59, Tal Shnaiderman:
> > Subject: Re: [PATCH] eal/windows: fix pthreads macros return values
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > Hi Tal,
> > 
> > Getting warnings from x86_64-w64-mingw32-gcc (GCC) 9.3.0:
> > 
> > ../../../lib/librte_eal/common/eal_common_thread.c: In function
> > ‘ctrl_params_free’:
> > ../../../lib/librte_eal/windows/include/pthread.h:42:2: warning: value
> > computed is not used [-Wunused-value]
> >    42 |  !DeleteSynchronizationBarrier(barrier)
> > 
> > Probably applies to other functions and may fire in combination with future
> > backported patches. Hopefully since 21.05 there will be new threading API.
> 
> Thanks Dmitry, it's odd that the compiler complains only now, I'd expect to see this warning even before the change.
> 
> Do we know if the new threading API will be in 21.05? API changes close in 3 days and I didn't see it get reviewed/acked.

Yes it seems too short for 21.05.

> I can change only pthread_barrier_init for now, since currently without this change Windows runtime is broken, what do you think? 

Re: [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values

[Dmitry Kozlyuk]

2021-04-12 07:59 (UTC+0000), Tal Shnaiderman:
> > Subject: Re: [PATCH] eal/windows: fix pthreads macros return values
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > Hi Tal,
> > 
> > Getting warnings from x86_64-w64-mingw32-gcc (GCC) 9.3.0:
> > 
> > ../../../lib/librte_eal/common/eal_common_thread.c: In function
> > ‘ctrl_params_free’:
> > ../../../lib/librte_eal/windows/include/pthread.h:42:2: warning: value
> > computed is not used [-Wunused-value]
> >    42 |  !DeleteSynchronizationBarrier(barrier)
> > 
> > Probably applies to other functions and may fire in combination with future
> > backported patches. Hopefully since 21.05 there will be new threading API.  
> 
> Thanks Dmitry, it's odd that the compiler complains only now, I'd expect to see this warning even before the change.

These functions don't have "nodiscard"-like attributes,
so a call without using result was OK, now it's an expression.

> Do we know if the new threading API will be in 21.05? API changes close in 3 days and I didn't see it get reviewed/acked.
> I can change only pthread_barrier_init for now, since currently without this change Windows runtime is broken, what do you think? 

(You probably mean pthread_barrier_destroy(), from which the warning comes.)
Yes, this is worth merging as soon as warnings are fixed.
Not sure new threading API will make it into 21.05.

Re: [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values

[David Marchand]

On Sat, Apr 10, 2021 at 9:55 PM Tal Shnaiderman <talshn@nvidia.com> wrote:
>
> The macro definitions of the following pthread functions
> return incorrect values from the inner function return code.
>
> while pthread_barrier_init, pthread_barrier_destroy and
> pthread_cancel return 0 in a case of success and non zero (errno) value
> otherwise the shimming functions InitializeSynchronizationBarrier,
> DeleteSynchronizationBarrier and TerminateThread return FALSE (0)
> in a case of failure and TRUE(1) in a case of success.
>
> This issue was undetected as none of the functions return codes was
> checked until such check was added in commit 34cc55cce6b1 ("eal: fix
> race in control thread creation") exposing the issue by failing
> pthread_barrier_init and rte_eal_init on Windows as a result.
>
> The fix aligned the return value of the 3 function with the expected
> pthread API return values.
>
> Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and macros")
> Fixes: 34cc55cce6b1 ("eal: fix race in control thread creation")

Only the first Fixes: makes sense.
The second commit you refer to relies on a working pthread implementation.


-- 
David Marchand

Re: [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values

[Tal Shnaiderman]

> Subject: Re: [PATCH] eal/windows: fix pthreads macros return values
> 
> External email: Use caution opening links or attachments
> 
> 
> On Sat, Apr 10, 2021 at 9:55 PM Tal Shnaiderman <talshn@nvidia.com>
> wrote:
> >
> > The macro definitions of the following pthread functions return
> > incorrect values from the inner function return code.
> >
> > while pthread_barrier_init, pthread_barrier_destroy and pthread_cancel
> > return 0 in a case of success and non zero (errno) value otherwise the
> > shimming functions InitializeSynchronizationBarrier,
> > DeleteSynchronizationBarrier and TerminateThread return FALSE (0) in a
> > case of failure and TRUE(1) in a case of success.
> >
> > This issue was undetected as none of the functions return codes was
> > checked until such check was added in commit 34cc55cce6b1 ("eal: fix
> > race in control thread creation") exposing the issue by failing
> > pthread_barrier_init and rte_eal_init on Windows as a result.
> >
> > The fix aligned the return value of the 3 function with the expected
> > pthread API return values.
> >
> > Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and
> > macros")
> > Fixes: 34cc55cce6b1 ("eal: fix race in control thread creation")
> 
> Only the first Fixes: makes sense.
> The second commit you refer to relies on a working pthread implementation.
> 

Thanks, will remove in v2.

> 
> --
> David Marchand

[dpdk-dev] [PATCH v2] eal/windows: fix pthreads macros return values

[Tal Shnaiderman]

The macro definitions of the following pthread functions
return incorrect values from the inner function return code.

while pthread_barrier_init, pthread_barrier_destroy and
pthread_cancel return 0 in a case of success and non zero (errno) value
otherwise the shimming functions InitializeSynchronizationBarrier,
DeleteSynchronizationBarrier and TerminateThread return FALSE (0)
in a case of failure and TRUE(1) in a case of success.

This issue was undetected as none of the functions return codes was
checked until such check was added in commit 34cc55cce6b1 ("eal: fix
race in control thread creation") exposing the issue by failing
pthread_barrier_init and rte_eal_init on Windows as a result.

The fix aligned the return value of the 3 function with the expected
pthread API return values.

Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and macros")
Cc: stable@dpdk.org

Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
---
v2:
-fix unused value warning in MinGW-64 [DmitryK]
-remove unneeded "fixes" comment [DavidM]
---
 lib/librte_eal/common/eal_common_thread.c | 4 ++--
 lib/librte_eal/windows/include/pthread.h  | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
index 03dbcd9e86..1a52f42a2b 100644
--- a/lib/librte_eal/common/eal_common_thread.c
+++ b/lib/librte_eal/common/eal_common_thread.c
@@ -176,7 +176,7 @@ struct rte_thread_ctrl_params {
 static void ctrl_params_free(struct rte_thread_ctrl_params *params)
 {
 	if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
-		pthread_barrier_destroy(&params->configured);
+		(void)pthread_barrier_destroy(&params->configured);
 		free(params);
 	}
 }
@@ -251,7 +251,7 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 	return -ret;
 
 fail_with_barrier:
-	pthread_barrier_destroy(&params->configured);
+	(void)pthread_barrier_destroy(&params->configured);
 
 fail_no_barrier:
 	free(params);
diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h
index 9aeab1fa70..1939b0121c 100644
--- a/lib/librte_eal/windows/include/pthread.h
+++ b/lib/librte_eal/windows/include/pthread.h
@@ -35,12 +35,12 @@ typedef CRITICAL_SECTION pthread_mutex_t;
 typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
 
 #define pthread_barrier_init(barrier, attr, count) \
-	InitializeSynchronizationBarrier(barrier, count, -1)
+	!InitializeSynchronizationBarrier(barrier, count, -1)
 #define pthread_barrier_wait(barrier) EnterSynchronizationBarrier(barrier, \
 	SYNCHRONIZATION_BARRIER_FLAGS_BLOCK_ONLY)
 #define pthread_barrier_destroy(barrier) \
-	DeleteSynchronizationBarrier(barrier)
-#define pthread_cancel(thread) TerminateThread((HANDLE) thread, 0)
+	!DeleteSynchronizationBarrier(barrier)
+#define pthread_cancel(thread) !TerminateThread((HANDLE) thread, 0)
 
 /* pthread function overrides */
 #define pthread_self() \
-- 
2.16.1.windows.4

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] eal/windows: fix pthreads macros return values

[Thomas Monjalon]

12/04/2021 12:37, Tal Shnaiderman:
> The macro definitions of the following pthread functions
> return incorrect values from the inner function return code.
> 
> while pthread_barrier_init, pthread_barrier_destroy and
> pthread_cancel return 0 in a case of success and non zero (errno) value
> otherwise the shimming functions InitializeSynchronizationBarrier,
> DeleteSynchronizationBarrier and TerminateThread return FALSE (0)
> in a case of failure and TRUE(1) in a case of success.
> 
> This issue was undetected as none of the functions return codes was
> checked until such check was added in commit 34cc55cce6b1 ("eal: fix
> race in control thread creation") exposing the issue by failing
> pthread_barrier_init and rte_eal_init on Windows as a result.
> 
> The fix aligned the return value of the 3 function with the expected
> pthread API return values.
> 
> Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and macros")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
> ---
> v2:
> -fix unused value warning in MinGW-64 [DmitryK]
> -remove unneeded "fixes" comment [DavidM]

Applied, thanks