Re: [dpdk-dev] [PATCH v4 4/9] eal: introduce thread uninit helper

[Olivier Matz <olivier.matz@6wind.com>]

On Fri, Jun 26, 2020 at 04:47:31PM +0200, David Marchand wrote:
> This is a preparation step for dynamically unregistering threads.
> 
> Since we explicitly allocate a per thread trace buffer in
> rte_thread_init, add an internal helper to free this buffer.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Note: I preferred renaming the current internal function to free all
> threads trace buffers (new name trace_mem_free()) and reuse the previous
> name (trace_mem_per_thread_free()) when freeing this buffer for a given
> thread.
> 
> Changes since v2:
> - added missing stub for windows tracing support,
> - moved free symbol to exported (experimental) ABI as a counterpart of
>   the alloc symbol we already had,
> 
> Changes since v1:
> - rebased on master, removed Windows workaround wrt traces support,
> 
> ---
>  lib/librte_eal/common/eal_common_thread.c |  9 ++++
>  lib/librte_eal/common/eal_common_trace.c  | 51 +++++++++++++++++++----
>  lib/librte_eal/common/eal_thread.h        |  5 +++
>  lib/librte_eal/common/eal_trace.h         |  2 +-
>  lib/librte_eal/include/rte_trace_point.h  |  9 ++++
>  lib/librte_eal/rte_eal_version.map        |  3 ++
>  lib/librte_eal/windows/eal.c              |  5 +++
>  7 files changed, 75 insertions(+), 9 deletions(-)

[...]

> diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/common/eal_common_trace.c
> index 875553d7e5..3e620d76ed 100644
> --- a/lib/librte_eal/common/eal_common_trace.c
> +++ b/lib/librte_eal/common/eal_common_trace.c
> @@ -101,7 +101,7 @@ eal_trace_fini(void)
>  {
>  	if (!rte_trace_is_enabled())
>  		return;
> -	trace_mem_per_thread_free();
> +	trace_mem_free();
>  	trace_metadata_destroy();
>  	eal_trace_args_free();
>  }
> @@ -370,24 +370,59 @@ __rte_trace_mem_per_thread_alloc(void)
>  	rte_spinlock_unlock(&trace->lock);
>  }
>  
> +static void
> +trace_mem_per_thread_free_unlocked(struct thread_mem_meta *meta)
> +{
> +	if (meta->area == TRACE_AREA_HUGEPAGE)
> +		eal_free_no_trace(meta->mem);
> +	else if (meta->area == TRACE_AREA_HEAP)
> +		free(meta->mem);
> +}
> +
> +void
> +__rte_trace_mem_per_thread_free(void)
> +{
> +	struct trace *trace = trace_obj_get();
> +	struct __rte_trace_header *header;
> +	uint32_t count;
> +
> +	if (RTE_PER_LCORE(trace_mem) == NULL)
> +		return;
> +
> +	header = RTE_PER_LCORE(trace_mem);

nit:

	header = RTE_PER_LCORE(trace_mem);
	if (header == NULL)
		return;

[...]

> diff --git a/lib/librte_eal/include/rte_trace_point.h b/lib/librte_eal/include/rte_trace_point.h
> index 377c2414aa..686b86fdb1 100644
> --- a/lib/librte_eal/include/rte_trace_point.h
> +++ b/lib/librte_eal/include/rte_trace_point.h
> @@ -230,6 +230,15 @@ __rte_trace_point_fp_is_enabled(void)
>  __rte_experimental
>  void __rte_trace_mem_per_thread_alloc(void);
>  
> +/**
> + * @internal
> + *
> + * Free trace memory buffer per thread.
> + *
> + */
> +__rte_experimental
> +void __rte_trace_mem_per_thread_free(void);

Maybe the doc comment could be reworded a bit
(and the empty line can be removed by the way).

> +
>  /**
>   * @internal
>   *
> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index 0d42d44ce9..5831eea4b0 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -393,6 +393,9 @@ EXPERIMENTAL {
>  	rte_trace_point_lookup;
>  	rte_trace_regexp;
>  	rte_trace_save;
> +
> +	# added in 20.08
> +	__rte_trace_mem_per_thread_free;

Is it really needed to export this function?