DPDK patches and discussions
 help / color / mirror / Atom feed
From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
To: Tal Shnaiderman <talshn@nvidia.com>
Cc: dev@dpdk.org, thomas@monjalon.net, pallavi.kadam@intel.com,
	navasile@linux.microsoft.com, dmitrym@microsoft.com,
	david.marchand@redhat.com
Subject: Re: [dpdk-dev] [PATCH v7 2/2] eal: add generic thread-local-storage functions
Date: Wed, 6 Jan 2021 18:05:25 +0300
Message-ID: <20210106180525.3f4dd1a3@sovereign> (raw)
In-Reply-To: <20210105170635.6212-3-talshn@nvidia.com>

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;
> +}


  reply	other threads:[~2021-01-06 15:05 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-13 20:24 [dpdk-dev] [PATCH v2] eal/windows: add pthread TLS function support Tal Shnaiderman
2020-12-15 22:36 ` Dmitry Kozlyuk
2020-12-17 17:49 ` [dpdk-dev] [PATCH v3] eal: add generic thread-local-storage functions Tal Shnaiderman
2020-12-17 20:56   ` Dmitry Kozlyuk
2020-12-18 19:37     ` Tal Shnaiderman
2020-12-22  7:30   ` [dpdk-dev] [PATCH v4] " Tal Shnaiderman
2020-12-23  1:18     ` Dmitry Kozlyuk
2020-12-23 11:44       ` Tal Shnaiderman
2020-12-23 11:58         ` Dmitry Kozlyuk
2020-12-23 18:16           ` [dpdk-dev] [EXTERNAL] " Dmitry Malloy (MESHCHANINOV)
2020-12-26 16:08     ` [dpdk-dev] [PATCH v5] " Tal Shnaiderman
2020-12-29 23:13       ` Dmitry Kozlyuk
2020-12-30 10:04         ` Tal Shnaiderman
2020-12-30 11:12       ` [dpdk-dev] [PATCH v6] " Tal Shnaiderman
2021-01-01 22:16         ` Dmitry Kozlyuk
2021-01-05 11:53         ` Thomas Monjalon
2021-01-05 17:06       ` [dpdk-dev] [PATCH v7 0/2] support generic threading functions Tal Shnaiderman
2021-01-05 17:06         ` [dpdk-dev] [PATCH v7 1/2] eal: move thread affinity functions to new file Tal Shnaiderman
2021-01-06 14:40           ` Dmitry Kozlyuk
2021-01-06 19:45           ` [dpdk-dev] [PATCH v8 0/2] support generic threading functions Tal Shnaiderman
2021-01-06 19:45             ` [dpdk-dev] [PATCH v8 1/2] eal: move thread affinity functions to new file Tal Shnaiderman
2021-01-06 19:45             ` [dpdk-dev] [PATCH v8 2/2] eal: add generic thread-local-storage functions Tal Shnaiderman
2021-01-06 20:10               ` Dmitry Kozlyuk
2021-01-06 20:33                 ` Tal Shnaiderman
2021-01-06 20:35             ` [dpdk-dev] [PATCH v9 0/2] support generic threading functions Tal Shnaiderman
2021-01-06 20:35               ` [dpdk-dev] [PATCH v9 1/2] eal: move thread affinity functions to new file Tal Shnaiderman
2021-01-06 20:35               ` [dpdk-dev] [PATCH v9 2/2] eal: add generic thread-local-storage functions Tal Shnaiderman
2021-01-07 14:46                 ` Dmitry Kozlyuk
2021-02-10 13:33                 ` Burakov, Anatoly
2021-02-10 14:26                   ` Medvedkin, Vladimir
2021-02-11  7:34                     ` Tal Shnaiderman
2021-02-11  7:26                   ` Tal Shnaiderman
2021-01-11 22:33               ` [dpdk-dev] [PATCH v9 0/2] support generic threading functions Thomas Monjalon
2021-01-05 17:06         ` [dpdk-dev] [PATCH v7 2/2] eal: add generic thread-local-storage functions Tal Shnaiderman
2021-01-06 15:05           ` Dmitry Kozlyuk [this message]
2021-01-06 16:04             ` Tal Shnaiderman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210106180525.3f4dd1a3@sovereign \
    --to=dmitry.kozliuk@gmail.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=dmitrym@microsoft.com \
    --cc=navasile@linux.microsoft.com \
    --cc=pallavi.kadam@intel.com \
    --cc=talshn@nvidia.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git