<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.wojciec...@partner.samsung.com>
> Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com>
> Reviewed-by: Phil Yang <phil.y...@arm.com>
Over all looks good, few nits inline.
Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@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
