From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id E2F2FA0C56;
Mon, 23 Aug 2021 12:16:30 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id 6C4E94014D;
Mon, 23 Aug 2021 12:16:30 +0200 (CEST)
Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com
[209.85.128.52]) by mails.dpdk.org (Postfix) with ESMTP id 5A7AE40042
for <dev@dpdk.org>; Mon, 23 Aug 2021 12:16:28 +0200 (CEST)
Received: by mail-wm1-f52.google.com with SMTP id
j17-20020a05600c1c1100b002e754875260so536636wms.4
for <dev@dpdk.org>; Mon, 23 Aug 2021 03:16:28 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google;
h=date:from:to:cc:subject:message-id:references:mime-version
:content-disposition:in-reply-to;
bh=tPOzX3fOM+DnNlS7cUOvcWDPrpVEF6g3iy0fb5AFluc=;
b=Gp4fw64gxnQ3cAkcuuORLN7Gt/iMpw2RKuaGHiJHBlEefegCBeuY3ehuZ/gu2/Y1II
KCD6vM1G/GhpiZNlZv1nAIWtSkW/1goBdABr7IpCUatGa3MwJC7vk5Ba4wV5RloWCOuJ
JqVuwl1TkvLE17tZlwy+xt1OmZs/9ZnBeN85IRNFMuUkib2Co3mfYPvIoQlGRv6GvDqc
keqIPFzKuU3kLmMRHU3x2C2ne3UKqu/OOilKTHBSznAhXuh+nLeKGdcg1eeVVcYoChcD
6eFfpWVdf+EfHqw5rCnEW3JGsgzStFf1kieQPyQ4RGIejtkdmYsa6kOzwEMrObe7vg0Z
J+2g==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:date:from:to:cc:subject:message-id:references
:mime-version:content-disposition:in-reply-to;
bh=tPOzX3fOM+DnNlS7cUOvcWDPrpVEF6g3iy0fb5AFluc=;
b=kdG6+ngSbJuwhXSiu+F8VyleYtnA3sLilxLHqnRCSI+x21FoN2rjkfyN6X85BB69gW
NWUvHlRrwMRcgMpuhU3Q14hpVSBU2EURQyzxU6eTFEcDtSOYA24Alcz4WC4AI3Hz8urL
wch4cElykA19F6zeMCXq4a72yq/9TlOBd53hbXhKlSwGU3Ei9/OKdkNxdc4gcWwHhAA+
Jyu7h7hWosO/thmWSYH7+n+nv6IT6CZQBc3dDw+AE//RcF+2Q8Kq+AvDryXoPaxqIKFJ
IJbEywOmyJ7BwiSX2AhgGXZlhhxp6tQcx50tB2bmurJ5H7FdQ8a3+GvEsuEpBnGVfvNw
ANbQ==
X-Gm-Message-State: AOAM531cEG6f62IJa1l3jLkRhYgxVPebqHr1jcODWgXB7POPRs24sYFr
MIN6GsmeEdRDiEiqyW2//P6DXw==
X-Google-Smtp-Source: ABdhPJxvHQqtueS0toWtFP9GW20zU8JkEcijaz5APRMEkR8TzAOq3XbCbzavKDY6irhE6pADllRGeg==
X-Received: by 2002:a7b:c048:: with SMTP id u8mr15645413wmc.113.1629713787966;
Mon, 23 Aug 2021 03:16:27 -0700 (PDT)
Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25])
by smtp.gmail.com with ESMTPSA id h11sm8972732wrx.9.2021.08.23.03.16.26
(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
Mon, 23 Aug 2021 03:16:27 -0700 (PDT)
Date: Mon, 23 Aug 2021 12:16:26 +0200
From: Olivier Matz <olivier.matz@6wind.com>
To: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Cc: dev@dpdk.org, lucp.at.work@gmail.com, david.marchand@redhat.com,
thomas@monjalon.net, ruifeng.wang@arm.com, nd@arm.com
Message-ID: <YSN1eveZBseV2t4J@platinum>
References: <20210730213709.19400-1-honnappa.nagarahalli@arm.com>
<20210802051652.3611-1-honnappa.nagarahalli@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210802051652.3611-1-honnappa.nagarahalli@arm.com>
Subject: Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of
rte_ctrl_thread_create
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
Hi Honnappa,
On Mon, Aug 02, 2021 at 12:16:52AM -0500, Honnappa Nagarahalli wrote:
> The current described behaviour of rte_ctrl_thread_create is
> rigid which makes the implementation of the function complex.
> The behavior is abstracted to allow for simplified implementation.
I agree that the behavior description should not reference the pthread
functions, however I don't think the current description prevents from
rewriting the code as you do.
I think it would be better to explain in commit log why the proposed
code is simpler than the current one.
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
> v2: Used compiler's C++11 atomic built-ins to access the shared variable.
>
> lib/eal/common/eal_common_thread.c | 65 +++++++++++++-----------------
> lib/eal/include/rte_lcore.h | 8 ++--
> 2 files changed, 32 insertions(+), 41 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index 1a52f42a2b..e3e0bf4bff 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -169,35 +169,35 @@ __rte_thread_uninit(void)
> struct rte_thread_ctrl_params {
> void *(*start_routine)(void *);
> void *arg;
> - pthread_barrier_t configured;
> - unsigned int refcnt;
> + int ret;
> + /* Synchronization variable between the control thread
> + * and the thread calling rte_ctrl_thread_create function.
> + * 0 - Initialized
> + * 1 - Control thread is running successfully
> + * 2 - Control thread encountered an error. 'ret' has the
> + * error code.
> + */
> + unsigned int sync;
what do you think about using an enum or defines?
> };
>
> -static void ctrl_params_free(struct rte_thread_ctrl_params *params)
> -{
> - if (__atomic_sub_fetch(¶ms->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
> - (void)pthread_barrier_destroy(¶ms->configured);
> - free(params);
> - }
> -}
> -
> static void *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 *);
> + void *(*start_routine)(void *) = params->start_routine;
> void *routine_arg = params->arg;
>
> __rte_thread_init(rte_lcore_id(), cpuset);
> -
> - pthread_barrier_wait(¶ms->configured);
> - start_routine = params->start_routine;
> - ctrl_params_free(params);
> -
> - if (start_routine == NULL)
> + params->ret = pthread_setaffinity_np(pthread_self(),
> + sizeof(*cpuset), cpuset);
> + if (params->ret != 0) {
> + __atomic_store_n(¶ms->sync, 2, __ATOMIC_RELEASE);
> return NULL;
> + }
Sorry if the question is stupid (I'm still not familiar with C++11 atomic
built-ins), but do we have the assurance that params->ret is set in memory
before params->sync is set? See below at [1].
> +
> + __atomic_store_n(¶ms->sync, 1, __ATOMIC_RELEASE);
>
> return start_routine(routine_arg);
> }
> @@ -207,9 +207,6 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
> const pthread_attr_t *attr,
> void *(*start_routine)(void *), 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;
> int ret;
>
> @@ -219,15 +216,12 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>
> params->start_routine = start_routine;
> params->arg = arg;
> - params->refcnt = 2;
> -
> - ret = pthread_barrier_init(¶ms->configured, NULL, 2);
> - if (ret != 0)
> - goto fail_no_barrier;
> + params->ret = 0;
> + params->sync = 0;
>
> ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
> if (ret != 0)
> - goto fail_with_barrier;
> + goto thread_create_failed;
>
> if (name != NULL) {
> ret = rte_thread_setname(*thread, name);
> @@ -236,24 +230,21 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
> "Cannot set name for ctrl thread\n");
> }
>
> - ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
> - if (ret != 0)
> - params->start_routine = NULL;
> + /* Wait for the control thread to initialize successfully */
> + while (!__atomic_load_n(¶ms->sync, __ATOMIC_ACQUIRE))
> + rte_pause();
One difference between this implementation and the previous one is this
busy loop. rte_pause() relaxes the cpu, but will not make the calling
thread to sleep and wait for the sync event. So here we can spin a quite
long time until the other thread is scheduled by the OS.
> + ret = params->ret;
[1]
Here, it is expected that when params->ret is seen as set before
param->sync.
>
> - pthread_barrier_wait(¶ms->configured);
> - ctrl_params_free(params);
> + free(params);
>
> - if (ret != 0)
> - /* start_routine has been set to NULL above; */
> - /* ctrl thread will exit immediately */
> + if (__atomic_load_n(¶ms->sync, __ATOMIC_ACQUIRE) != 1)
it suggest == instead of !=, like this:
if (__atomic_load_n(¶ms->sync, __ATOMIC_ACQUIRE) == CTRL_THREAD_ERR)
> + /* ctrl thread is exiting */
> pthread_join(*thread, NULL);
>
> return -ret;
>
> -fail_with_barrier:
> - (void)pthread_barrier_destroy(¶ms->configured);
> +thread_create_failed:
>
> -fail_no_barrier:
> free(params);
>
> return -ret;
> diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> index 1550b75da0..f1cc5e38dc 100644
> --- a/lib/eal/include/rte_lcore.h
> +++ b/lib/eal/include/rte_lcore.h
> @@ -420,10 +420,10 @@ rte_thread_unregister(void);
> /**
> * Create a control thread.
> *
> - * Wrapper to pthread_create(), pthread_setname_np() and
> - * pthread_setaffinity_np(). 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.
> + * 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.
The description is indeed better. Maybe it is the opportunity to
highlight that if the name cannot be set, no error is returned, see
commit 368a91d6bdc8 ("eal: ignore failure of naming a control thread")
> *
> * @param thread
> * Filled with the thread id of the new created thread.
> --
> 2.17.1
>