Re: [lttng-dev] [PATCH lttng-ust] Fix: destructors do not run on probe provider dlclose
- On Feb 21, 2018, at 5:10 PM, Francis Deslauriers
francis.deslauri...@efficios.com wrote:
> Calling dlclose on a probe provider library does not unregister the
> probes from the callsites as the destructors are not executed.
please specify that this is only true for the first shared object loading
the __tracepoints__disable_destructors into the symbol table. Otherwise,
the sentence above is unclear, and we can be led to think that the destructor
is _never_ executed.
>
> The __tracepoints__disable_destructors weak symbol was exposed by probe
please use current tense: "is exposed"
> providers, liblttng-ust.so and liblttng-ust-tracepoint.so libraries. If
> a probe provider was loaded first into the address space, its definition
"is loaded"
> would be binded to the symbol. All the subsequent libraries using the
"is bound"
> symbol would use the existing definition of the symbol. Thus creating a
", thus..."
> a situation where liblttng-ust.so or liblttng-ust-tracepoint.so would
> have a dependency on the probe provider library.
would have a dependency -> depend
>
> This was preventing the dynamic loader from unloading the library as it
was preventing -> prevents
> was still in use by other libraries. Because of this, the execution of
is still
> its destructors and the unregistration of the probes was postponed.
is postponed
>
> To overcome this issue, we no longer expose this symbol in the
> tracepoint.h file to remove the explicit dependency of the probe
> provider on the symbol. We instead use the existing dlopen handle on
> liblttng-ust-tracepoint.so to get an handle on a getter and setter
> functions for this value.
>
> Signed-off-by: Francis Deslauriers
> ---
> include/lttng/tracepoint.h | 31 ++-
> liblttng-ust/tracepoint.c | 26 ++
> 2 files changed, 40 insertions(+), 17 deletions(-)
>
> diff --git a/include/lttng/tracepoint.h b/include/lttng/tracepoint.h
> index 39f2c4d..95f5de9 100644
> --- a/include/lttng/tracepoint.h
> +++ b/include/lttng/tracepoint.h
> @@ -221,26 +221,13 @@ struct lttng_ust_tracepoint_dlopen {
> void (*rcu_read_lock_sym_bp)(void);
> void (*rcu_read_unlock_sym_bp)(void);
> void *(*rcu_dereference_sym_bp)(void *p);
> + void (*tracepoint_set_destructors_disabled)(int is_disabled);
If we expose a "is_disabled" boolean as argument here, it means we want
to allow application to disable and eventually re-enable destructors.
It makes sense, but then we need to keep a reference count within
liblttng-ust-tracepoint.so rather than a simple 1/0 state. Can you
change that ?
This would then be exposed by 3 functions:
Initial state: destructors are enabled, refcount = 1.
void tracepoint_disable_destructors(void)
void tracepoint_enable_destructors(void)
bool tracepoint_get_destructors_state(void)
> + int (*tracepoint_get_destructors_disabled)(void);
> };
Those added fields to:
struct lttng_ust_tracepoint_dlopen tracepoint_dlopen
__attribute__((weak, visibility("hidden")));
mean that if someone has an old .o or .a laying around compiled with the
prior tracepoint_dlopen definition, and links it against a new definition,
there is a mismatch. Arguably, it's limited to within a .so or executable
(module), so typically people recompile, but it won't work if an application
is statically linked against a library: we will have a layout mismatch for
that symbol.
A safer alternative would be to leave this structure unchanged, and have
a new structure that contains those new fields.
>
> extern struct lttng_ust_tracepoint_dlopen tracepoint_dlopen;
> extern struct lttng_ust_tracepoint_dlopen *tracepoint_dlopen_ptr;
>
> -/* Disable tracepoint destructors. */
> -int __tracepoints__disable_destructors __attribute__((weak));
> -
> -/*
> - * Programs that have threads that survive after they exit, and
> - * therefore call library destructors, should disable the tracepoint
> - * destructors by calling tracepoint_disable_destructors(). This will
> - * leak the tracepoint instrumentation library shared object, leaving
> - * its teardown to the operating system process teardown.
> - */
> -static inline void tracepoint_disable_destructors(void)
> -{
> - __tracepoints__disable_destructors = 1;
> -}
> -
> /*
> * These weak symbols, the constructor, and destructor take care of
> * registering only _one_ instance of the tracepoints per shared-ojbect
> @@ -335,7 +322,8 @@ __tracepoints__destroy(void)
> return;
> if (!tracepoint_dlopen_ptr)
> tracepoint_dlopen_ptr = &tracepoint_dlopen;
> - if (!__tracepoints__disable_destructors
> + if (tracepoint_dlopen_ptr->tracepoint_get_destructors_disabled
> + &&
> !tracepoint_dlopen_ptr->tracepoint_get_destructors_disabled()
> && tracepoint_dlopen_ptr->liblttngust_handle
you can move the check for liblttngust_handle as first check in the if().
> &&
[lttng-dev] [PATCH lttng-ust v2] Fix: destructors do not run on probe provider dlclose
Calling dlclose on the probe provider library that first loaded
__tracepoints__disable_destructors in the symbol table does not
unregister the probes from the callsites as the destructors are not
executed.
The __tracepoints__disable_destructors weak symbol is exposed by probe
providers, liblttng-ust.so and liblttng-ust-tracepoint.so libraries. If
a probe provider is loaded first into the address space, its definition
is binded to the symbol. All the subsequent loaded libraries using the
symbol will use the existing definition of the symbol, thus creating a
situation where liblttng-ust.so or liblttng-ust-tracepoint.so depend on
the probe provider library.
This prevents the dynamic loader from unloading the library as it is
still in use by other libraries. Because of this, the execution of its
destructors and the unregistration of the probes is postponed.
To overcome this issue, we no longer expose this symbol in the
tracepoint.h file to remove the explicit dependency of the probe
provider on the symbol. We instead use the existing dlopen handle on
liblttng-ust-tracepoint.so to get handles on getter and setter functions
for this value.
Signed-off-by: Francis Deslauriers
---
include/lttng/tracepoint.h | 82 +++---
liblttng-ust/tracepoint.c | 35
2 files changed, 98 insertions(+), 19 deletions(-)
diff --git a/include/lttng/tracepoint.h b/include/lttng/tracepoint.h
index 39f2c4d..d285347 100644
--- a/include/lttng/tracepoint.h
+++ b/include/lttng/tracepoint.h
@@ -226,21 +226,6 @@ struct lttng_ust_tracepoint_dlopen {
extern struct lttng_ust_tracepoint_dlopen tracepoint_dlopen;
extern struct lttng_ust_tracepoint_dlopen *tracepoint_dlopen_ptr;
-/* Disable tracepoint destructors. */
-int __tracepoints__disable_destructors __attribute__((weak));
-
-/*
- * Programs that have threads that survive after they exit, and
- * therefore call library destructors, should disable the tracepoint
- * destructors by calling tracepoint_disable_destructors(). This will
- * leak the tracepoint instrumentation library shared object, leaving
- * its teardown to the operating system process teardown.
- */
-static inline void tracepoint_disable_destructors(void)
-{
- __tracepoints__disable_destructors = 1;
-}
-
/*
* These weak symbols, the constructor, and destructor take care of
* registering only _one_ instance of the tracepoints per shared-ojbect
@@ -265,6 +250,47 @@ struct lttng_ust_tracepoint_dlopen tracepoint_dlopen
struct lttng_ust_tracepoint_dlopen *tracepoint_dlopen_ptr
__attribute__((weak, visibility("hidden")));
+/*
+ * Tracepoint dynamic linkage handling (callbacks). Hidden visibility: shared
+ * across objects in a module/main executable. The callbacks are used to
+ * control and check if the destructors should be executed.
+ */
+struct lttng_ust_tracepoint_destructors_syms {
+ void (*tracepoint_enable_destructors)(void);
+ void (*tracepoint_disable_destructors)(void);
+ int (*tracepoint_get_destructors_state)(void);
+};
+
+extern struct lttng_ust_tracepoint_destructors_syms
tracepoint_destructors_syms;
+extern struct lttng_ust_tracepoint_destructors_syms
*tracepoint_destructors_syms_ptr;
+
+struct lttng_ust_tracepoint_destructors_syms tracepoint_destructors_syms
+ __attribute__((weak, visibility("hidden")));
+struct lttng_ust_tracepoint_destructors_syms *tracepoint_destructors_syms_ptr
+ __attribute__((weak, visibility("hidden")));
+
+static inline void tracepoint_disable_destructors(void)
+{
+ if (!tracepoint_dlopen_ptr)
+ tracepoint_dlopen_ptr = &tracepoint_dlopen;
+ if (!tracepoint_destructors_syms_ptr)
+ tracepoint_destructors_syms_ptr = &tracepoint_destructors_syms;
+ if (tracepoint_dlopen_ptr->liblttngust_handle
+ &&
tracepoint_destructors_syms_ptr->tracepoint_disable_destructors)
+
tracepoint_destructors_syms_ptr->tracepoint_disable_destructors();
+}
+
+static inline void tracepoint_enable_destructors(void)
+{
+ if (!tracepoint_dlopen_ptr)
+ tracepoint_dlopen_ptr = &tracepoint_dlopen;
+ if (!tracepoint_destructors_syms_ptr)
+ tracepoint_destructors_syms_ptr = &tracepoint_destructors_syms;
+ if (tracepoint_dlopen_ptr->liblttngust_handle
+ &&
tracepoint_destructors_syms_ptr->tracepoint_enable_destructors)
+
tracepoint_destructors_syms_ptr->tracepoint_enable_destructors();
+}
+
#ifndef _LGPL_SOURCE
static inline void lttng_ust_notrace
__tracepoint__init_urcu_sym(void);
@@ -335,8 +361,11 @@ __tracepoints__destroy(void)
return;
if (!tracepoint_dlopen_ptr)
tracepoint_dlopen_ptr = &tracepoint_dlopen;
- if (!__tracepoints__disable_destructors
- && tracepoint_dlopen_ptr->liblttngust_handle
+ if (!tracepoint_destructors_syms_ptr)
+ tracepoint_dest
[lttng-dev] [RFC PATCH lttng-modules 1/2] Create a memory pool for temporary tracepoint probes storage
This memory pool is created when the lttng-tracer module is loaded. It
allocates 4 buffers of 4k on each CPU. These buffers are designed to
allow tracepoint probes to temporarily store data that does not fit on
the stack (during the code_pre and code_post phases). The memory is
freed when the lttng-tracer module is unloaded.
This removes the need for dynamic allocation during the execution of
tracepoint probes, which does not behave well on PREEMPT_RT kernel, even
if the GFP_ATOMIC and GFP_NOWAIT flags are set.
Signed-off-by: Julien Desfossez
---
Makefile | 3 +-
lttng-abi.c | 9 ++
probes/lttng-tracepoint-event-impl.h | 1 +
tp-mempool.c | 175 +++
tp-mempool.h | 48 ++
5 files changed, 235 insertions(+), 1 deletion(-)
create mode 100644 tp-mempool.c
create mode 100644 tp-mempool.h
diff --git a/Makefile b/Makefile
index 2cd2df0..78f6661 100644
--- a/Makefile
+++ b/Makefile
@@ -59,7 +59,8 @@ ifneq ($(KERNELRELEASE),)
lttng-filter.o lttng-filter-interpreter.o \
lttng-filter-specialize.o \
lttng-filter-validator.o \
- probes/lttng-probe-user.o
+ probes/lttng-probe-user.o \
+ tp-mempool.o
ifneq ($(CONFIG_HAVE_SYSCALL_TRACEPOINTS),)
lttng-tracer-objs += lttng-syscalls.o
diff --git a/lttng-abi.c b/lttng-abi.c
index d202b72..9c29612 100644
--- a/lttng-abi.c
+++ b/lttng-abi.c
@@ -56,6 +56,7 @@
#include
#include
#include
+#include
#include
/*
@@ -1771,6 +1772,12 @@ int __init lttng_abi_init(void)
wrapper_vmalloc_sync_all();
lttng_clock_ref();
+
+ ret = tp_mempool_init();
+ if (ret) {
+ goto error;
+ }
+
lttng_proc_dentry = proc_create_data("lttng", S_IRUSR | S_IWUSR, NULL,
free_list);
+ INIT_LIST_HEAD(&cpu_buf->allocated_list);
+
+ for (i = 0; i < NR_BUF_PER_CPU; i++) {
+ struct tp_buf_entry *entry;
+
+ entry = kzalloc(sizeof(struct tp_buf_entry),
+ GFP_KERNEL);
+ if (!entry) {
+
[lttng-dev] [RFC PATCH lttng-modules 2/2] Use the memory pool instead of kmalloc
Replace the use of kmalloc/kfree in the tracepoint probes that need
dynamic allocation with the tracepoint memory pool alloc/free.
Signed-off-by: Julien Desfossez
---
.../syscalls/headers/syscalls_pointers_override.h | 33 +-
1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/instrumentation/syscalls/headers/syscalls_pointers_override.h
b/instrumentation/syscalls/headers/syscalls_pointers_override.h
index 184f3a9..01576d9 100644
--- a/instrumentation/syscalls/headers/syscalls_pointers_override.h
+++ b/instrumentation/syscalls/headers/syscalls_pointers_override.h
@@ -100,9 +100,8 @@ SC_LTTNG_TRACEPOINT_EVENT(pipe2,
}
\
\
if (inp) {
\
- tp_locvar->fds_in = kmalloc(
\
- tp_locvar->nr_ulong * sizeof(unsigned
long),\
- GFP_ATOMIC | GFP_NOWAIT);
\
+ tp_locvar->fds_in = tp_mempool_alloc(
\
+ tp_locvar->nr_ulong * sizeof(unsigned
long)); \
if (!tp_locvar->fds_in)
\
goto error;
\
\
@@ -113,9 +112,8 @@ SC_LTTNG_TRACEPOINT_EVENT(pipe2,
goto error;
\
}
\
if (outp) {
\
- tp_locvar->fds_out = kmalloc(
\
- tp_locvar->nr_ulong * sizeof(unsigned
long),\
- GFP_ATOMIC | GFP_NOWAIT);
\
+ tp_locvar->fds_out = tp_mempool_alloc(
\
+ tp_locvar->nr_ulong * sizeof(unsigned
long)); \
if (!tp_locvar->fds_out)
\
goto error;
\
\
@@ -126,9 +124,8 @@ SC_LTTNG_TRACEPOINT_EVENT(pipe2,
goto error;
\
}
\
if (exp) {
\
- tp_locvar->fds_ex = kmalloc(
\
- tp_locvar->nr_ulong * sizeof(unsigned
long),\
- GFP_ATOMIC | GFP_NOWAIT);
\
+ tp_locvar->fds_ex = tp_mempool_alloc(
\
+ tp_locvar->nr_ulong * sizeof(unsigned
long)); \
if (!tp_locvar->fds_ex)
\
goto error;
\
\
@@ -221,9 +218,9 @@ end:; /* Label at end of compound statement. */
\
)
#define LTTNG_SYSCALL_SELECT_code_post \
- kfree(tp_locvar->fds_in); \
- kfree(tp_locvar->fds_out); \
- kfree(tp_locvar->fds_ex);
+ tp_mempool_free(tp_locvar->fds_in); \
+ tp_mempool_free(tp_locvar->fds_out);\
+ tp_mempool_free(tp_locvar->fds_ex);
#if defined(CONFIG_X86_32) || defined(CONFIG_X86_64) || defined(CONFIG_ARM)
#define OVERRIDE_32_select
@@ -413,8 +410,7 @@ static struct lttng_type lttng_pollfd_elem = {
{
\
int err;
\
\
- tp_locvar->fds = kmalloc(tp_locvar->alloc_fds,
\
- GFP_ATOMIC | GFP_NOWAIT);
\
+ tp_locvar->fds = tp_mempool_alloc(tp_locvar->alloc_fds);
\
if (!tp_locvar->fds)
