[v4,4/9] eal: introduce thread uninit helper
Checks
Commit Message
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(-)
Comments
On Fri, Jun 26, 2020 at 8:18 PM David Marchand
<david.marchand@redhat.com> 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,
> +/**
> + * 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?
>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Friday, June 26, 2020 8:18 PM
>To: dev@dpdk.org
>Cc: jerinjacobk@gmail.com; bruce.richardson@intel.com; mdr@ashroe.eu;
>thomas@monjalon.net; arybchenko@solarflare.com; ktraynor@redhat.com;
>ian.stokes@intel.com; i.maximets@ovn.org; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>; Sunil Kumar Kori <skori@marvell.com>; Neil Horman
><nhorman@tuxdriver.com>; Harini Ramakrishnan
><harini.ramakrishnan@microsoft.com>; Omar Cardona
><ocardona@microsoft.com>; Pallavi Kadam <pallavi.kadam@intel.com>;
>Ranjit Menon <ranjit.menon@intel.com>
>Subject: [EXT] [PATCH v4 4/9] eal: introduce thread uninit helper
>
>External Email
>
>----------------------------------------------------------------------
>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_thread.c
>b/lib/librte_eal/common/eal_common_thread.c
>index afb30236c5..3b30cc99d9 100644
>--- a/lib/librte_eal/common/eal_common_thread.c
>+++ b/lib/librte_eal/common/eal_common_thread.c
>@@ -20,6 +20,7 @@
> #include "eal_internal_cfg.h"
> #include "eal_private.h"
> #include "eal_thread.h"
>+#include "eal_trace.h"
>
> RTE_DEFINE_PER_LCORE(unsigned int, _lcore_id) = LCORE_ID_ANY;
>RTE_DEFINE_PER_LCORE(int, _thread_id) = -1; @@ -161,6 +162,14 @@
>rte_thread_init(unsigned int lcore_id, rte_cpuset_t *cpuset)
> __rte_trace_mem_per_thread_alloc();
> }
>
>+void
>+rte_thread_uninit(void)
>+{
Need to check whether trace is enabled or not similar to trace_mem_free().
>+ __rte_trace_mem_per_thread_free();
>+
>+ RTE_PER_LCORE(_lcore_id) = LCORE_ID_ANY; }
>+
> struct rte_thread_ctrl_params {
> void *(*start_routine)(void *);
> void *arg;
[snipped]
>2.23.0
On Fri, Jun 26, 2020 at 5:00 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 8:18 PM David Marchand
> <david.marchand@redhat.com> 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,
>
> > +/**
> > + * 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.
On Mon, Jun 29, 2020 at 10:59 AM Sunil Kumar Kori <skori@marvell.com> wrote:
> >diff --git a/lib/librte_eal/common/eal_common_thread.c
> >b/lib/librte_eal/common/eal_common_thread.c
> >index afb30236c5..3b30cc99d9 100644
> >--- a/lib/librte_eal/common/eal_common_thread.c
> >+++ b/lib/librte_eal/common/eal_common_thread.c
> >@@ -20,6 +20,7 @@
> > #include "eal_internal_cfg.h"
> > #include "eal_private.h"
> > #include "eal_thread.h"
> >+#include "eal_trace.h"
> >
> > RTE_DEFINE_PER_LCORE(unsigned int, _lcore_id) = LCORE_ID_ANY;
> >RTE_DEFINE_PER_LCORE(int, _thread_id) = -1; @@ -161,6 +162,14 @@
> >rte_thread_init(unsigned int lcore_id, rte_cpuset_t *cpuset)
> > __rte_trace_mem_per_thread_alloc();
> > }
> >
> >+void
> >+rte_thread_uninit(void)
> >+{
>
> Need to check whether trace is enabled or not similar to trace_mem_free().
The internal trace api abstracts this.
It should be in the trace code itself, in the same way it is done for
allocating the trace buffer.
But is an additional check needed?
The check on the trace buffer being initialised in
__rte_trace_mem_per_thread_free should be enough.
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?
On Tue, Jun 30, 2020 at 11:42 AM Olivier Matz <olivier.matz@6wind.com> 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.
@@ -20,6 +20,7 @@
#include "eal_internal_cfg.h"
#include "eal_private.h"
#include "eal_thread.h"
+#include "eal_trace.h"
RTE_DEFINE_PER_LCORE(unsigned int, _lcore_id) = LCORE_ID_ANY;
RTE_DEFINE_PER_LCORE(int, _thread_id) = -1;
@@ -161,6 +162,14 @@ rte_thread_init(unsigned int lcore_id, rte_cpuset_t *cpuset)
__rte_trace_mem_per_thread_alloc();
}
+void
+rte_thread_uninit(void)
+{
+ __rte_trace_mem_per_thread_free();
+
+ RTE_PER_LCORE(_lcore_id) = LCORE_ID_ANY;
+}
+
struct rte_thread_ctrl_params {
void *(*start_routine)(void *);
void *arg;
@@ -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);
+ rte_spinlock_lock(&trace->lock);
+ for (count = 0; count < trace->nb_trace_mem_list; count++) {
+ if (trace->lcore_meta[count].mem == header)
+ break;
+ }
+ if (count != trace->nb_trace_mem_list) {
+ struct thread_mem_meta *meta = &trace->lcore_meta[count];
+
+ trace_mem_per_thread_free_unlocked(meta);
+ if (count != trace->nb_trace_mem_list - 1) {
+ memmove(meta, meta + 1,
+ sizeof(*meta) *
+ (trace->nb_trace_mem_list - count - 1));
+ }
+ trace->nb_trace_mem_list--;
+ }
+ rte_spinlock_unlock(&trace->lock);
+}
+
void
-trace_mem_per_thread_free(void)
+trace_mem_free(void)
{
struct trace *trace = trace_obj_get();
uint32_t count;
- void *mem;
if (!rte_trace_is_enabled())
return;
rte_spinlock_lock(&trace->lock);
for (count = 0; count < trace->nb_trace_mem_list; count++) {
- mem = trace->lcore_meta[count].mem;
- if (trace->lcore_meta[count].area == TRACE_AREA_HUGEPAGE)
- eal_free_no_trace(mem);
- else if (trace->lcore_meta[count].area == TRACE_AREA_HEAP)
- free(mem);
+ trace_mem_per_thread_free_unlocked(&trace->lcore_meta[count]);
}
+ trace->nb_trace_mem_list = 0;
rte_spinlock_unlock(&trace->lock);
}
@@ -25,6 +25,11 @@ __rte_noreturn void *eal_thread_loop(void *arg);
*/
void rte_thread_init(unsigned int lcore_id, rte_cpuset_t *cpuset);
+/**
+ * Uninitialize per-lcore info for current thread.
+ */
+void rte_thread_uninit(void);
+
/**
* Get the NUMA socket id from cpu id.
* This function is private to EAL.
@@ -106,7 +106,7 @@ int trace_metadata_create(void);
void trace_metadata_destroy(void);
int trace_mkdir(void);
int trace_epoch_time_save(void);
-void trace_mem_per_thread_free(void);
+void trace_mem_free(void);
/* EAL interface */
int eal_trace_init(void);
@@ -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);
+
/**
* @internal
*
@@ -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;
};
INTERNAL {
@@ -255,6 +255,11 @@ __rte_trace_mem_per_thread_alloc(void)
{
}
+void
+__rte_trace_mem_per_thread_free(void)
+{
+}
+
void
__rte_trace_point_emit_field(size_t sz, const char *field,
const char *type)