diff mbox series

[dpdk-dev,1/2] eal/thread: fix return codes for rte_thread_setname()

Message ID 1528461427-164113-1-git-send-email-dariuszx.stojaczyk@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series [dpdk-dev,1/2] eal/thread: fix return codes for rte_thread_setname() | expand

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

Stojaczyk, DariuszX June 8, 2018, 12:37 p.m. UTC
The doc says this function returns negative errno
on error, but it currently returns either -1 or
positive errno.

It was incorrectly assumed that pthread_setname_np()
returns negative error numbers. It always returns
positive ones, so this patch negates its return value
before returning.

While here, also ignore rte_thread_setname() failure
in rte_ctrl_thread_create() and print a debug message
instead.

Fixes: 3901ed99c2f8 ("eal: fix thread naming on FreeBSD")
Cc: thomas.monjalon@6wind.com
Cc: stable@dpdk.org

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
 lib/librte_eal/common/eal_common_thread.c | 3 ++-
 lib/librte_eal/linuxapp/eal/eal_thread.c  | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

Comments

Stojaczyk, DariuszX June 8, 2018, 9:02 a.m. UTC | #1
+CC thomas@monjalon.net 
The previous @6wind email appears to be unavailable now.

> -----Original Message-----
> From: Stojaczyk, DariuszX
> Sent: Friday, June 8, 2018 2:37 PM
> To: dev@dpdk.org
> Cc: Stojaczyk, DariuszX <dariuszx.stojaczyk@intel.com>;
> thomas.monjalon@6wind.com; stable@dpdk.org
> Subject: [PATCH 1/2] eal/thread: fix return codes for rte_thread_setname()
> 
> The doc says this function returns negative errno on error, but it currently
> returns either -1 or positive errno.
> 
> It was incorrectly assumed that pthread_setname_np() returns negative
> error numbers. It always returns positive ones, so this patch negates its
> return value before returning.
> 
> While here, also ignore rte_thread_setname() failure in
> rte_ctrl_thread_create() and print a debug message instead.
> 
> Fixes: 3901ed99c2f8 ("eal: fix thread naming on FreeBSD")
> Cc: thomas.monjalon@6wind.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> ---
>  lib/librte_eal/common/eal_common_thread.c | 3 ++-
> lib/librte_eal/linuxapp/eal/eal_thread.c  | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_thread.c
> b/lib/librte_eal/common/eal_common_thread.c
> index 4239863..8110ac2 100644
> --- a/lib/librte_eal/common/eal_common_thread.c
> +++ b/lib/librte_eal/common/eal_common_thread.c
> @@ -191,7 +191,8 @@ rte_ctrl_thread_create(pthread_t *thread, const
> char *name,
>  	if (name != NULL) {
>  		ret = rte_thread_setname(*thread, name);
>  		if (ret < 0)
> -			goto fail;
> +			RTE_LOG(DEBUG, EAL,
> +				"Cannot set name for ctrl thread\n");
>  	}
> 
>  	cpu_found = 0;
> diff --git a/lib/librte_eal/linuxapp/eal/eal_thread.c
> b/lib/librte_eal/linuxapp/eal/eal_thread.c
> index f652ff9..b496fc7 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_thread.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_thread.c
> @@ -176,7 +176,7 @@ int rte_sys_gettid(void)
> 
>  int rte_thread_setname(pthread_t id, const char *name)  {
> -	int ret = -1;
> +	int ret = ENOSYS;
>  #if defined(__GLIBC__) && defined(__GLIBC_PREREQ)  #if
> __GLIBC_PREREQ(2, 12)
>  	ret = pthread_setname_np(id, name);
> @@ -184,5 +184,5 @@ int rte_thread_setname(pthread_t id, const char
> *name)  #endif
>  	RTE_SET_USED(id);
>  	RTE_SET_USED(name);
> -	return ret;
> +	return -ret;
>  }
> --
> 2.7.4
Anatoly Burakov June 18, 2018, 10 a.m. UTC | #2
On 08-Jun-18 1:37 PM, Dariusz Stojaczyk wrote:
> The doc says this function returns negative errno
> on error, but it currently returns either -1 or
> positive errno.
> 
> It was incorrectly assumed that pthread_setname_np()
> returns negative error numbers. It always returns
> positive ones, so this patch negates its return value
> before returning.
> 
> While here, also ignore rte_thread_setname() failure
> in rte_ctrl_thread_create() and print a debug message
> instead.
> 
> Fixes: 3901ed99c2f8 ("eal: fix thread naming on FreeBSD")
> Cc: thomas.monjalon@6wind.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> ---

For patch contents,

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

However, maybe this should be split in two patches.
Olivier Matz June 25, 2018, 2:35 p.m. UTC | #3
On Mon, Jun 18, 2018 at 11:00:55AM +0100, Burakov, Anatoly wrote:
> On 08-Jun-18 1:37 PM, Dariusz Stojaczyk wrote:
> > The doc says this function returns negative errno
> > on error, but it currently returns either -1 or
> > positive errno.
> > 
> > It was incorrectly assumed that pthread_setname_np()
> > returns negative error numbers. It always returns
> > positive ones, so this patch negates its return value
> > before returning.
> > 
> > While here, also ignore rte_thread_setname() failure
> > in rte_ctrl_thread_create() and print a debug message
> > instead.
> > 
> > Fixes: 3901ed99c2f8 ("eal: fix thread naming on FreeBSD")
> > Cc: thomas.monjalon@6wind.com
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> > ---
> 
> For patch contents,
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> However, maybe this should be split in two patches.

Agree it should be split.

Reviewed-by: Olivier Matz <olivier.matz@6wind.com>

Out of curiosity, do you have a use-case where rte_thread_setname()
fails? The only reason I see is a too long name. Why this error
should be ignored?
Stojaczyk, DariuszX June 25, 2018, 4:58 p.m. UTC | #4
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Monday, June 25, 2018 4:36 PM
> To: Burakov, Anatoly <anatoly.burakov@intel.com>
> Cc: Stojaczyk, DariuszX <dariuszx.stojaczyk@intel.com>; dev@dpdk.org;
> thomas.monjalon@6wind.com; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/2] eal/thread: fix return codes for
> rte_thread_setname()
> 
> On Mon, Jun 18, 2018 at 11:00:55AM +0100, Burakov, Anatoly wrote:
> > On 08-Jun-18 1:37 PM, Dariusz Stojaczyk wrote:
> > > The doc says this function returns negative errno
> > > on error, but it currently returns either -1 or
> > > positive errno.
> > >
> > > It was incorrectly assumed that pthread_setname_np()
> > > returns negative error numbers. It always returns
> > > positive ones, so this patch negates its return value
> > > before returning.
> > >
> > > While here, also ignore rte_thread_setname() failure
> > > in rte_ctrl_thread_create() and print a debug message
> > > instead.
> > >
> > > Fixes: 3901ed99c2f8 ("eal: fix thread naming on FreeBSD")
> > > Cc: thomas.monjalon@6wind.com
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> > > ---
> >
> > For patch contents,
> >
> > Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >
> > However, maybe this should be split in two patches.
> 
> Agree it should be split.
> 
> Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Out of curiosity, do you have a use-case where rte_thread_setname()
> fails? The only reason I see is a too long name. Why this error
> should be ignored?

