Re: [dpdk-dev] [PATCH v4 4/9] eal: introduce thread uninit helper
On Tue, Jun 30, 2020 at 11:42 AM Olivier Matz wrote:
> > 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).
Copy/paste.
If we keep this symbol, I'll reword.
>
> > +
> > /**
> > * @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?
>
There is no need for the series.
When an application non-EAL thread (not talking about threads that
dpdk is aware of) calls a tracepoint callback, there is an implicit
call to _alloc.
We end up with a memory leak and the application has no way to fix this.
I left this symbol exported, but this is not documented properly.
--
David Marchand
Re: [dpdk-dev] [PATCH v4 4/9] eal: introduce thread uninit helper
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
> ---
> 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?
Re: [dpdk-dev] [PATCH v4 4/9] eal: introduce thread uninit helper
On Fri, Jun 26, 2020 at 5:00 PM Jerin Jacob wrote: > > On Fri, Jun 26, 2020 at 8:18 PM 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 > > --- > > 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, > > > +/** > > + * Uninitialize per-lcore info for current thread. > > + */ > > +void rte_thread_uninit(void); > > + > > Is it a public API? I guess not as it not adding in .map file. > If it is private API, Is n't it better to change as eal_thread_ like > another private API in eal_thread.h? Before this series, we have: - rte_thread_ public APIs for both EAL and non-EAL threads (declared in rte_eal_interrupts.h and rte_lcore.h), - eal_thread_ internal APIs that apply to EAL threads (declared in eal_thread.h), I guess __rte_thread_ could do the trick and I will move this to eal_private.h. -- David Marchand
Re: [dpdk-dev] [PATCH v4 4/9] eal: introduce thread uninit helper
On Fri, Jun 26, 2020 at 8:18 PM 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 > --- > 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, > +/** > + * Uninitialize per-lcore info for current thread. > + */ > +void rte_thread_uninit(void); > + Is it a public API? I guess not as it not adding in .map file. If it is private API, Is n't it better to change as eal_thread_ like another private API in eal_thread.h?
