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)