Re: [RFC v4 5/6] service: keep per-lcore state in lcore variable

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2024-02-25 16:03, Mattias Rönnblom wrote:
> Replace static array of cache-aligned structs with an lcore variable,
> to slightly benefit code simplicity and performance.
> 
> RFC v4:
>   * Remove strange-looking lcore value lookup potentially containing
>     invalid lcore id. (Morten Brørup)
>   * Replace misplaced tab with space. (Morten Brørup)
> 
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> ---
>   lib/eal/common/rte_service.c | 120 ++++++++++++++++++++---------------
>   1 file changed, 69 insertions(+), 51 deletions(-)
> 
> diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c
> index d959c91459..7fbae704ed 100644
> --- a/lib/eal/common/rte_service.c
> +++ b/lib/eal/common/rte_service.c
> @@ -11,6 +11,7 @@
>   
>   #include <eal_trace_internal.h>
>   #include <rte_lcore.h>
> +#include <rte_lcore_var.h>
>   #include <rte_branch_prediction.h>
>   #include <rte_common.h>
>   #include <rte_cycles.h>
> @@ -75,7 +76,7 @@ struct core_state {
>   
>   static uint32_t rte_service_count;
>   static struct rte_service_spec_impl *rte_services;
> -static struct core_state *lcore_states;
> +static RTE_LCORE_VAR_HANDLE(struct core_state, lcore_states);
>   static uint32_t rte_service_library_initialized;
>   
>   int32_t
> @@ -101,11 +102,12 @@ rte_service_init(void)
>   		goto fail_mem;
>   	}
>   
> -	lcore_states = rte_calloc("rte_service_core_states", RTE_MAX_LCORE,
> -			sizeof(struct core_state), RTE_CACHE_LINE_SIZE);
> -	if (!lcore_states) {
> -		EAL_LOG(ERR, "error allocating core states array");
> -		goto fail_mem;
> +	if (lcore_states == NULL)
> +		RTE_LCORE_VAR_ALLOC(lcore_states);
> +	else {
> +		struct core_state *cs;
> +		RTE_LCORE_VAR_FOREACH_VALUE(cs, lcore_states)
> +			memset(cs, 0, sizeof(struct core_state));
>   	}
>   
>   	int i;
> @@ -122,7 +124,6 @@ rte_service_init(void)
>   	return 0;
>   fail_mem:
>   	rte_free(rte_services);
> -	rte_free(lcore_states);
>   	return -ENOMEM;
>   }
>   
> @@ -136,7 +137,6 @@ rte_service_finalize(void)
>   	rte_eal_mp_wait_lcore();
>   
>   	rte_free(rte_services);
> -	rte_free(lcore_states);
>   
>   	rte_service_library_initialized = 0;
>   }
> @@ -286,7 +286,6 @@ rte_service_component_register(const struct rte_service_spec *spec,
>   int32_t
>   rte_service_component_unregister(uint32_t id)
>   {
> -	uint32_t i;
>   	struct rte_service_spec_impl *s;
>   	SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
>   
> @@ -294,9 +293,10 @@ rte_service_component_unregister(uint32_t id)
>   
>   	s->internal_flags &= ~(SERVICE_F_REGISTERED);
>   
> +	struct core_state *cs;
>   	/* clear the run-bit in all cores */
> -	for (i = 0; i < RTE_MAX_LCORE; i++)
> -		lcore_states[i].service_mask &= ~(UINT64_C(1) << id);
> +	RTE_LCORE_VAR_FOREACH_VALUE(cs, lcore_states)
> +		cs->service_mask &= ~(UINT64_C(1) << id);
>   
>   	memset(&rte_services[id], 0, sizeof(struct rte_service_spec_impl));
>   
> @@ -454,7 +454,10 @@ 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])
> +		struct core_state *cs =
> +			RTE_LCORE_VAR_LCORE_PTR(ids[i], lcore_states);
> +
> +		if (cs->service_active_on_lcore[id])
>   			return 1;
>   	}
>   
> @@ -464,7 +467,7 @@ rte_service_may_be_active(uint32_t id)
>   int32_t
>   rte_service_run_iter_on_app_lcore(uint32_t id, uint32_t serialize_mt_unsafe)
>   {
> -	struct core_state *cs = &lcore_states[rte_lcore_id()];
> +	struct core_state *cs =	RTE_LCORE_VAR_PTR(lcore_states);
>   	struct rte_service_spec_impl *s;
>   
>   	SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
> @@ -486,8 +489,7 @@ service_runner_func(void *arg)
>   {
>   	RTE_SET_USED(arg);
>   	uint8_t i;
> -	const int lcore = rte_lcore_id();
> -	struct core_state *cs = &lcore_states[lcore];
> +	struct core_state *cs = RTE_LCORE_VAR_PTR(lcore_states);
>   
>   	rte_atomic_store_explicit(&cs->thread_active, 1, rte_memory_order_seq_cst);
>   
> @@ -533,13 +535,17 @@ service_runner_func(void *arg)
>   int32_t
>   rte_service_lcore_may_be_active(uint32_t lcore)
>   {
> -	if (lcore >= RTE_MAX_LCORE || !lcore_states[lcore].is_service_core)
> +	struct core_state *cs;
> +
> +	if (lcore >= RTE_MAX_LCORE || !cs->is_service_core)

This doesn't work, since 'cs' is not yet initialized. I'll fix it v5.

<snip>