diff mbox series

eal/windows: fix pthreads macros return values

Message ID 20210410195433.13416-1-talshn@nvidia.com (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers show
Series eal/windows: fix pthreads macros return values | expand

Checks

Context Check Description
ci/intel-Testing success Testing PASS
ci/iol-testing warning Testing issues
ci/iol-abi-testing success Testing PASS
ci/github-robot success github build: passed
ci/travis-robot success travis build: passed
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Tal Shnaiderman April 10, 2021, 7:54 p.m. UTC
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(-)

Comments

Dmitry Kozlyuk April 11, 2021, 9 p.m. UTC | #1
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.
Tal Shnaiderman April 12, 2021, 7:59 a.m. UTC | #2
> 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?
Thomas Monjalon April 12, 2021, 8:25 a.m. UTC | #3
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?
Dmitry Kozlyuk April 12, 2021, 10:03 a.m. UTC | #4
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.
David Marchand April 12, 2021, 10:07 a.m. UTC | #5
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.
Tal Shnaiderman April 12, 2021, 10:26 a.m. UTC | #6
> 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
diff mbox series

Patch

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() \