DPDK patches and discussions
 help / color / mirror / Atom feed
From: Luc Pelletier <lucp.at.work@gmail.com>
To: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Cc: dev <dev@dpdk.org>, Olivier Matz <olivier.matz@6wind.com>,
	david.marchand@redhat.com, ruifeng.wang@arm.com, nd@arm.com
Subject: Re: [dpdk-dev] [RFC] eal: simplify the implementation of rte_ctrl_thread_create
Date: Wed, 25 Aug 2021 14:48:49 -0400
Message-ID: <CAFeRdtCWRofGZa2v5E3GRa=6a2nGjiXHdtMqKBEvvEEzH6gFCg@mail.gmail.com> (raw)
In-Reply-To: <20210730213709.19400-1-honnappa.nagarahalli@arm.com>

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 à 17:37, Honnappa Nagarahalli
<honnappa.nagarahalli@arm.com> a écrit :
<snip>
> +       /* 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.

<snip>
>         ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
>         if (ret != 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.

<snip>
> +       /* Wait for the control thread to initialize successfully */
> +       while (!params->sync)
> +               rte_pause();
> +       ret = params->ret;
>
> -       pthread_barrier_wait(&params->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 à 17:37, Honnappa Nagarahalli
<honnappa.nagarahalli@arm.com> a écrit :
>
> 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 <honnappa.nagarahalli@arm.com>
> ---
>  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..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(&params->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
> -               (void)pthread_barrier_destroy(&params->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(&params->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) {
> +               params->sync = 2;
>                 return NULL;
> +       }
> +
> +       params->sync = 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 =
> -               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(&params->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 (!params->sync)
> +               rte_pause();
> +       ret = params->ret;
>
> -       pthread_barrier_wait(&params->configured);
> -       ctrl_params_free(params);
> +       free(params);
>
> -       if (ret != 0)
> -               /* start_routine has been set to NULL above; */
> -               /* ctrl thread will exit immediately */
> +       if (params->sync != 1)
> +               /* ctrl thread is exiting */
>                 pthread_join(*thread, NULL);
>
>         return -ret;
>
> -fail_with_barrier:
> -       (void)pthread_barrier_destroy(&params->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
>

  parent reply	other threads:[~2021-08-25 18:49 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-30 21:37 Honnappa Nagarahalli
2021-08-02  5:16 ` [dpdk-dev] [RFC v2] " Honnappa Nagarahalli
2021-08-23 10:16   ` Olivier Matz
2021-08-24 20:03     ` Honnappa Nagarahalli
2021-08-24 21:30       ` Stephen Hemminger
2021-08-25  1:26         ` Honnappa Nagarahalli
2021-08-25 15:19         ` Mattias Rönnblom
2021-08-25 15:31           ` Honnappa Nagarahalli
2021-09-01 20:04             ` Mattias Rönnblom
2021-09-01 22:21               ` Honnappa Nagarahalli
2021-08-30 22:30         ` Honnappa Nagarahalli
2021-08-25 15:12   ` Mattias Rönnblom
2021-08-25 15:23     ` Honnappa Nagarahalli
2021-08-25 21:57       ` Stephen Hemminger
2021-08-25 18:48 ` Luc Pelletier [this message]
2021-08-25 19:26   ` [dpdk-dev] [RFC] " Honnappa Nagarahalli
2021-08-30 16:34 ` [dpdk-dev] [PATCH v3 1/2] " Honnappa Nagarahalli
2021-08-30 16:34   ` [dpdk-dev] [PATCH v3 2/2] test/eal: add a test for rte_ctrl_thread_create Honnappa Nagarahalli
2021-08-30 16:41     ` Stephen Hemminger
2021-08-30 16:42       ` Honnappa Nagarahalli
2021-08-30 16:44   ` [dpdk-dev] [PATCH v3 1/2] eal: simplify the implementation of rte_ctrl_thread_create Stephen Hemminger
2021-08-30 22:12     ` Honnappa Nagarahalli
2021-10-13 20:18 ` [dpdk-dev] [PATCH v4 " Honnappa Nagarahalli
2021-10-13 20:18   ` [dpdk-dev] [PATCH v4 2/2] test/eal: add a test for rte_ctrl_thread_create Honnappa Nagarahalli
2021-10-21 15:46   ` [dpdk-dev] [PATCH v4 1/2] eal: simplify the implementation of rte_ctrl_thread_create Olivier Matz
2021-10-21 20:49     ` Honnappa Nagarahalli
2021-10-21 21:32 ` [dpdk-dev] [PATCH v5 " Honnappa Nagarahalli
2021-10-21 21:32   ` [dpdk-dev] [PATCH v5 2/2] test/eal: add a test for rte_ctrl_thread_create Honnappa Nagarahalli
2021-10-22  8:35     ` Olivier Matz
2021-10-25 19:46       ` David Marchand

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='CAFeRdtCWRofGZa2v5E3GRa=6a2nGjiXHdtMqKBEvvEEzH6gFCg@mail.gmail.com' \
    --to=lucp.at.work@gmail.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=honnappa.nagarahalli@arm.com \
    --cc=nd@arm.com \
    --cc=olivier.matz@6wind.com \
    --cc=ruifeng.wang@arm.com \
    /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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git