diff mbox series

[v2,1/2] eal: error number enhancement for thread TLS API

Message ID 20210302152658.9136-2-talshn@nvidia.com (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers show
Series EAL Thread TLS API enhancements | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Tal Shnaiderman March 2, 2021, 3:26 p.m. UTC
add error number reporting to rte_errno in all
functions in the rte_thread_tls_* API.

Suggested-by: Anatoly Burakov <anatoly.burakov@intel.com>
Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
---
 lib/librte_eal/include/rte_thread.h | 6 +++---
 lib/librte_eal/unix/rte_thread.c    | 6 ++++++
 lib/librte_eal/windows/rte_thread.c | 8 +++++++-
 3 files changed, 16 insertions(+), 4 deletions(-)

Comments

Dmitry Kozlyuk March 2, 2021, 5:09 p.m. UTC | #1
2021-03-02 17:26, Tal Shnaiderman:
> add error number reporting to rte_errno in all
> functions in the rte_thread_tls_* API.
> 
> Suggested-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
> ---
>  lib/librte_eal/include/rte_thread.h | 6 +++---
>  lib/librte_eal/unix/rte_thread.c    | 6 ++++++
>  lib/librte_eal/windows/rte_thread.c | 8 +++++++-
>  3 files changed, 16 insertions(+), 4 deletions(-)

Using OS error codes for rte_errno isn't the right thing to do: this way
callers cannot write a portable check of rte_thread_*() result. Consider
returning some suitable stable values. OS-specific error info can be logged
at debug level, as it is already is some places.
Tal Shnaiderman March 3, 2021, 10:37 a.m. UTC | #2
> Subject: Re: [PATCH v2 1/2] eal: error number enhancement for thread TLS
> API
> 
> External email: Use caution opening links or attachments
> 
> 
> 2021-03-02 17:26, Tal Shnaiderman:
> > add error number reporting to rte_errno in all functions in the
> > rte_thread_tls_* API.
> >
> > Suggested-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
> > ---
> >  lib/librte_eal/include/rte_thread.h | 6 +++---
> >  lib/librte_eal/unix/rte_thread.c    | 6 ++++++
> >  lib/librte_eal/windows/rte_thread.c | 8 +++++++-
> >  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> Using OS error codes for rte_errno isn't the right thing to do: this way callers
> cannot write a portable check of rte_thread_*() result. Consider returning
> some suitable stable values. OS-specific error info can be logged at debug
> level, as it is already is some places.

In Linux the error codes return are not OS specific (namely EAGAIN, ENOMEM and EINVAL).

The problem is that documentation on Windows doesn't publish which error code can be returned from GetLastError() for the TLS functions.

Or do we want to use the same errno for both OSs in case the functions fail regardless to the failure reason?
Dmitry Kozlyuk March 3, 2021, 11:10 a.m. UTC | #3
2021-03-03 10:37, Tal Shnaiderman:
> > Subject: Re: [PATCH v2 1/2] eal: error number enhancement for thread TLS
> > API
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > 2021-03-02 17:26, Tal Shnaiderman:  
> > > add error number reporting to rte_errno in all functions in the
> > > rte_thread_tls_* API.
> > >
> > > Suggested-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > > Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
> > > ---
> > >  lib/librte_eal/include/rte_thread.h | 6 +++---
> > >  lib/librte_eal/unix/rte_thread.c    | 6 ++++++
> > >  lib/librte_eal/windows/rte_thread.c | 8 +++++++-
> > >  3 files changed, 16 insertions(+), 4 deletions(-)  
> > 
> > Using OS error codes for rte_errno isn't the right thing to do: this way callers
> > cannot write a portable check of rte_thread_*() result. Consider returning
> > some suitable stable values. OS-specific error info can be logged at debug
> > level, as it is already is some places.  
> 
> In Linux the error codes return are not OS specific (namely EAGAIN, ENOMEM and EINVAL).
> 
> The problem is that documentation on Windows doesn't publish which error code can be returned from GetLastError() for the TLS functions.
> 
> Or do we want to use the same errno for both OSs in case the functions fail regardless to the failure reason?

Yes, so that callers know which codes to check against regardless of the OS.
Tal Shnaiderman March 3, 2021, 11:53 a.m. UTC | #4
> Subject: Re: [PATCH v2 1/2] eal: error number enhancement for thread TLS
> API
> 
> External email: Use caution opening links or attachments
> 
> 
> 2021-03-03 10:37, Tal Shnaiderman:
> > > Subject: Re: [PATCH v2 1/2] eal: error number enhancement for thread
> > > TLS API
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > 2021-03-02 17:26, Tal Shnaiderman:
> > > > add error number reporting to rte_errno in all functions in the
> > > > rte_thread_tls_* API.
> > > >
> > > > Suggested-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > > > Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
> > > > ---
> > > >  lib/librte_eal/include/rte_thread.h | 6 +++---
> > > >  lib/librte_eal/unix/rte_thread.c    | 6 ++++++
> > > >  lib/librte_eal/windows/rte_thread.c | 8 +++++++-
> > > >  3 files changed, 16 insertions(+), 4 deletions(-)
> > >
> > > Using OS error codes for rte_errno isn't the right thing to do: this
> > > way callers cannot write a portable check of rte_thread_*() result.
> > > Consider returning some suitable stable values. OS-specific error
> > > info can be logged at debug level, as it is already is some places.
> >
> > In Linux the error codes return are not OS specific (namely EAGAIN,
> ENOMEM and EINVAL).
> >
> > The problem is that documentation on Windows doesn't publish which
> error code can be returned from GetLastError() for the TLS functions.
> >
> > Or do we want to use the same errno for both OSs in case the functions fail
> regardless to the failure reason?
> 
> Yes, so that callers know which codes to check against regardless of the OS.

This is different from Anatoly's original suggestion that values would be potentially different on different OS's.

Anatoly, is the approach suggested by Dmitry fine with you?
diff mbox series

Patch

diff --git a/lib/librte_eal/include/rte_thread.h b/lib/librte_eal/include/rte_thread.h
index e640ea1857..39737d1829 100644
--- a/lib/librte_eal/include/rte_thread.h
+++ b/lib/librte_eal/include/rte_thread.h
@@ -59,7 +59,7 @@  void rte_thread_get_affinity(rte_cpuset_t *cpusetp);
  *
  * @return
  *   On success, zero.
- *   On failure, a negative number.
+ *   On failure, a negative number and an error number is set in rte_errno.
  */
 
 __rte_experimental
@@ -73,7 +73,7 @@  int rte_thread_tls_key_create(rte_tls_key *key, void (*destructor)(void *));
  *
  * @return
  *   On success, zero.
- *   On failure, a negative number.
+ *   On failure, a negative number and an error number is set in rte_errno.
  */
 __rte_experimental
 int rte_thread_tls_key_delete(rte_tls_key key);
@@ -88,7 +88,7 @@  int rte_thread_tls_key_delete(rte_tls_key key);
  *
  * @return
  *   On success, zero.
- *   On failure, a negative number.
+ *   On failure, a negative number and an error number is set in rte_errno.
  */
 __rte_experimental
 int rte_thread_tls_value_set(rte_tls_key key, const void *value);
diff --git a/lib/librte_eal/unix/rte_thread.c b/lib/librte_eal/unix/rte_thread.c
index 86ffeebc95..86e42bf653 100644
--- a/lib/librte_eal/unix/rte_thread.c
+++ b/lib/librte_eal/unix/rte_thread.c
@@ -24,6 +24,7 @@  rte_thread_tls_key_create(rte_tls_key *key, void (*destructor)(void *))
 	*key = malloc(sizeof(**key));
 	if ((*key) == NULL) {
 		RTE_LOG(DEBUG, EAL, "Cannot allocate TLS key.\n");
+		rte_errno = ENOMEM;
 		return -1;
 	}
 	err = pthread_key_create(&((*key)->thread_index), destructor);
@@ -31,6 +32,7 @@  rte_thread_tls_key_create(rte_tls_key *key, void (*destructor)(void *))
 		RTE_LOG(DEBUG, EAL, "pthread_key_create failed: %s\n",
 			 strerror(err));
 		free(*key);
+		rte_errno = err;
 		return -1;
 	}
 	return 0;