I don't have any use-case like that. It's just that the error is not fatal and we can physically continue creating the thread. EAL does the same thing for the lcore threads.
diff mbox series

Patch

diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
index 4239863..8110ac2 100644
--- a/lib/librte_eal/common/eal_common_thread.c
+++ b/lib/librte_eal/common/eal_common_thread.c
@@ -191,7 +191,8 @@  rte_ctrl_thread_create(pthread_t *thread, const char *name,
 	if (name != NULL) {
 		ret = rte_thread_setname(*thread, name);
 		if (ret < 0)
-			goto fail;
+			RTE_LOG(DEBUG, EAL,
+				"Cannot set name for ctrl thread\n");
 	}
 
 	cpu_found = 0;
diff --git a/lib/librte_eal/linuxapp/eal/eal_thread.c b/lib/librte_eal/linuxapp/eal/eal_thread.c
index f652ff9..b496fc7 100644
--- a/lib/librte_eal/linuxapp/eal/eal_thread.c
+++ b/lib/librte_eal/linuxapp/eal/eal_thread.c
@@ -176,7 +176,7 @@  int rte_sys_gettid(void)
 
 int rte_thread_setname(pthread_t id, const char *name)
 {
-	int ret = -1;
+	int ret = ENOSYS;
 #if defined(__GLIBC__) && defined(__GLIBC_PREREQ)
 #if __GLIBC_PREREQ(2, 12)
 	ret = pthread_setname_np(id, name);
@@ -184,5 +184,5 @@  int rte_thread_setname(pthread_t id, const char *name)
 #endif
 	RTE_SET_USED(id);
 	RTE_SET_USED(name);
-	return ret;
+	return -ret;
 }