Re: [dpdk-dev] [PATCH v3 1/2] service: add API to retrieve service core active

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> Subject: [PATCH v3 1/2] service: add API to retrieve service core active
> 
> This commit adds a new experimental API which allows the user to retrieve
> the active state of an lcore. Knowing when the service lcore is completed its
> polling loop can be useful to applications to avoid race conditions when e.g.
> finalizing statistics.
> 
> The service thread itself now has a variable to indicate if its thread is active.
> When zero the service thread has completed its service, and has returned
> from the service_runner_func() function.
> 
> Suggested-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>

Over all looks good, few nits inline.
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

> 
> ---
> 
> v3:
> - Change service lcore stores to SEQ_CST (Honnappa, David)
> - Change control thread load to ACQ (Honnappa, David)
> - Comment reasons for SEQ_CST/ACQ (Honnappa, David)
> - Add comments to Doxygen for _stop() and _lcore_active() (Honnappa, David)
> - Add Phil's review tag from ML
> ---
>  lib/librte_eal/common/rte_service.c  | 23 ++++++++++++++++++++++-
> lib/librte_eal/include/rte_service.h | 20 +++++++++++++++++++-
>  lib/librte_eal/rte_eal_version.map   |  1 +
>  3 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/common/rte_service.c
> b/lib/librte_eal/common/rte_service.c
> index 6a0e0ff65..b0cf7c4ab 100644
> --- a/lib/librte_eal/common/rte_service.c
> +++ b/lib/librte_eal/common/rte_service.c
> @@ -65,6 +65,7 @@ struct core_state {
>  	/* map of services IDs are run on this core */
>  	uint64_t service_mask;
>  	uint8_t runstate; /* running or stopped */
> +	uint8_t thread_active; /* indicates when thread is in service_run() */
>  	uint8_t is_service_core; /* set if core is currently a service core */
>  	uint8_t service_active_on_lcore[RTE_SERVICE_NUM_MAX];
>  	uint64_t loops;
> @@ -422,7 +423,7 @@ rte_service_may_be_active(uint32_t id)
>  		return -EINVAL;
> 
>  	for (i = 0; i < lcore_count; i++) {
> -		if (lcore_states[ids[i]].service_active_on_lcore[id])
> +	if (lcore_states[ids[i]].service_active_on_lcore[id])
nit, I think the <tab> is required here.

>  			return 1;
>  	}
> 
> @@ -457,6 +458,8 @@ service_runner_func(void *arg)
>  	const int lcore = rte_lcore_id();
>  	struct core_state *cs = &lcore_states[lcore];
> 
> +	__atomic_store_n(&cs->thread_active, 1, __ATOMIC_SEQ_CST);
> +
>  	/* runstate act as the guard variable. Use load-acquire
>  	 * memory order here to synchronize with store-release
>  	 * in runstate update functions.
> @@ -475,9 +478,27 @@ service_runner_func(void *arg)
>  		cs->loops++;
>  	}
> 
> +	/* Use SEQ CST memory ordering to avoid any re-ordering around
> +	 * this store, ensuring that once this store is visible, the service
> +	 * lcore thread really is done in service cores code.
> +	 */
> +	__atomic_store_n(&cs->thread_active, 0, __ATOMIC_SEQ_CST);
>  	return 0;
>  }
> 
> +int32_t
> +rte_service_lcore_active(uint32_t lcore) {
> +	if (lcore >= RTE_MAX_LCORE || !lcore_states[lcore].is_service_core)
> +		return -EINVAL;
> +
> +	/* Load thread_active using ACQUIRE to avoid instructions dependent
> on
> +	 * the result being re-ordered before this load completes.
> +	 */
> +	return __atomic_load_n(&lcore_states[lcore].thread_active,
> +			       __ATOMIC_ACQUIRE);
> +}
> +
>  int32_t
>  rte_service_lcore_count(void)
>  {
> diff --git a/lib/librte_eal/include/rte_service.h
> b/lib/librte_eal/include/rte_service.h
> index e2d0a6dd3..84245020d 100644
> --- a/lib/librte_eal/include/rte_service.h
> +++ b/lib/librte_eal/include/rte_service.h
> @@ -249,7 +249,9 @@ int32_t rte_service_lcore_start(uint32_t lcore_id);
>   * Stop a service core.
>   *
>   * Stopping a core makes the core become idle, but remains  assigned as a
> - * service core.
> + * service core. Note that the service lcore thread may not have
> + returned from
> + * the service it is running when this API returns. To check if the
> + service
> + * lcore is still active, the *rte_service_lcore_active* API is added.
nit
Instead of "To check if the service lcore is still active, the *rte_service_lcore_active* API is added.", may be "*rte_service_lcore_active* API can be used to check if the service lcore is still active"?

>   *
>   * @retval 0 Success
>   * @retval -EINVAL Invalid *lcore_id* provided @@ -261,6 +263,22 @@
> int32_t rte_service_lcore_start(uint32_t lcore_id);
>   */
>  int32_t rte_service_lcore_stop(uint32_t lcore_id);
> 
> +/**
> + * Reports if a service lcore is currently running.
nit                                                     ^^^^^^^^^^^^^^^ may be "currently running mapped services"? (matching with the documentation for existing APIs)

> + *
> + * This function returns if the core has finished service cores code,
nit                                                                     ^^^^^^^^^^^^^^^^^^^^^^ may be "completed running assigned services"?

> +and has
> + * returned to EAL control. If *rte_service_lcore_stop* has been called
> +but
> + * the lcore has not returned to EAL yet, it might be required to wait
> +and call
> + * this function again. The amount of time to wait before the core
> +returns
> + * depends on the duration of the services being run.
> + *
> + * @retval 0 Service thread is not active, and has been returned to EAL.
nit,                                    ^^^^^ lcore?
> + * @retval 1 Service thread is in the service core polling loop.
nit, may be "Service lcore is running assigned services."? (matching the terms used in existing APIs)

> + * @retval -EINVAL Invalid *lcore_id* provided.
> + */
> +__rte_experimental
> +int32_t rte_service_lcore_active(uint32_t lcore_id);
> +
>  /**
>   * Adds lcore to the list of service cores.
>   *
> diff --git a/lib/librte_eal/rte_eal_version.map
> b/lib/librte_eal/rte_eal_version.map
> index bf0c17c23..d53d5d5b9 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -401,6 +401,7 @@ EXPERIMENTAL {
>  	rte_lcore_dump;
>  	rte_lcore_iterate;
>  	rte_mp_disable;
> +	rte_service_lcore_active;
>  	rte_thread_register;
>  	rte_thread_unregister;
>  };
> --
> 2.17.1