Re: [PATCH 1/2] eal/thread: fix return codes for rte_thread_setname()

Re: [PATCH 1/2] eal/thread: fix return codes for rte_thread_setname()

[Stojaczyk, DariuszX]

+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

[PATCH 1/2] eal/thread: fix return codes for rte_thread_setname()

[Dariusz Stojaczyk]

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

[PATCH 2/2] eal/thread: fix return codes for rte_ctrl_thread_create()

[Dariusz Stojaczyk]

This function returned positive error numbers instead
of negative ones as desbribed in the doc. What's worse,
multiple of its callers only check for (rc < 0) to detect
failure.

It was incorrectly assumed that pthread_create
and pthread_setaffinity_np return negative errnos. They
always returns positive ones, so this patch negates their
return values before returning.

Fixes: 9e5afc72c909 ("eal: add function to create control threads")
Cc: olivier.matz@6wind.com
Cc: stable@dpdk.org

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
 lib/librte_eal/common/eal_common_thread.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
index 8110ac2..48ef4d6 100644
--- a/lib/librte_eal/common/eal_common_thread.c
+++ b/lib/librte_eal/common/eal_common_thread.c
@@ -175,7 +175,7 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 
 	params = malloc(sizeof(*params));
 	if (!params)
-		return -1;
+		return -ENOMEM;
 
 	params->start_routine = start_routine;
 	params->arg = arg;
@@ -185,7 +185,7 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 	ret = pthread_create(thread, attr, rte_thread_init, (void *)params);
 	if (ret != 0) {
 		free(params);
-		return ret;
+		return -ret;
 	}
 
 	if (name != NULL) {
@@ -228,5 +228,5 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 	}
 	pthread_cancel(*thread);
 	pthread_join(*thread, NULL);
-	return ret;
+	return -ret;
 }
-- 
2.7.4

Re: [PATCH 1/2] eal/thread: fix return codes for rte_thread_setname()

[Burakov, Anatoly]

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.

-- 
Thanks,
Anatoly

Re: [PATCH 2/2] eal/thread: fix return codes for rte_ctrl_thread_create()

[Burakov, Anatoly]

On 08-Jun-18 1:37 PM, Dariusz Stojaczyk wrote:
> This function returned positive error numbers instead
> of negative ones as desbribed in the doc. What's worse,
> multiple of its callers only check for (rc < 0) to detect
> failure.
> 
> It was incorrectly assumed that pthread_create
> and pthread_setaffinity_np return negative errnos. They
> always returns positive ones, so this patch negates their
> return values before returning.
> 
> Fixes: 9e5afc72c909 ("eal: add function to create control threads")
> Cc: olivier.matz@6wind.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> ---

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

-- 
Thanks,
Anatoly

Re: [PATCH 1/2] eal/thread: fix return codes for rte_thread_setname()

[Olivier Matz]

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?

Re: [PATCH 2/2] eal/thread: fix return codes for rte_ctrl_thread_create()

[Olivier Matz]

On Mon, Jun 18, 2018 at 11:01:40AM +0100, Burakov, Anatoly wrote:
> On 08-Jun-18 1:37 PM, Dariusz Stojaczyk wrote:
> > This function returned positive error numbers instead
> > of negative ones as desbribed in the doc. What's worse,
> > multiple of its callers only check for (rc < 0) to detect
> > failure.
> > 
> > It was incorrectly assumed that pthread_create
> > and pthread_setaffinity_np return negative errnos. They
> > always returns positive ones, so this patch negates their
> > return values before returning.
> > 
> > Fixes: 9e5afc72c909 ("eal: add function to create control threads")
> > Cc: olivier.matz@6wind.com
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> > ---
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

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

Thanks for the fix.

Re: [PATCH 1/2] eal/thread: fix return codes for rte_thread_setname()

[Stojaczyk, DariuszX]

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

[PATCH v2 1/3] eal/thread: ignore rte_thread_setname() failure in ctrl thread

[Dariusz Stojaczyk]

From: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>

The error is not fatal and we can physically continue
creating the thread. It simply won't have a name.

If rte_thread_setname() fails, we will just print
a debug log now. EAL does the same for lcore threads.

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_eal/common/eal_common_thread.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
index 42398630a..8110ac2ae 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;
-- 
2.11.0

[PATCH v2 2/3] eal/thread: fix return codes for rte_thread_setname()

[Dariusz Stojaczyk]

From: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>

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.

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>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
---
Changes from v1:
 * split this patch into two parts

 lib/librte_eal/linuxapp/eal/eal_thread.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_thread.c b/lib/librte_eal/linuxapp/eal/eal_thread.c
index f652ff988..b496fc711 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.11.0

[PATCH v2 3/3] eal/thread: fix return codes for rte_ctrl_thread_create()

[Dariusz Stojaczyk]

From: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>

This function returned positive error numbers instead
of negative ones as desbribed in the doc. What's worse,
multiple of its callers only check for (rc < 0) to detect
failure.

It was incorrectly assumed that pthread_create
and pthread_setaffinity_np return negative errnos. They
always returns positive ones, so this patch negates their
return values before returning.

Fixes: 9e5afc72c909 ("eal: add function to create control threads")
Cc: olivier.matz@6wind.com
Cc: stable@dpdk.org

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_eal/common/eal_common_thread.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
index 8110ac2ae..48ef4d6de 100644
--- a/lib/librte_eal/common/eal_common_thread.c
+++ b/lib/librte_eal/common/eal_common_thread.c
@@ -175,7 +175,7 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 
 	params = malloc(sizeof(*params));
 	if (!params)
-		return -1;
+		return -ENOMEM;
 
 	params->start_routine = start_routine;
 	params->arg = arg;
@@ -185,7 +185,7 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 	ret = pthread_create(thread, attr, rte_thread_init, (void *)params);
 	if (ret != 0) {
 		free(params);
-		return ret;
+		return -ret;
 	}
 
 	if (name != NULL) {
@@ -228,5 +228,5 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 	}
 	pthread_cancel(*thread);
 	pthread_join(*thread, NULL);
-	return ret;
+	return -ret;
 }
-- 
2.11.0

Re: [dpdk-stable] [PATCH v2 1/3] eal/thread: ignore rte_thread_setname() failure in ctrl thread

[Thomas Monjalon]

10/07/2018 12:44, Dariusz Stojaczyk:
> From: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> 
> The error is not fatal and we can physically continue
> creating the thread. It simply won't have a name.
> 
> If rte_thread_setname() fails, we will just print
> a debug log now. EAL does the same for lcore threads.
> 
> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Reviewed-by: Olivier Matz <olivier.matz@6wind.com>

Series applied, thanks