On Tue, 5 Jan 2021 19:06:35 +0200, Tal Shnaiderman wrote:
> Add support for TLS functionality in EAL.
>
> The following functions are added:
> rte_thread_tls_key_create - function to create a TLS data key.
> rte_thread_tls_key_delete - function to delete a TLS data key.
> rte_thread_tls_value_set - function to set value bound to the TLS key
> rte_thread_tls_value_get - function to get value bound to the TLS key
"Function to" is redundant both here and in Doxygen comments.
> TLS key will be defined by the new type rte_tls_key.
Please use present tense when describing patch content.
> The API Allocates the thread local storage (TLS) key.
Typo: "allocates".
> Any thread of the process can subsequently use this key
> to store and retrieve values that are local to the thread.
>
> Those functions are added in addition to TLS capability
> in rte_per_lcore.h to allow user to use the thread reasouce
> destruction capability in rte_thread_tls_key_create.
Err, I assumed we're abstracting pthread, otherwise the capability has
already been there. Also, a typo: "resource".
> Windows implementation is under librte_eal/windows and
> implemented using WIN32 API for Windows only.
>
> common implementation is under librte_eal/common and
> implemented using pthread for UNIX compilation.
librte_eal/unix
[...]
> +/**
> + * Opaque pointer for TLS key.
> + */
> +typedef struct eal_tls_key *rte_tls_key;
Suggesting "TLS key type, an opaque pointer". Now it's unclear if there's a
TLS key and a pointer to it or TLS key is an opaque pointer itself (which is
the case, API-wise).
> +
> /**
> * Set core affinity of the current thread.
> * Support both EAL and non-EAL thread and update TLS.
> @@ -36,4 +41,67 @@ int rte_thread_set_affinity(rte_cpuset_t *cpusetp);
> */
> void rte_thread_get_affinity(rte_cpuset_t *cpusetp);
>
> +/**
> + * Function to create a TLS data key visible to all threads in the process
> + * function need to be called once to create a key usable by all threads.
The sentences repeat each other and are not separated.
> + * rte_tls_key is an opaque pointer used to store the allocated key.
> + * which is later used to get/set a value.
> + * and optional destructor can be set to be called when a thread expires.
"expires" -> "exits", here and below.
No need to repeat parameter description in function description.
> + *
> + * @param key
> + * Pointer to store the allocated rte_tls_key
> + * @param destructor
> + * The function to be called when the thread expires.
> + * Ignored on Windows OS.
> + *
> + * @return
> + * On success, zero.
> + * On failure, a negative number.
> + */
> +
> +__rte_experimental
> +int rte_thread_tls_key_create(rte_tls_key *key, void (*destructor)(void *));
> +
> +/**
> + * Function to delete a TLS data key visible to all threads in the process
> + * rte_tls_key is the opaque pointer allocated by rte_thread_tls_key_create.
> + *
> + * @param key
> + * The rte_tls_key will contain the allocated key
"Will"? It's an rte_tls_key key allocated by rte_thread_tls_key_create().
> + *
> + * @return
> + * On success, zero.
> + * On failure, a negative number.
> + */
> +__rte_experimental
> +int rte_thread_tls_key_delete(rte_tls_key key);
> +/**
> + * Function to set value bound to the tls key on behalf of the calling thread
"tls" -> "TLS"
> + *
> + * @param key
> + * The rte_tls_key key allocated by rte_thread_tls_key_create.
> + * @param value
> + * The value bound to the rte_tls_key key for the calling thread.
> + *
> + * @return
> + * On success, zero.
> + * On failure, a negative number.
> + */
> +__rte_experimental
> +int rte_thread_tls_value_set(rte_tls_key key, const void *value);
> +
> +/**
> + * Function to get value bound to the tls key on behalf of the calling thread
"tls" -> "TLS"
> + *
> + * @param key
> + * The rte_tls_key key allocated by rte_thread_tls_key_create.
> + *
> + * @return
> + * On success, value data pointer (can also be NULL).
> + * On failure, NULL and an error number is set in rte_errno.
> + */
> +__rte_experimental
> +void *rte_thread_tls_value_get(rte_tls_key key);
> +
> #endif /* _RTE_THREAD_H_ */
> diff --git a/lib/librte_eal/windows/rte_thread.c
> b/lib/librte_eal/windows/rte_thread.c
> new file mode 100644
> index 0000000000..4ced283c62
> --- /dev/null
> +++ b/lib/librte_eal/windows/rte_thread.c
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 Mellanox Technologies, Ltd
> + */
> +
> +#include <rte_common.h>
> +#include <rte_errno.h>
> +#include <rte_thread.h>
> +#include <rte_windows.h>
> +
> +struct eal_tls_key {
> + DWORD thread_index;
> +};
> +
> +int
> +rte_thread_tls_key_create(rte_tls_key *key,
> + __rte_unused void (*destructor)(void *))
> +{
> + *key = malloc(sizeof(struct eal_tls_key));
sizeof(*key)
> + if ((*key) == NULL) {
> + RTE_LOG(DEBUG, EAL, "Cannot allocate tls key.");
"tls" -> "TLS"
> + return -1;
> + }
> + (*key)->thread_index = TlsAlloc();
> + if ((*key)->thread_index == TLS_OUT_OF_INDEXES) {
> + RTE_LOG_WIN32_ERR("TlsAlloc()");
> + free(*key);
> + return -1;
> + }
> + return 0;
> +}
> +
> +int
> +rte_thread_tls_key_delete(rte_tls_key key)
> +{
> + if (!key) {
> + RTE_LOG(DEBUG, EAL, "invalid tls key passed to function.\n");
"tls" -> "TLS"
Messages should start with a capital letter.
To which "function"?
> + return -1;
> + }
> + if (!TlsFree(key->thread_index)) {
> + RTE_LOG_WIN32_ERR("TlsFree()");
> + free(key);
> + return -1;
> + }
> + free(key);
> + return 0;
> +}
