DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values
@ 2021-04-10 19:54 Tal Shnaiderman
  2021-04-11 21:00 ` Dmitry Kozlyuk
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Tal Shnaiderman @ 2021-04-10 19:54 UTC (permalink / raw)
  To: dev
  Cc: thomas, pallavi.kadam, dmitry.kozliuk, navasile, dmitrym,
	david.marchand, lucp.at.work, stable

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values
  2021-04-10 19:54 [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values Tal Shnaiderman
@ 2021-04-11 21:00 ` Dmitry Kozlyuk
  2021-04-12  7:59   ` Tal Shnaiderman
  2021-04-12 10:07 ` David Marchand
  2021-04-12 10:37 ` [dpdk-dev] [PATCH v2] " Tal Shnaiderman
  2 siblings, 1 reply; 9+ messages in thread
From: Dmitry Kozlyuk @ 2021-04-11 21:00 UTC (permalink / raw)
  To: Tal Shnaiderman
  Cc: dev, thomas, pallavi.kadam, navasile, dmitrym, david.marchand,
	lucp.at.work, stable

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values
  2021-04-11 21:00 ` Dmitry Kozlyuk
@ 2021-04-12  7:59   ` Tal Shnaiderman
  2021-04-12  8:25     ` Thomas Monjalon
  2021-04-12 10:03     ` Dmitry Kozlyuk
  0 siblings, 2 replies; 9+ messages in thread
From: Tal Shnaiderman @ 2021-04-12  7:59 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: dev, NBU-Contact-Thomas Monjalon, pallavi.kadam, navasile,
	dmitrym, david.marchand, lucp.at.work, stable

> 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? 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values
  2021-04-12  7:59   ` Tal Shnaiderman
@ 2021-04-12  8:25     ` Thomas Monjalon
  2021-04-12 10:03     ` Dmitry Kozlyuk
  1 sibling, 0 replies; 9+ messages in thread
From: Thomas Monjalon @ 2021-04-12  8:25 UTC (permalink / raw)
  To: Dmitry Kozlyuk, Tal Shnaiderman
  Cc: dev, pallavi.kadam, navasile, dmitrym, david.marchand,
	lucp.at.work, stable

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? 




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values
  2021-04-12  7:59   ` Tal Shnaiderman
  2021-04-12  8:25     ` Thomas Monjalon
@ 2021-04-12 10:03     ` Dmitry Kozlyuk
  1 sibling, 0 replies; 9+ messages in thread
From: Dmitry Kozlyuk @ 2021-04-12 10:03 UTC (permalink / raw)
  To: Tal Shnaiderman
  Cc: dev, NBU-Contact-Thomas Monjalon, pallavi.kadam, navasile,
	dmitrym, david.marchand, lucp.at.work, stable

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values
  2021-04-10 19:54 [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values Tal Shnaiderman
  2021-04-11 21:00 ` Dmitry Kozlyuk
@ 2021-04-12 10:07 ` David Marchand
  2021-04-12 10:26   ` Tal Shnaiderman
  2021-04-12 10:37 ` [dpdk-dev] [PATCH v2] " Tal Shnaiderman
  2 siblings, 1 reply; 9+ messages in thread
From: David Marchand @ 2021-04-12 10:07 UTC (permalink / raw)
  To: Tal Shnaiderman
  Cc: dev, Thomas Monjalon, Pallavi Kadam, Dmitry Kozlyuk,
	Narcisa Ana Maria Vasile, Dmitry Malloy (MESHCHANINOV),
	Luc Pelletier, dpdk stable

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values
  2021-04-12 10:07 ` David Marchand
@ 2021-04-12 10:26   ` Tal Shnaiderman
  0 siblings, 0 replies; 9+ messages in thread
From: Tal Shnaiderman @ 2021-04-12 10:26 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, NBU-Contact-Thomas Monjalon, Pallavi Kadam, Dmitry Kozlyuk,
	Narcisa Ana Maria Vasile, Dmitry Malloy (MESHCHANINOV),
	Luc Pelletier, dpdk stable

> 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [dpdk-dev] [PATCH v2] eal/windows: fix pthreads macros return values
  2021-04-10 19:54 [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values Tal Shnaiderman
  2021-04-11 21:00 ` Dmitry Kozlyuk
  2021-04-12 10:07 ` David Marchand
@ 2021-04-12 10:37 ` Tal Shnaiderman
  2021-04-12 20:38   ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
  2 siblings, 1 reply; 9+ messages in thread
From: Tal Shnaiderman @ 2021-04-12 10:37 UTC (permalink / raw)
  To: dev
  Cc: thomas, pallavi.kadam, dmitry.kozliuk, navasile, dmitrym,
	david.marchand, lucp.at.work, stable

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] eal/windows: fix pthreads macros return values
  2021-04-12 10:37 ` [dpdk-dev] [PATCH v2] " Tal Shnaiderman
@ 2021-04-12 20:38   ` Thomas Monjalon
  0 siblings, 0 replies; 9+ messages in thread
From: Thomas Monjalon @ 2021-04-12 20:38 UTC (permalink / raw)
  To: Tal Shnaiderman
  Cc: dev, stable, pallavi.kadam, dmitry.kozliuk, navasile, dmitrym,
	david.marchand, lucp.at.work

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




^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-04-12 20:38 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-10 19:54 [dpdk-dev] [PATCH] eal/windows: fix pthreads macros return values Tal Shnaiderman
2021-04-11 21:00 ` Dmitry Kozlyuk
2021-04-12  7:59   ` Tal Shnaiderman
2021-04-12  8:25     ` Thomas Monjalon
2021-04-12 10:03     ` Dmitry Kozlyuk
2021-04-12 10:07 ` David Marchand
2021-04-12 10:26   ` Tal Shnaiderman
2021-04-12 10:37 ` [dpdk-dev] [PATCH v2] " Tal Shnaiderman
2021-04-12 20:38   ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git