From 6addfa379ee608b20cfe5e15d135bcb6a9724e90 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Wed, 19 Aug 2015 00:29:52 -0700 Subject: [PATCH] Fix: reference counting of consumer output MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The UST app session has a reference on the consumer output object, but it belongs to the UST session. Implement a refcounting scheme to ensure it is not freed before all users are done using it. Signed-off-by: Mathieu Desnoyers Signed-off-by: Jérémie Galarneau --- src/bin/lttng-sessiond/consumer.c | 54 +++++++++++++++++---------- src/bin/lttng-sessiond/consumer.h | 6 ++- src/bin/lttng-sessiond/main.c | 4 +- src/bin/lttng-sessiond/session.c | 2 +- src/bin/lttng-sessiond/snapshot.c | 2 +- src/bin/lttng-sessiond/trace-kernel.c | 11 +----- src/bin/lttng-sessiond/trace-kernel.h | 7 ---- src/bin/lttng-sessiond/trace-ust.c | 11 +----- src/bin/lttng-sessiond/trace-ust.h | 7 ---- src/bin/lttng-sessiond/ust-app.c | 8 +++- 10 files changed, 53 insertions(+), 59 deletions(-) diff --git a/src/bin/lttng-sessiond/consumer.c b/src/bin/lttng-sessiond/consumer.c index 87d5f3438..41ad46d86 100644 --- a/src/bin/lttng-sessiond/consumer.c +++ b/src/bin/lttng-sessiond/consumer.c @@ -473,6 +473,7 @@ struct consumer_output *consumer_create_output(enum consumer_dst_type type) output->enabled = 1; output->type = type; output->net_seq_index = (uint64_t) -1ULL; + urcu_ref_init(&output->ref); output->socks = lttng_ht_new(0, LTTNG_HT_TYPE_ULONG); @@ -507,11 +508,10 @@ void consumer_destroy_output_sockets(struct consumer_output *obj) * * Should *NOT* be called with RCU read-side lock held. */ -void consumer_destroy_output(struct consumer_output *obj) +static void consumer_release_output(struct urcu_ref *ref) { - if (obj == NULL) { - return; - } + struct consumer_output *obj = + caa_container_of(ref, struct consumer_output, ref); consumer_destroy_output_sockets(obj); @@ -523,6 +523,27 @@ void consumer_destroy_output(struct consumer_output *obj) free(obj); } +/* + * Get the consumer_output object. + */ +void consumer_output_get(struct consumer_output *obj) +{ + urcu_ref_get(&obj->ref); +} + +/* + * Put the consumer_output object. + * + * Should *NOT* be called with RCU read-side lock held. + */ +void consumer_output_put(struct consumer_output *obj) +{ + if (!obj) { + return; + } + urcu_ref_put(&obj->ref, consumer_release_output); +} + /* * Copy consumer output and returned the newly allocated copy. * @@ -531,33 +552,28 @@ void consumer_destroy_output(struct consumer_output *obj) struct consumer_output *consumer_copy_output(struct consumer_output *obj) { int ret; - struct lttng_ht *tmp_ht_ptr; struct consumer_output *output; assert(obj); output = consumer_create_output(obj->type); if (output == NULL) { - goto error; + goto end; } - /* Avoid losing the HT reference after the memcpy() */ - tmp_ht_ptr = output->socks; - - memcpy(output, obj, sizeof(struct consumer_output)); - - /* Putting back the HT pointer and start copying socket(s). */ - output->socks = tmp_ht_ptr; - + output->enabled = obj->enabled; + output->net_seq_index = obj->net_seq_index; + memcpy(output->subdir, obj->subdir, PATH_MAX); + output->snapshot = obj->snapshot; + memcpy(&output->dst, &obj->dst, sizeof(output->dst)); ret = consumer_copy_sockets(output, obj); if (ret < 0) { - goto malloc_error; + goto error_put; } - -error: +end: return output; -malloc_error: - consumer_destroy_output(output); +error_put: + consumer_output_put(output); return NULL; } diff --git a/src/bin/lttng-sessiond/consumer.h b/src/bin/lttng-sessiond/consumer.h index e4845f527..73f113d0a 100644 --- a/src/bin/lttng-sessiond/consumer.h +++ b/src/bin/lttng-sessiond/consumer.h @@ -21,6 +21,7 @@ #include #include #include +#include #include "snapshot.h" @@ -140,6 +141,8 @@ struct consumer_net { * Consumer output object describing where and how to send data. */ struct consumer_output { + struct urcu_ref ref; /* Refcount */ + /* If the consumer is enabled meaning that should be used */ unsigned int enabled; enum consumer_dst_type type; @@ -192,7 +195,8 @@ int consumer_socket_recv(struct consumer_socket *socket, void *msg, struct consumer_output *consumer_create_output(enum consumer_dst_type type); struct consumer_output *consumer_copy_output(struct consumer_output *obj); -void consumer_destroy_output(struct consumer_output *obj); +void consumer_output_get(struct consumer_output *obj); +void consumer_output_put(struct consumer_output *obj); int consumer_set_network_uri(struct consumer_output *obj, struct lttng_uri *uri); int consumer_send_fds(struct consumer_socket *sock, int *fds, size_t nb_fd); diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c index cb3b17cdd..59faae853 100644 --- a/src/bin/lttng-sessiond/main.c +++ b/src/bin/lttng-sessiond/main.c @@ -2717,7 +2717,7 @@ static int copy_session_consumer(int domain, struct ltt_session *session) * domain. */ if (session->kernel_session->consumer) { - consumer_destroy_output(session->kernel_session->consumer); + consumer_output_put(session->kernel_session->consumer); } session->kernel_session->consumer = consumer_copy_output(session->consumer); @@ -2731,7 +2731,7 @@ static int copy_session_consumer(int domain, struct ltt_session *session) case LTTNG_DOMAIN_UST: DBG3("Copying tracing session consumer output in UST session"); if (session->ust_session->consumer) { - consumer_destroy_output(session->ust_session->consumer); + consumer_output_put(session->ust_session->consumer); } session->ust_session->consumer = consumer_copy_output(session->consumer); diff --git a/src/bin/lttng-sessiond/session.c b/src/bin/lttng-sessiond/session.c index b82a330e9..49055fb10 100644 --- a/src/bin/lttng-sessiond/session.c +++ b/src/bin/lttng-sessiond/session.c @@ -198,7 +198,7 @@ int session_destroy(struct ltt_session *session) del_session_list(session); pthread_mutex_destroy(&session->lock); - consumer_destroy_output(session->consumer); + consumer_output_put(session->consumer); snapshot_destroy(&session->snapshot); free(session); diff --git a/src/bin/lttng-sessiond/snapshot.c b/src/bin/lttng-sessiond/snapshot.c index f9366b36e..898f77af9 100644 --- a/src/bin/lttng-sessiond/snapshot.c +++ b/src/bin/lttng-sessiond/snapshot.c @@ -223,7 +223,7 @@ void snapshot_output_destroy(struct snapshot_output *obj) if (obj->consumer) { consumer_output_send_destroy_relayd(obj->consumer); - consumer_destroy_output(obj->consumer); + consumer_output_put(obj->consumer); } free(obj); } diff --git a/src/bin/lttng-sessiond/trace-kernel.c b/src/bin/lttng-sessiond/trace-kernel.c index b86bdfe60..b4dc1b9e8 100644 --- a/src/bin/lttng-sessiond/trace-kernel.c +++ b/src/bin/lttng-sessiond/trace-kernel.c @@ -162,14 +162,6 @@ struct ltt_kernel_session *trace_kernel_create_session(void) goto error; } - /* - * The tmp_consumer stays NULL until a set_consumer_uri command is - * executed. At this point, the consumer should be nullify until an - * enable_consumer command. This assignment is symbolic since we've zmalloc - * the struct. - */ - lks->tmp_consumer = NULL; - return lks; error: @@ -578,8 +570,7 @@ void trace_kernel_destroy_session(struct ltt_kernel_session *session) } /* Wipe consumer output object */ - consumer_destroy_output(session->consumer); - consumer_destroy_output(session->tmp_consumer); + consumer_output_put(session->consumer); free(session); } diff --git a/src/bin/lttng-sessiond/trace-kernel.h b/src/bin/lttng-sessiond/trace-kernel.h index 9d50f4c79..b9bcbfa77 100644 --- a/src/bin/lttng-sessiond/trace-kernel.h +++ b/src/bin/lttng-sessiond/trace-kernel.h @@ -103,14 +103,7 @@ struct ltt_kernel_session { /* UID/GID of the user owning the session */ uid_t uid; gid_t gid; - /* - * Two consumer_output object are needed where one is needed for the - * current output object and the second one is the temporary object used to - * store URI being set by the lttng_set_consumer_uri call. Once - * lttng_enable_consumer is called, the two pointers are swapped. - */ struct consumer_output *consumer; - struct consumer_output *tmp_consumer; /* Tracing session id */ uint64_t id; /* Session is active or not meaning it has been started or stopped. */ diff --git a/src/bin/lttng-sessiond/trace-ust.c b/src/bin/lttng-sessiond/trace-ust.c index 0fca3b33a..9bc0b73d0 100644 --- a/src/bin/lttng-sessiond/trace-ust.c +++ b/src/bin/lttng-sessiond/trace-ust.c @@ -284,14 +284,6 @@ struct ltt_ust_session *trace_ust_create_session(uint64_t session_id) goto error_consumer; } - /* - * The tmp_consumer stays NULL until a set_consumer_uri command is - * executed. At this point, the consumer should be nullify until an - * enable_consumer command. This assignment is symbolic since we've zmalloc - * the struct. - */ - lus->tmp_consumer = NULL; - DBG2("UST trace session create successful"); return lus; @@ -1073,8 +1065,7 @@ void trace_ust_destroy_session(struct ltt_ust_session *session) buffer_reg_uid_destroy(reg, session->consumer); } - consumer_destroy_output(session->consumer); - consumer_destroy_output(session->tmp_consumer); + consumer_output_put(session->consumer); fini_pid_tracker(&session->pid_tracker); diff --git a/src/bin/lttng-sessiond/trace-ust.h b/src/bin/lttng-sessiond/trace-ust.h index b1b200f1d..890a68d95 100644 --- a/src/bin/lttng-sessiond/trace-ust.h +++ b/src/bin/lttng-sessiond/trace-ust.h @@ -106,14 +106,7 @@ struct ltt_ust_session { gid_t gid; /* Is the session active meaning has is been started or stopped. */ unsigned int active:1; - /* - * Two consumer_output object are needed where one is for the current - * output object and the second one is the temporary object used to store - * URI being set by the lttng_set_consumer_uri call. Once - * lttng_enable_consumer is called, the two pointers are swapped. - */ struct consumer_output *consumer; - struct consumer_output *tmp_consumer; /* Sequence number for filters so the tracer knows the ordering. */ uint64_t filter_seq_num; /* This indicates which type of buffer this session is set for. */ diff --git a/src/bin/lttng-sessiond/ust-app.c b/src/bin/lttng-sessiond/ust-app.c index d1cc39bae..01204beb7 100644 --- a/src/bin/lttng-sessiond/ust-app.c +++ b/src/bin/lttng-sessiond/ust-app.c @@ -722,6 +722,8 @@ void delete_ust_app_session(int sock, struct ust_app_session *ua_sess, } pthread_mutex_unlock(&ua_sess->lock); + consumer_output_put(ua_sess->consumer); + call_rcu(&ua_sess->rcu_head, delete_ust_app_session_rcu); } @@ -1685,8 +1687,11 @@ static void shadow_copy_session(struct ust_app_session *ua_sess, ua_sess->egid = usess->gid; ua_sess->buffer_type = usess->buffer_type; ua_sess->bits_per_long = app->bits_per_long; + /* There is only one consumer object per session possible. */ + consumer_output_get(usess->consumer); ua_sess->consumer = usess->consumer; + ua_sess->output_traces = usess->output_traces; ua_sess->live_timer_interval = usess->live_timer_interval; copy_channel_attr_to_ustctl(&ua_sess->metadata_attr, @@ -1773,9 +1778,10 @@ static void shadow_copy_session(struct ust_app_session *ua_sess, lttng_ht_add_unique_str(ua_sess->channels, &ua_chan->node); } + return; error: - return; + consumer_output_put(ua_sess->consumer); } /* -- 2.34.1