From: Tal Shnaiderman <talshn@nvidia.com>
To: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
NBU-Contact-Thomas Monjalon <thomas@monjalon.net>,
"pallavi.kadam@intel.com" <pallavi.kadam@intel.com>,
"navasile@linux.microsoft.com" <navasile@linux.microsoft.com>,
"dmitrym@microsoft.com" <dmitrym@microsoft.com>,
"david.marchand@redhat.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 16:04:33 +0000 [thread overview]
Message-ID: <DM6PR12MB39450855FD40253AE55BC178A4D00@DM6PR12MB3945.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20210106180525.3f4dd1a3@sovereign>
> Subject: Re: [PATCH v7 2/2] eal: add generic thread-local-storage functions
>
> External email: Use caution opening links or attachments
>
>
> 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;
> > +}
Thanks for the review Dmitry, I'll fix and resend both patches.
prev parent reply other threads:[~2021-01-06 16:04 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
2021-01-06 16:04 ` Tal Shnaiderman [this message]
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=DM6PR12MB39450855FD40253AE55BC178A4D00@DM6PR12MB3945.namprd12.prod.outlook.com \
--to=talshn@nvidia.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=dmitry.kozliuk@gmail.com \
--cc=dmitrym@microsoft.com \
--cc=navasile@linux.microsoft.com \
--cc=pallavi.kadam@intel.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).