From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
To: Tyler Retzlaff <roretzla@linux.microsoft.com>
Cc: dev@dpdk.org, thomas@monjalon.net, anatoly.burakov@intel.com,
Narcisa Vasile <navasile@microsoft.com>
Subject: Re: [PATCH v2 2/6] eal: add thread lifetime management
Date: Sat, 18 Jun 2022 15:59:08 +0300 [thread overview]
Message-ID: <20220618155908.70e822af@sovereign> (raw)
In-Reply-To: <1655250438-18044-3-git-send-email-roretzla@linux.microsoft.com>
2022-06-14 16:47 (UTC-0700), Tyler Retzlaff:
> On Windows, the function executed by a thread when the thread starts is
> represeneted by a function pointer of type DWORD (*func) (void*).
> On other platforms, the function pointer is a void* (*func) (void*).
>
> Performing a cast between these two types of function pointers to
> uniformize the API on all platforms may result in undefined behavior.
> TO fix this issue, a wrapper that respects the signature required by
> CreateThread() has been created on Windows.
The interface issue is still there:
`rte_thread_func` allows the thread routine to have a pointer-sized result.
`rte_thread_join()` allows to obtain this value as `unsigned long`,
which is pointer-sized on 32-bit platforms
and less than that on 64-bit platforms.
This can lead to issues when developers assume they can return a pointer
and this works on 32 bits, but doesn't work on 64 bits.
If you want to keep API as close to phtread as possible,
the limitation must be at least clearly documented in Doxygen
(`rte_thread_func` is undocumented BTW).
I also suggest using `uint32_t` instead of `unsigned long`
precisely to avoid "is it pointer-sized?" doubts.
(I don't see much benefit in keeping pthread-like signature.
When moving from pthread to rte_thread,
it is as trivial to change the thread routine return type.)
> +int
> +rte_thread_create(rte_thread_t *thread_id,
> + const rte_thread_attr_t *thread_attr,
> + rte_thread_func thread_func, void *args)
> +{
> [...]
> + if (thread_attr->priority ==
> + RTE_THREAD_PRIORITY_REALTIME_CRITICAL) {
> + ret = ENOTSUP;
> + goto cleanup;
> + }
> + ret = thread_map_priority_to_os_value(thread_attr->priority,
> + ¶m.sched_priority, &policy);
> + if (ret != 0)
> + goto cleanup;
thread_map_priority_to_os_value() already checks for unsupported values,
why not let it do this particular check?
> +int
> +rte_thread_join(rte_thread_t thread_id, unsigned long *value_ptr)
> +{
> + int ret = 0;
> + void *res = NULL;
> + void **pres = NULL;
> +
> + if (value_ptr != NULL)
> + pres = &res;
> +
> + ret = pthread_join((pthread_t)thread_id.opaque_id, pres);
> + if (ret != 0) {
> + RTE_LOG(DEBUG, EAL, "pthread_join failed\n");
> + return ret;
> + }
> +
> + if (value_ptr != NULL && *pres != NULL)
> + *value_ptr = *(unsigned long *)(*pres);
> +
> + return 0;
> +}
What makes *pres == NULL special?
> +static DWORD
> +thread_func_wrapper(void *args)
> +{
> + struct thread_routine_ctx *pctx = args;
> + struct thread_routine_ctx ctx;
> +
> + ctx.thread_func = pctx->thread_func;
> + ctx.routine_args = pctx->routine_args;
ctx = *pctx?
> +
> + free(pctx);
> +
> + return (DWORD)(uintptr_t)ctx.thread_func(ctx.routine_args);
> +}
> +
> +int
> +rte_thread_create(rte_thread_t *thread_id,
> + const rte_thread_attr_t *thread_attr,
> + rte_thread_func thread_func, void *args)
> +{
> + int ret = 0;
> + DWORD tid;
> + HANDLE thread_handle = NULL;
> + GROUP_AFFINITY thread_affinity;
> + struct thread_routine_ctx *ctx = NULL;
> +
> + ctx = calloc(1, sizeof(*ctx));
> + if (ctx == NULL) {
> + RTE_LOG(DEBUG, EAL, "Insufficient memory for thread context allocations\n");
> + ret = ENOMEM;
> + goto cleanup;
> + }
> + ctx->routine_args = args;
> + ctx->thread_func = thread_func;
> +
> + thread_handle = CreateThread(NULL, 0, thread_func_wrapper, ctx,
> + CREATE_SUSPENDED, &tid);
> + if (thread_handle == NULL) {
> + ret = thread_log_last_error("CreateThread()");
> + free(ctx);
> + goto cleanup;
Missing `free(ctx)` from other error paths below.
> + }
> + thread_id->opaque_id = tid;
> +
> + if (thread_attr != NULL) {
> + if (CPU_COUNT(&thread_attr->cpuset) > 0) {
> + ret = convert_cpuset_to_affinity(
> + &thread_attr->cpuset,
> + &thread_affinity
> + );
> + if (ret != 0) {
> + RTE_LOG(DEBUG, EAL, "Unable to convert cpuset to thread affinity\n");
> + goto cleanup;
> + }
> +
> + if (!SetThreadGroupAffinity(thread_handle,
> + &thread_affinity, NULL)) {
> + ret = thread_log_last_error("SetThreadGroupAffinity()");
> + goto cleanup;
> + }
> + }
> + ret = rte_thread_set_priority(*thread_id,
> + thread_attr->priority);
> + if (ret != 0) {
> + RTE_LOG(DEBUG, EAL, "Unable to set thread priority\n");
> + goto cleanup;
> + }
> + }
> +
> + if (ResumeThread(thread_handle) == (DWORD)-1) {
> + ret = thread_log_last_error("ResumeThread()");
> + goto cleanup;
> + }
> +
> +cleanup:
> + if (thread_handle != NULL) {
> + CloseHandle(thread_handle);
> + thread_handle = NULL;
> + }
> +
> + return ret;
> +}
next prev parent reply other threads:[~2022-06-18 12:59 UTC|newest]
Thread overview: 69+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-09 13:58 [PATCH 0/6] add thread lifetime and attributes API Tyler Retzlaff
2022-06-09 13:58 ` [PATCH 1/6] eal: add thread attributes Tyler Retzlaff
2022-06-09 13:58 ` [PATCH 2/6] eal: add thread lifetime management Tyler Retzlaff
2022-06-09 13:58 ` [PATCH 3/6] eal: add basic rte thread ID equal API Tyler Retzlaff
2022-06-09 22:24 ` Konstantin Ananyev
2022-06-10 22:48 ` Tyler Retzlaff
2022-06-11 12:25 ` Konstantin Ananyev
2022-06-13 13:39 ` Tyler Retzlaff
2022-06-10 0:40 ` fengchengwen
2022-06-09 13:58 ` [PATCH 4/6] test/threads: add tests for thread lifetime API Tyler Retzlaff
2022-06-09 13:58 ` [PATCH 5/6] test/threads: add tests for thread attributes API Tyler Retzlaff
2022-06-09 13:58 ` [PATCH 6/6] test/threads: remove unit test use of pthread Tyler Retzlaff
2022-06-14 23:47 ` [PATCH v2 0/6] add thread lifetime and attributes API Tyler Retzlaff
2022-06-14 23:47 ` [PATCH v2 1/6] eal: add thread attributes Tyler Retzlaff
2022-06-14 23:47 ` [PATCH v2 2/6] eal: add thread lifetime management Tyler Retzlaff
2022-06-18 12:59 ` Dmitry Kozlyuk [this message]
2022-06-20 17:39 ` Tyler Retzlaff
2022-06-21 16:24 ` Tyler Retzlaff
2022-06-21 18:51 ` Tyler Retzlaff
2022-06-21 19:44 ` Dmitry Kozlyuk
2022-06-21 21:28 ` Tyler Retzlaff
2022-06-21 22:24 ` Dmitry Kozlyuk
2022-06-22 18:21 ` Tyler Retzlaff
2022-06-14 23:47 ` [PATCH v2 3/6] eal: add basic rte thread ID equal API Tyler Retzlaff
2022-06-20 8:34 ` Konstantin Ananyev
2022-06-14 23:47 ` [PATCH v2 4/6] test/threads: add tests for thread lifetime API Tyler Retzlaff
2022-06-14 23:47 ` [PATCH v2 5/6] test/threads: add tests for thread attributes API Tyler Retzlaff
2022-06-14 23:47 ` [PATCH v2 6/6] test/threads: remove unit test use of pthread Tyler Retzlaff
2022-06-22 20:26 ` [PATCH v3 0/6] add thread lifetime and attributes API Tyler Retzlaff
2022-06-22 20:26 ` [PATCH v3 1/6] eal: add thread attributes Tyler Retzlaff
2022-06-22 20:26 ` [PATCH v3 2/6] eal: add thread lifetime management Tyler Retzlaff
2022-06-22 20:26 ` [PATCH v3 3/6] eal: add basic rte thread ID equal API Tyler Retzlaff
2022-06-22 20:26 ` [PATCH v3 4/6] test/threads: add tests for thread lifetime API Tyler Retzlaff
2022-06-22 20:26 ` [PATCH v3 5/6] test/threads: add tests for thread attributes API Tyler Retzlaff
2022-06-22 20:26 ` [PATCH v3 6/6] test/threads: remove unit test use of pthread Tyler Retzlaff
2022-06-27 16:56 ` [PATCH v4 0/6] add thread lifetime and attributes API Tyler Retzlaff
2022-06-27 16:56 ` [PATCH v4 1/6] eal: add thread attributes Tyler Retzlaff
2022-06-27 16:56 ` [PATCH v4 2/6] eal: add thread lifetime management Tyler Retzlaff
2022-06-27 16:56 ` [PATCH v4 3/6] eal: add basic rte thread ID equal API Tyler Retzlaff
2022-06-27 16:56 ` [PATCH v4 4/6] test/threads: add tests for thread lifetime API Tyler Retzlaff
2022-06-27 16:56 ` [PATCH v4 5/6] test/threads: add tests for thread attributes API Tyler Retzlaff
2022-06-27 16:56 ` [PATCH v4 6/6] test/threads: remove unit test use of pthread Tyler Retzlaff
2022-07-31 21:16 ` [PATCH v4 0/6] add thread lifetime and attributes API Dmitry Kozlyuk
2022-09-21 8:15 ` David Marchand
2022-09-29 7:02 ` David Marchand
2022-10-05 16:11 ` Tyler Retzlaff
2022-10-05 16:34 ` Tyler Retzlaff
2022-10-06 6:52 ` David Marchand
2022-10-06 15:14 ` Tyler Retzlaff
2022-10-06 13:36 ` Thomas Monjalon
2022-10-06 15:10 ` Tyler Retzlaff
2022-10-06 15:14 ` Thomas Monjalon
2022-10-06 15:20 ` Tyler Retzlaff
2022-10-06 15:26 ` David Marchand
2022-10-06 15:27 ` Tyler Retzlaff
2022-10-05 17:07 ` [PATCH v5 " Tyler Retzlaff
2022-10-05 17:07 ` [PATCH v5 1/6] eal: add thread attributes Tyler Retzlaff
2022-10-06 8:32 ` David Marchand
2022-10-05 17:07 ` [PATCH v5 2/6] eal: add thread lifetime management Tyler Retzlaff
2023-03-01 8:11 ` David Marchand
2023-03-01 20:34 ` Tyler Retzlaff
2022-10-05 17:07 ` [PATCH v5 3/6] eal: add basic rte thread ID equal API Tyler Retzlaff
2022-10-05 17:07 ` [PATCH v5 4/6] test/threads: add tests for thread lifetime API Tyler Retzlaff
2022-10-06 8:32 ` David Marchand
2022-10-06 15:19 ` Tyler Retzlaff
2022-10-05 17:07 ` [PATCH v5 5/6] test/threads: add tests for thread attributes API Tyler Retzlaff
2022-10-05 17:07 ` [PATCH v5 6/6] test/threads: remove unit test use of pthread Tyler Retzlaff
2022-10-06 19:25 ` [PATCH v5 0/6] add thread lifetime and attributes API David Marchand
2022-10-07 19:20 ` Tyler Retzlaff
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=20220618155908.70e822af@sovereign \
--to=dmitry.kozliuk@gmail.com \
--cc=anatoly.burakov@intel.com \
--cc=dev@dpdk.org \
--cc=navasile@microsoft.com \
--cc=roretzla@linux.microsoft.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).