From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 07295A00C2; Thu, 8 Dec 2022 22:59:19 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B12F440E28; Thu, 8 Dec 2022 22:59:18 +0100 (CET) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 22D5C40685 for ; Thu, 8 Dec 2022 22:59:17 +0100 (CET) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id AB17213FB5 for ; Thu, 8 Dec 2022 22:59:16 +0100 (CET) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id A9C3F14030; Thu, 8 Dec 2022 22:59:16 +0100 (CET) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.6 required=5.0 tests=ALL_TRUSTED, AWL, NICE_REPLY_A autolearn=disabled version=3.4.6 X-Spam-Score: -1.6 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 0382114314; Thu, 8 Dec 2022 22:59:14 +0100 (CET) Message-ID: <3e6b1bb8-e97d-17b1-207e-ef9d0447480d@lysator.liu.se> Date: Thu, 8 Dec 2022 22:59:13 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH v2 1/3] eal: add rte control thread create API To: Tyler Retzlaff Cc: dev@dpdk.org, thomas@monjalon.net, david.marchand@redhat.com, stephen@networkplumber.org, olivier.matz@6wind.com, mb@smartsharesystems.com References: <1670271868-11364-1-git-send-email-roretzla@linux.microsoft.com> <1670347706-23802-1-git-send-email-roretzla@linux.microsoft.com> <1670347706-23802-2-git-send-email-roretzla@linux.microsoft.com> <5b080d72-f7a8-232d-86a1-9164a005fcfb@lysator.liu.se> <20221207163839.GB14713@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= In-Reply-To: <20221207163839.GB14713@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 >>> --- >>> 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 {