DPDK patches and discussions
 help / color / mirror / Atom feed
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
Subject: Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create
Date: Mon, 23 Aug 2021 12:16:26 +0200
Message-ID: <YSN1eveZBseV2t4J@platinum> (raw)
In-Reply-To: <20210802051652.3611-1-honnappa.nagarahalli@arm.com>

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(&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) {
> +		__atomic_store_n(&params->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(&params->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(&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 (!__atomic_load_n(&params->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(&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 (__atomic_load_n(&params->sync, __ATOMIC_ACQUIRE) != 1)

it suggest == instead of !=, like this:

  if (__atomic_load_n(&params->sync, __ATOMIC_ACQUIRE) == CTRL_THREAD_ERR)


> +		/* 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.

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
> 

  reply	other threads:[~2021-08-23 10:16 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-30 21:37 [dpdk-dev] [RFC] " Honnappa Nagarahalli
2021-08-02  5:16 ` [dpdk-dev] [RFC v2] " Honnappa Nagarahalli
2021-08-23 10:16   ` Olivier Matz [this message]
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 ` [dpdk-dev] [RFC] " Luc Pelletier
2021-08-25 19:26   ` 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=YSN1eveZBseV2t4J@platinum \
    --to=olivier.matz@6wind.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=honnappa.nagarahalli@arm.com \
    --cc=lucp.at.work@gmail.com \
    --cc=nd@arm.com \
    --cc=ruifeng.wang@arm.com \
    --cc=thomas@monjalon.net \
    /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