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 D4DEA41D5D; Fri, 24 Feb 2023 09:14:12 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BBB7840697; Fri, 24 Feb 2023 09:14:12 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id B573540693 for ; Fri, 24 Feb 2023 09:14:10 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1677226450; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=nltIdwBqueVlkg9X6rBvdXHqAfY17/0E5ffxFe5NHyw=; b=WrcSTukxlHFDWjwLczNwPwWCCPcz/tePrzcA3QpuO/n7bR/g6IEAeSklNXcAHsVPBkZI4Z DAVmHJJKTT/l3hbjQ/AdUNMdbEYECEFQGVVTLI5YghMA1lwa2QSSaKrcDPHW6P3uc413kb YWE8YEnEs51gySsmbXrG9JWGkvJruxY= Received: from mail-pl1-f197.google.com (mail-pl1-f197.google.com [209.85.214.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-421-0eShhN9OOo2oFf7UfkaiiA-1; Fri, 24 Feb 2023 03:14:08 -0500 X-MC-Unique: 0eShhN9OOo2oFf7UfkaiiA-1 Received: by mail-pl1-f197.google.com with SMTP id j18-20020a170903025200b00198aa765a9dso6553221plh.6 for ; Fri, 24 Feb 2023 00:14:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=nltIdwBqueVlkg9X6rBvdXHqAfY17/0E5ffxFe5NHyw=; b=UstwJgbP37ZIWhW/qnupcE1hDoUD3jMH6P6moobYYfaJ+IEwk0KCS+OdDu/Jx7Og6w /1mqbwaLkWQSin3l8LutIPpL7u0B44O1XZhhJQ6F6t4/G8Mor7MJgqCLkMuOEq9v7kgc 7HM5HuesKVsYPI5MnHhq+3lgWRwlvy0MAV189hp6snm6HiDU9fzmmaGlU+ZEEgZtTxlJ c/zreX9hEkIjom5+FWV7vCMNMbNQXP68QJ/6NX0vh6Qoow+UkXmDLtAjj+VUZbFhptiU u+VYPbVrSdREMLbCkBrtw7xNcxjSKs7I+q14TZvo5e8YzYY8Pjh5gh1ggjVeegXZVtRi ut2w== X-Gm-Message-State: AO0yUKU/4QutJDgTIO05M3X1gJSkqTWRuh75I8ITjaKDfdlImBXLPQH6 dDr9KGwTHQ7pkFS1baw9F9fm8CU9gLQGHUEXHFl4kFAeRPOo5N/D37X9yuBBcJNA4/bDz3bCCvN gz2BvuRt4Qo/aB+Rs/aE= X-Received: by 2002:a17:903:40ca:b0:19a:b502:4280 with SMTP id t10-20020a17090340ca00b0019ab5024280mr3420811pld.3.1677226447832; Fri, 24 Feb 2023 00:14:07 -0800 (PST) X-Google-Smtp-Source: AK7set/HK+1/zLgdGhJpWfxIbJtKVQu0NpPimnEQLMrc0n4/C9D9asth+uvzkCbCx0GSao1nrl3tGw8VlcV0IrdXxEU= X-Received: by 2002:a17:903:40ca:b0:19a:b502:4280 with SMTP id t10-20020a17090340ca00b0019ab5024280mr3420801pld.3.1677226447456; Fri, 24 Feb 2023 00:14:07 -0800 (PST) MIME-Version: 1.0 References: <1670271868-11364-1-git-send-email-roretzla@linux.microsoft.com> <1675891595-28366-1-git-send-email-roretzla@linux.microsoft.com> <1675891595-28366-2-git-send-email-roretzla@linux.microsoft.com> In-Reply-To: <1675891595-28366-2-git-send-email-roretzla@linux.microsoft.com> From: David Marchand Date: Fri, 24 Feb 2023 09:13:56 +0100 Message-ID: Subject: Re: [PATCH v6 1/3] eal: add rte thread create control API To: Tyler Retzlaff Cc: dev@dpdk.org, thomas@monjalon.net, olivier.matz@6wind.com, stephen@networkplumber.org, mb@smartsharesystems.com, hofors@lysator.liu.se X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Wed, Feb 8, 2023 at 10:26 PM Tyler Retzlaff wrote: > > Add rte_thread_create_control API as a replacement for > rte_ctrl_thread_create to allow deprecation of the use of platform > specific types in DPDK public API. > > Add test from David Marchand to exercise the new API. > > Signed-off-by: Tyler Retzlaff > Acked-by: Morten Br=C3=B8rup > Reviewed-by: Mattias R=C3=B6nnblom > --- > app/test/test_threads.c | 26 ++++++++++++ > lib/eal/common/eal_common_thread.c | 85 ++++++++++++++++++++++++++++++++= ++---- > lib/eal/include/rte_thread.h | 33 +++++++++++++++ > lib/eal/version.map | 1 + > 4 files changed, 137 insertions(+), 8 deletions(-) > > diff --git a/app/test/test_threads.c b/app/test/test_threads.c > index e0f18e4..657ecad 100644 > --- a/app/test/test_threads.c > +++ b/app/test/test_threads.c > @@ -232,6 +232,31 @@ > return 0; > } > > +static int > +test_thread_create_control_join(void) > +{ > + rte_thread_t thread_id; > + rte_thread_t thread_main_id; > + > + thread_id_ready =3D 0; > + RTE_TEST_ASSERT(rte_thread_create_control(&thread_id, "test_contr= ol_threads", > + NULL, thread_main, &thread_main_id) =3D=3D 0, > + "Failed to create thread."); > + > + while (__atomic_load_n(&thread_id_ready, __ATOMIC_ACQUIRE) =3D=3D= 0) > + ; > + > + RTE_TEST_ASSERT(rte_thread_equal(thread_id, thread_main_id) !=3D = 0, > + "Unexpected thread id."); > + > + __atomic_store_n(&thread_id_ready, 2, __ATOMIC_RELEASE); > + > + RTE_TEST_ASSERT(rte_thread_join(thread_id, NULL) =3D=3D 0, > + "Failed to join thread."); > + > + return 0; > +} > + > static struct unit_test_suite threads_test_suite =3D { > .suite_name =3D "threads autotest", > .setup =3D NULL, > @@ -243,6 +268,7 @@ > TEST_CASE(test_thread_priority), > TEST_CASE(test_thread_attributes_affinity), > TEST_CASE(test_thread_attributes_priority), > + TEST_CASE(test_thread_create_control_join), > TEST_CASES_END() > } > }; > diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_comm= on_thread.c > index 3181515..4f83c97 100644 > --- a/lib/eal/common/eal_common_thread.c > +++ b/lib/eal/common/eal_common_thread.c > @@ -232,7 +232,10 @@ enum __rte_ctrl_thread_status { > }; > > struct rte_thread_ctrl_params { > - void *(*start_routine)(void *); > + union { > + void *(*ctrl_start_routine)(void *arg); > + rte_thread_func control_start_routine; > + } u; > void *arg; > int ret; > /* Control thread status. > @@ -241,27 +244,47 @@ 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 =3D > eal_get_internal_configuration(); > rte_cpuset_t *cpuset =3D &internal_conf->ctrl_cpuset; > struct rte_thread_ctrl_params *params =3D arg; > - void *(*start_routine)(void *) =3D params->start_routine; > - void *routine_arg =3D params->arg; > > __rte_thread_init(rte_lcore_id(), cpuset); > params->ret =3D rte_thread_set_affinity_by_id(rte_thread_self(), = cpuset); > if (params->ret !=3D 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 =3D arg; > + void *(*start_routine)(void *) =3D params->u.ctrl_start_routine; > + > + if (ctrl_thread_init(arg) !=3D 0) > + return NULL; > + > + return start_routine(params->arg); Does this patch open a race on params->arg between the newly created thread, and the one that asked for creation? Reading params->arg is not under the ctrl_thread_status syncrhonisation point anymore (which was protecting against free(params)). WDYT? (Note that I doubt this patch has anything to do with the recent bz opened by Intel QE) > +} > + > +static uint32_t control_thread_start(void *arg) > +{ > + struct rte_thread_ctrl_params *params =3D arg; > + rte_thread_func start_routine =3D params->u.control_start_routine= ; > + > + if (ctrl_thread_init(arg) !=3D 0) > + return params->ret; Idem here for params->ret. > + > + return start_routine(params->arg); > } > > int > @@ -277,12 +300,12 @@ static void *ctrl_thread_init(void *arg) > if (!params) > return -ENOMEM; > > - params->start_routine =3D start_routine; > + params->u.ctrl_start_routine =3D start_routine; > params->arg =3D arg; > params->ret =3D 0; > params->ctrl_thread_status =3D CTRL_THREAD_LAUNCHING; > > - ret =3D pthread_create(thread, attr, ctrl_thread_init, (void *)pa= rams); > + ret =3D pthread_create(thread, attr, ctrl_thread_start, (void *)p= arams); > if (ret !=3D 0) { > free(params); > return -ret; > @@ -315,6 +338,52 @@ static void *ctrl_thread_init(void *arg) > } > > int > +rte_thread_create_control(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 =3D malloc(sizeof(*params)); > + if (params =3D=3D NULL) > + return -ENOMEM; > + > + params->u.control_start_routine =3D start_routine; > + params->arg =3D arg; > + params->ret =3D 0; > + params->ctrl_thread_status =3D CTRL_THREAD_LAUNCHING; > + > + ret =3D rte_thread_create(thread, attr, control_thread_start, par= ams); > + if (ret !=3D 0) { > + free(params); > + return -ret; > + } > + > + if (name !=3D NULL) > + rte_thread_set_name(*thread, name); > + > + /* Wait for the control thread to initialize successfully */ > + while ((ctrl_thread_status =3D > + __atomic_load_n(¶ms->ctrl_thread_status, > + __ATOMIC_ACQUIRE)) =3D=3D CTRL_THREAD_LAUNCHING) = { > + rte_delay_us_sleep(1); > + } > + > + /* Check if the control thread encountered an error */ > + if (ctrl_thread_status =3D=3D CTRL_THREAD_ERROR) { > + /* ctrl thread is exiting */ > + rte_thread_join(*thread, NULL); > + } > + > + ret =3D 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 d247930..fae26a7 100644 > --- a/lib/eal/include/rte_thread.h > +++ b/lib/eal/include/rte_thread.h > @@ -98,6 +98,39 @@ int rte_thread_create(rte_thread_t *thread_id, > * @warning > * @b EXPERIMENTAL: this API may change without prior notice. > * > + * 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 EAL threads are then > + * excluded. If setting the name of the thread fails, 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 RTE_MAX_THREAD_NAME_LEN characters including '\0'). > + * @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. > + */ > +__rte_experimental > +int > +rte_thread_create_control(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. > + * > * Waits for the thread identified by 'thread_id' to terminate > * > * @param thread_id > diff --git a/lib/eal/version.map b/lib/eal/version.map > index 6523102..285a0bd 100644 > --- a/lib/eal/version.map > +++ b/lib/eal/version.map > @@ -441,6 +441,7 @@ EXPERIMENTAL { > rte_thread_join; > > # added in 23.03 > + rte_thread_create_control; > rte_thread_set_name; > }; > > -- > 1.8.3.1 > --=20 David Marchand