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 287CBA0C41; Wed, 25 Aug 2021 20:49:03 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B88804013F; Wed, 25 Aug 2021 20:49:02 +0200 (CEST) Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com [209.85.167.52]) by mails.dpdk.org (Postfix) with ESMTP id 380934003D for ; Wed, 25 Aug 2021 20:49:01 +0200 (CEST) Received: by mail-lf1-f52.google.com with SMTP id o10so933378lfr.11 for ; Wed, 25 Aug 2021 11:49:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=5sWL0UfLU2nS6kff67oXxR53WwOhGjIf96ASYlaZJrA=; b=mpbDzmxmVUJDXF/hiUExrj9EJLF2lDQ7s2bDX4Ufw/jE9MD6B+YecfgbkLKvBkmGJx GMpQvIIBnDOJ55lmLq9t/4hXWE9X+J4yY8t98uIv/ChHVyY97oOhbi3oHvxFRJBLQiFn FVz3mcZh4cbrMPmy5lF2xdb50oj+0Zm9SBpT/tmELAEdJ9lyWiSJuaRc5bYjSRGu3NWu fv6w0RgxaZrr2UKwynYkLB7/Vi1+ZK9ByTJC0eJX1LWzGKyk5gknoo3Adai3EN358Cco BLL0xAXXGVbd2/SCtRJZeBtAvkhl1ifEGtAGMPn+xOKGp9zirsEsNfJjTGj+CnfFXg3e WMow== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=5sWL0UfLU2nS6kff67oXxR53WwOhGjIf96ASYlaZJrA=; b=b0UAT0wgJ9+DnTq2mpCtc0wzO+z/k3vlFdfwzi7jrfhIckOWMzmGCVVCldVzILcxdF lW6ffHLv/YuhifSODDDBGrk6fNeK8KUO3xyVrbIKw/DV+sfl+NDQzniHB0Fm2YKvTOEb 04iIarLu/jz18Ot0VEqig8SZjjRiB5C3GM0MDj1bHPxClhqHcSg0LHwx4UFmaqetA+ON TJWc5iZ64OrTOMjrBvjz0vQshu5OHjMTkAoL43a8/a3U6dGV/MsYTk9M2lBpBCoNIHPp BPYzwRXWMWdZszQkdXK6e8jRcosSE+zgc/j9d+rNPwZEQcv1o2hHYy4RCTmXcIHOnBJr 7AVw== X-Gm-Message-State: AOAM532FVGgqbYC+Ehbul9EqNeGKxx4BvvmM06ueU4Bz3d/e1C19/cYC nq9gc8DXPScv8OsyKj5u7x/a77T0DCc7ZRPUodA= X-Google-Smtp-Source: ABdhPJym79vJB+HrB39qDAt6Pes5y6Cvvvu/JQLEL7yZGA9HmsmM/K4Bon4YEqlhzcATCwQRZjOCpWHPhui3Ct7KK+A= X-Received: by 2002:a05:6512:3e26:: with SMTP id i38mr34977529lfv.29.1629917340692; Wed, 25 Aug 2021 11:49:00 -0700 (PDT) MIME-Version: 1.0 References: <20210730213709.19400-1-honnappa.nagarahalli@arm.com> In-Reply-To: <20210730213709.19400-1-honnappa.nagarahalli@arm.com> From: Luc Pelletier Date: Wed, 25 Aug 2021 14:48:49 -0400 Message-ID: To: Honnappa Nagarahalli Cc: dev , Olivier Matz , david.marchand@redhat.com, ruifeng.wang@arm.com, nd@arm.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [RFC] 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Honnappa, (Sorry if you get this message twice, I forgot to reply all the first time) Sorry for the late reply. I was also away. I have only made one small contribution to DPDK so I'll defer to others to decide whether this patch should be accepted. When I submitted my patch, I got the feeling that busy loops were somewhat frowned upon but it might be a good solution here. Le ven. 30 juil. 2021 =C3=A0 17:37, Honnappa Nagarahalli a =C3=A9crit : > + /* 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; This is highly subjective but the name of this variable doesn't clearly indicate that once it is set, the control thread is effectively done with the rte_thread_ctrl_params. The downside that I can see (as opposed to the previous reference counting approach) is that anyone making further changes would have to be cognizant of that fact. Otherwise, any new code that accesses "sync" in the control thread, after "sync" has been set, would create a race. > ret =3D pthread_create(thread, attr, ctrl_thread_init, (void *)pa= rams); > if (ret !=3D 0) > - goto fail_with_barrier; > + goto thread_create_failed; This is also highly subjective but since the goto label is only used here, you could completely get rid of the goto and simply free and return here. > + /* Wait for the control thread to initialize successfully */ > + while (!params->sync) > + rte_pause(); > + ret =3D params->ret; > > - pthread_barrier_wait(¶ms->configured); > - ctrl_params_free(params); > + free(params); Maybe I'm missing something but it seems like you're accessing params->sync after this line (use after free). I assume you meant to add this line after the subsequent if block? Le ven. 30 juil. 2021 =C3=A0 17:37, Honnappa Nagarahalli a =C3=A9crit : > > 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. > > Signed-off-by: Honnappa Nagarahalli > --- > 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_comm= on_thread.c > index 1a52f42a2b..86cacd840c 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; > }; > > -static void ctrl_params_free(struct rte_thread_ctrl_params *params) > -{ > - if (__atomic_sub_fetch(¶ms->refcnt, 1, __ATOMIC_ACQ_REL) =3D= =3D 0) { > - (void)pthread_barrier_destroy(¶ms->configured); > - free(params); > - } > -} > - > static void *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 *); > + void *(*start_routine)(void *) =3D params->start_routine; > void *routine_arg =3D params->arg; > > __rte_thread_init(rte_lcore_id(), cpuset); > - > - pthread_barrier_wait(¶ms->configured); > - start_routine =3D params->start_routine; > - ctrl_params_free(params); > - > - if (start_routine =3D=3D NULL) > + params->ret =3D pthread_setaffinity_np(pthread_self(), > + sizeof(*cpuset), cpuset); > + if (params->ret !=3D 0) { > + params->sync =3D 2; > return NULL; > + } > + > + params->sync =3D 1; > > 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 =3D > - eal_get_internal_configuration(); > - rte_cpuset_t *cpuset =3D &internal_conf->ctrl_cpuset; > struct rte_thread_ctrl_params *params; > int ret; > > @@ -219,15 +216,12 @@ rte_ctrl_thread_create(pthread_t *thread, const cha= r *name, > > params->start_routine =3D start_routine; > params->arg =3D arg; > - params->refcnt =3D 2; > - > - ret =3D pthread_barrier_init(¶ms->configured, NULL, 2); > - if (ret !=3D 0) > - goto fail_no_barrier; > + params->ret =3D 0; > + params->sync =3D 0; > > ret =3D pthread_create(thread, attr, ctrl_thread_init, (void *)pa= rams); > if (ret !=3D 0) > - goto fail_with_barrier; > + goto thread_create_failed; > > if (name !=3D NULL) { > ret =3D rte_thread_setname(*thread, name); > @@ -236,24 +230,21 @@ rte_ctrl_thread_create(pthread_t *thread, const cha= r *name, > "Cannot set name for ctrl thread\n"); > } > > - ret =3D pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset); > - if (ret !=3D 0) > - params->start_routine =3D NULL; > + /* Wait for the control thread to initialize successfully */ > + while (!params->sync) > + rte_pause(); > + ret =3D params->ret; > > - pthread_barrier_wait(¶ms->configured); > - ctrl_params_free(params); > + free(params); > > - if (ret !=3D 0) > - /* start_routine has been set to NULL above; */ > - /* ctrl thread will exit immediately */ > + if (params->sync !=3D 1) > + /* 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. > * > * @param thread > * Filled with the thread id of the new created thread. > -- > 2.17.1 >