[v2,1/2] eal: error number enhancement for thread TLS API
Checks
Commit Message
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
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.
> 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?
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.
> 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?
@@ -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);
@@ -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;
@@ -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;