From: "Mattias Rönnblom" <hofors@lysator.liu.se>
To: Tyler Retzlaff <roretzla@linux.microsoft.com>
Cc: dev@dpdk.org, thomas@monjalon.net, david.marchand@redhat.com,
stephen@networkplumber.org, olivier.matz@6wind.com,
mb@smartsharesystems.com
Subject: Re: [PATCH v2 1/3] eal: add rte control thread create API
Date: Thu, 8 Dec 2022 22:59:13 +0100 [thread overview]
Message-ID: <3e6b1bb8-e97d-17b1-207e-ef9d0447480d@lysator.liu.se> (raw)
In-Reply-To: <20221207163839.GB14713@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
On 2022-12-07 17:38, Tyler Retzlaff wrote:
> On Wed, Dec 07, 2022 at 10:13:39AM +0100, Mattias Rönnblom wrote:
>> On 2022-12-06 18:28, Tyler Retzlaff wrote:
>>> Add rte_control_thread_create API as a replacement for
>>> rte_ctrl_thread_create to allow deprecation of the use of platform
>>> specific types in DPDK public API.
>>>
>>> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
>>> ---
>>> lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
>>> lib/eal/include/rte_thread.h | 29 ++++++++++++
>>> lib/eal/version.map | 3 ++
>>> 3 files changed, 117 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
>>> index c5d8b43..7950b50 100644
>>> --- a/lib/eal/common/eal_common_thread.c
>>> +++ b/lib/eal/common/eal_common_thread.c
>>> @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
>>> };
>>> struct rte_thread_ctrl_params {
>>> - void *(*start_routine)(void *);
>>> + union {
>>> + void *(*start_routine)(void *);
>>> + rte_thread_func thread_func;
>>
>> To be consistent with function naming scheme, where "ctrl" is the
>> old API, and "control" the new, you could call the start functions
>> something with "ctrl" and "control" as well.
>
> i'll make this change in v3.
>
>>
>> Maybe it's worth mentioning that fact somewhere in the beginning of
>> the file, as a comment (i.e., that "ctrl" denotes the old API).
>
> i'll make this change in v3.
>
>>
>>> + } u;
>>> void *arg;
>>> int ret;
>>
>> Why is 'ret' needed? (This question is unrelated to your patch.)
>
> i'm not the original author so difficult to answer authoritatively. but
> if i have to speculate i'd say it might be to work around the windows
> pthread_join stub being implemented as a noop. specifically it didn't
> communicate the return value from the start_routine.
>
> the recently added rte_thread_join addresses this (which
> rte_control_thread_create uses) and could remove ret parameter and to
> avoid touching the new function implementation in the future it can not
> use ret now.
>
> i'll make this change in v3.
>
> for the original function i will leave the code as is to minimize the
> diff. when rte_ctrl_thread_create is removed i'll make a note to
> eliminate the ret parameter as well.
>
I would focus on minimizing the complexity of the end result, rather
than the size of the patch.
>>
>>> /* Control thread status.
>>> @@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
>>> enum __rte_ctrl_thread_status ctrl_thread_status;
>>> };
>>> -static void *ctrl_thread_init(void *arg)
>>> +static int ctrl_thread_init(void *arg)
>>> {
>>> struct internal_config *internal_conf =
>>> eal_get_internal_configuration();
>>> rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
>>> struct rte_thread_ctrl_params *params = arg;
>>> - void *(*start_routine)(void *) = params->start_routine;
>>> - void *routine_arg = params->arg;
>>> __rte_thread_init(rte_lcore_id(), cpuset);
>>> params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
>>> @@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
>>> if (params->ret != 0) {
>>> __atomic_store_n(¶ms->ctrl_thread_status,
>>> CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
>>> - return NULL;
>>> + return params->ret;
>>> }
>>> __atomic_store_n(¶ms->ctrl_thread_status,
>>> CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
>>> - return start_routine(routine_arg);
>>> + return 0;
>>> +}
>>> +
>>> +static void *ctrl_thread_start(void *arg)
>>> +{
>>> + struct rte_thread_ctrl_params *params = arg;
>>> + void *(*start_routine)(void *) = params->u.start_routine;
>>> +
>>> + if (ctrl_thread_init(arg) != 0)
>>> + return NULL;
>>> +
>>> + return start_routine(params->arg);
>>> +}
>>> +
>>> +static uint32_t control_thread_start(void *arg)
>>> +{
>>> + struct rte_thread_ctrl_params *params = arg;
>>> + rte_thread_func start_routine = params->u.thread_func;
>>> +
>>> + if (ctrl_thread_init(arg) != 0)
>>> + return params->ret;
>>> +
>>> + return start_routine(params->arg);
>>> }
>>> int
>>> @@ -280,12 +303,12 @@ static void *ctrl_thread_init(void *arg)
>>> if (!params)
>>> return -ENOMEM;
>>> - params->start_routine = start_routine;
>>> + params->u.start_routine = start_routine;
>>> params->arg = arg;
>>> params->ret = 0;
>>> params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
>>> - ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
>>> + ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
>>> if (ret != 0) {
>>> free(params);
>>> return -ret;
>>> @@ -322,6 +345,60 @@ static void *ctrl_thread_init(void *arg)
>>> }
>>> int
>>> +rte_control_thread_create(rte_thread_t *thread, const char *name,
>>> + const rte_thread_attr_t *attr,
>>> + rte_thread_func start_routine, void *arg)
>>> +{
>>> + struct rte_thread_ctrl_params *params;
>>> + enum __rte_ctrl_thread_status ctrl_thread_status;
>>> + int ret;
>>> +
>>> + params = malloc(sizeof(*params));
>>> + if (!params)
>>
>> params == NULL
>
> copied from original code, i'll fix the style in the new function to
> comply with the dpdk coding standard.
>
> i'll fix in v3.
>
>>
>>> + return -ENOMEM;
>>> +
>>> + params->u.thread_func = start_routine;
>>> + params->arg = arg;
>>> + params->ret = 0;
>>> + params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
>>> +
>>> + ret = rte_thread_create(thread, attr, control_thread_start, params);
>>> + if (ret != 0) {
>>> + free(params);
>>> + return -ret;
>>> + }
>>> +
>>> + if (name != NULL) {
>>> + ret = rte_thread_setname((pthread_t)thread->opaque_id, name);
>>> + if (ret < 0)
>>> + RTE_LOG(DEBUG, EAL,
>>> + "Cannot set name for ctrl thread\n");
>>> + }
>>> +
>>> + /* Wait for the control thread to initialize successfully */
>>> + while ((ctrl_thread_status =
>>> + __atomic_load_n(¶ms->ctrl_thread_status,
>>> + __ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
>>> + /* Yield the CPU. Using sched_yield call requires maintaining
>>> + * another implementation for Windows as sched_yield is not
>>> + * supported on Windows.
>>> + */
>>
>> sched_yield() also doesn't necessarily yield the CPU.
>
> copied from original code and understood, but are you requesting a
> change here or just a comment correction? can you offer wording you
> would find suitable?
>
I would just remove the comment.
"Yield the CPU. sched_yield() may seem like a natural choice here, but
does not guarantee that a context switch will actually occur, and also
does not exist on Windows."
>>
>>> + rte_delay_us_sleep(1);
>>> + }
>>> +
>>> + /* Check if the control thread encountered an error */
>>> + if (ctrl_thread_status == CTRL_THREAD_ERROR) {
>>> + /* ctrl thread is exiting */
>>> + rte_thread_join(*thread, NULL);
>>> + }
>>> +
>>> + ret = params->ret;
>>> + free(params);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +int
>>> rte_thread_register(void)
>>> {
>>> unsigned int lcore_id;
>>> diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
>>> index b9edf70..ae7afbe 100644
>>> --- a/lib/eal/include/rte_thread.h
>>> +++ b/lib/eal/include/rte_thread.h
>>> @@ -95,6 +95,35 @@ int rte_thread_create(rte_thread_t *thread_id,
>>> rte_thread_func thread_func, void *arg);
>>> /**
>>> + * Create a control thread.
>>> + *
>>> + * Creates a control thread with the given name and attributes. The
>>> + * affinity of the new thread is based on the CPU affinity retrieved
>>> + * at the time rte_eal_init() was called, the dataplane and service
>>> + * lcores are then excluded. If setting the name of the thread fails,
>>
>> "the EAL threads are then excluded"
>
> i'll modify in v3.
>
>>
>>> + * the error is ignored and a debug message is logged.
>>> + *
>>> + * @param thread
>>> + * Filled with the thread id of the new created thread.
>>> + * @param name
>>> + * The name of the control thread (max 16 characters including '\0').
>>
>> Is there a constant for this limit?
>
> i have a series introducing rte_lcore_{set,get}_name api that introduces
> a constant (probably i'll post it today). as of this series there is no
> constant.
>
>>
>>> + * @param thread_attr
>>> + * Attributes for the new thread.
>>> + * @param thread_func
>>> + * Function to be executed by the new thread.
>>> + * @param arg
>>> + * Argument passed to start_routine.
>>> + * @return
>>> + * On success, returns 0; on error, it returns a negative value
>>> + * corresponding to the error number.
>>
>> List the possible error codes.
>
> i would like to see the range of codes be part of the api & abi i've
> received resistance to the idea. for now i'll nack this comment which
> leaves it consistent with other apis documentation.
>>
>>> + */
>>> +__rte_experimental
>>> +int
>>> +rte_control_thread_create(rte_thread_t *thread, const char *name,
>>> + const rte_thread_attr_t *thread_attr,
>>> + rte_thread_func thread_func, void *arg);
>>> +
>>> +/**
>>> * @warning
>>> * @b EXPERIMENTAL: this API may change without prior notice.
>>> *
>>> diff --git a/lib/eal/version.map b/lib/eal/version.map
>>> index 7ad12a7..8f9eeb9 100644
>>> --- a/lib/eal/version.map
>>> +++ b/lib/eal/version.map
>>> @@ -440,6 +440,9 @@ EXPERIMENTAL {
>>> rte_thread_detach;
>>> rte_thread_equal;
>>> rte_thread_join;
>>> +
>>> + # added in 23.03
>>> + rte_control_thread_create;
>>> };
>>> INTERNAL {
next prev parent reply other threads:[~2022-12-08 21:59 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-05 20:24 [PATCH 0/3] eal: rte_ctrl_thread_create API replacement Tyler Retzlaff
2022-12-05 20:24 ` [PATCH 1/3] eal: add rte control thread create API Tyler Retzlaff
2022-12-05 21:11 ` Stephen Hemminger
2022-12-06 0:21 ` Tyler Retzlaff
2022-12-06 17:35 ` Tyler Retzlaff
2022-12-07 0:49 ` Stephen Hemminger
2022-12-05 20:24 ` [PATCH 2/3] test: add rte control thread create API test Tyler Retzlaff
2022-12-05 20:24 ` [PATCH 3/3] eal: deprecate pthread control thread create API Tyler Retzlaff
2022-12-05 21:18 ` Stephen Hemminger
2022-12-06 0:24 ` Tyler Retzlaff
2022-12-05 20:34 ` [PATCH 0/3] eal: rte_ctrl_thread_create API replacement Morten Brørup
2022-12-06 17:28 ` [PATCH v2 " Tyler Retzlaff
2022-12-06 17:28 ` [PATCH v2 1/3] eal: add rte control thread create API Tyler Retzlaff
2022-12-07 9:13 ` Mattias Rönnblom
2022-12-07 16:38 ` Tyler Retzlaff
2022-12-07 20:37 ` Tyler Retzlaff
2022-12-08 21:59 ` Mattias Rönnblom [this message]
2022-12-08 22:15 ` Tyler Retzlaff
2022-12-09 1:09 ` Stephen Hemminger
2022-12-09 19:49 ` Tyler Retzlaff
2022-12-06 17:28 ` [PATCH v2 2/3] test: add rte control thread create API test Tyler Retzlaff
2022-12-06 17:28 ` [PATCH v2 3/3] eal: deprecate pthread control thread create API Tyler Retzlaff
2022-12-07 20:31 ` [PATCH v3 0/3] eal: rte_ctrl_thread_create API replacement Tyler Retzlaff
2022-12-07 20:31 ` [PATCH v3 1/3] eal: add rte control thread create API Tyler Retzlaff
2022-12-07 20:31 ` [PATCH v3 2/3] test: add rte control thread create API test Tyler Retzlaff
2022-12-07 20:31 ` [PATCH v3 3/3] eal: deprecate pthread control thread create API Tyler Retzlaff
2022-12-08 23:19 ` [PATCH v4 0/3] eal: rte_ctrl_thread_create API replacement Tyler Retzlaff
2022-12-08 23:19 ` [PATCH v4 1/3] eal: add rte control thread create API Tyler Retzlaff
2022-12-08 23:19 ` [PATCH v4 2/3] test: add rte control thread create API test Tyler Retzlaff
2022-12-08 23:19 ` [PATCH v4 3/3] eal: deprecate pthread control thread create API Tyler Retzlaff
2022-12-10 14:31 ` [PATCH v4 0/3] eal: rte_ctrl_thread_create API replacement Mattias Rönnblom
2023-01-31 19:46 ` [PATCH v5 0/3] eal: deprecate last use of pthread_t in public API Tyler Retzlaff
2023-01-31 19:46 ` [PATCH v5 1/3] eal: add rte control thread create API Tyler Retzlaff
2023-01-31 19:46 ` [PATCH v5 2/3] doc: add missing index entry for thread Tyler Retzlaff
2023-01-31 19:46 ` [PATCH v5 3/3] doc: announce deprecation of thread ctrl create function Tyler Retzlaff
2023-01-31 20:30 ` [PATCH v5 0/3] eal: deprecate last use of pthread_t in public API Tyler Retzlaff
2023-01-31 20:30 ` [PATCH v5 1/3] eal: add rte control thread create API Tyler Retzlaff
2023-02-07 13:12 ` David Marchand
2023-02-07 21:41 ` Tyler Retzlaff
2023-01-31 20:30 ` [PATCH v5 2/3] doc: add missing index entry for thread Tyler Retzlaff
2023-02-07 15:39 ` David Marchand
2023-02-07 21:39 ` Tyler Retzlaff
2023-01-31 20:30 ` [PATCH v5 3/3] doc: announce deprecation of thread ctrl create function Tyler Retzlaff
2023-02-07 15:40 ` David Marchand
2023-01-31 20:33 ` [PATCH v5 0/3] eal: deprecate last use of pthread_t in public API Tyler Retzlaff
2023-02-08 21:26 ` [PATCH v6 " Tyler Retzlaff
2023-02-08 21:26 ` [PATCH v6 1/3] eal: add rte thread create control API Tyler Retzlaff
2023-02-24 5:52 ` Li, WeiyuanX
2023-02-24 8:13 ` David Marchand
2023-02-24 19:04 ` Tyler Retzlaff
2023-02-24 20:52 ` Tyler Retzlaff
2023-02-08 21:26 ` [PATCH v6 2/3] doc: add missing index entry for thread Tyler Retzlaff
2023-02-08 21:26 ` [PATCH v6 3/3] doc: announce deprecation of thread ctrl create function Tyler Retzlaff
2023-02-09 8:59 ` Bruce Richardson
2023-02-09 8:51 ` [PATCH v6 0/3] eal: deprecate last use of pthread_t in public API David Marchand
2023-02-09 11:55 ` David Marchand
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=3e6b1bb8-e97d-17b1-207e-ef9d0447480d@lysator.liu.se \
--to=hofors@lysator.liu.se \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=mb@smartsharesystems.com \
--cc=olivier.matz@6wind.com \
--cc=roretzla@linux.microsoft.com \
--cc=stephen@networkplumber.org \
--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).