@@ -43,6 +45,7 @@  rte_thread_tls_key_delete(rte_tls_key key)
 
 	if (!key) {
 		RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
+		rte_errno = EINVAL;
 		return -1;
 	}
 	err = pthread_key_delete(key->thread_index);
@@ -50,6 +53,7 @@  rte_thread_tls_key_delete(rte_tls_key key)
 		RTE_LOG(DEBUG, EAL, "pthread_key_delete failed: %s\n",
 			 strerror(err));
 		free(key);
+		rte_errno = err;
 		return -1;
 	}
 	free(key);
@@ -63,12 +67,14 @@  rte_thread_tls_value_set(rte_tls_key key, const void *value)
 
 	if (!key) {
 		RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
+		rte_errno = EINVAL;
 		return -1;
 	}
 	err = pthread_setspecific(key->thread_index, value);
 	if (err) {
 		RTE_LOG(DEBUG, EAL, "pthread_setspecific failed: %s\n",
 			strerror(err));
+		rte_errno = err;
 		return -1;
 	}
 	return 0;
diff --git a/lib/librte_eal/windows/rte_thread.c b/lib/librte_eal/windows/rte_thread.c
index 2e2ab29177..1c226b3e30 100644
--- a/lib/librte_eal/windows/rte_thread.c
+++ b/lib/librte_eal/windows/rte_thread.c
@@ -18,12 +18,14 @@  rte_thread_tls_key_create(rte_tls_key *key,
 	*key = malloc(sizeof(**key));
 	if ((*key) == NULL) {
 		RTE_LOG(DEBUG, EAL, "Cannot allocate TLS key.\n");
+		rte_errno = ENOMEM;
 		return -1;
 	}
 	(*key)->thread_index = TlsAlloc();
 	if ((*key)->thread_index == TLS_OUT_OF_INDEXES) {
 		RTE_LOG_WIN32_ERR("TlsAlloc()");
 		free(*key);
+		rte_errno = GetLastError();
 		return -1;
 	}
 	return 0;
@@ -34,11 +36,13 @@  rte_thread_tls_key_delete(rte_tls_key key)
 {
 	if (!key) {
 		RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
+		rte_errno = EINVAL;
 		return -1;
 	}
 	if (!TlsFree(key->thread_index)) {
 		RTE_LOG_WIN32_ERR("TlsFree()");
 		free(key);
+		rte_errno = GetLastError();
 		return -1;
 	}
 	free(key);
@@ -52,12 +56,14 @@  rte_thread_tls_value_set(rte_tls_key key, const void *value)
 
 	if (!key) {
 		RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
+		rte_errno = EINVAL;
 		return -1;
 	}
 	/* discard const qualifier */
 	p = (char *) (uintptr_t) value;
 	if (!TlsSetValue(key->thread_index, p)) {
 		RTE_LOG_WIN32_ERR("TlsSetValue()");
+		rte_errno = GetLastError();
 		return -1;
 	}
 	return 0;
@@ -76,7 +82,7 @@  rte_thread_tls_value_get(rte_tls_key key)
 	output = TlsGetValue(key->thread_index);
 	if (GetLastError() != ERROR_SUCCESS) {
 		RTE_LOG_WIN32_ERR("TlsGetValue()");
-		rte_errno = ENOEXEC;
+		rte_errno = GetLastError();
 		return NULL;
 	}
 	return output;