[lttng-dev] [PATCH lttng-tools] Fix: sanitize wait queue in the dispatch thread
Mathieu Desnoyers
mathieu.desnoyers at efficios.com
Fri Jun 14 10:55:53 EDT 2013
* David Goulet (dgoulet at efficios.com) wrote:
> This is to avoid memory leaks when the notify socket is never received
> thus cleaning the wait node for command socket that are invalid.
>
> Acked-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> Signed-off-by: David Goulet <dgoulet at efficios.com>
> ---
> src/bin/lttng-sessiond/lttng-sessiond.h | 18 +++++
> src/bin/lttng-sessiond/main.c | 111 ++++++++++++++++++++++++++++---
> src/bin/lttng-sessiond/ust-app.c | 12 ++++
> src/bin/lttng-sessiond/ust-app.h | 6 ++
> 4 files changed, 139 insertions(+), 8 deletions(-)
>
> diff --git a/src/bin/lttng-sessiond/lttng-sessiond.h b/src/bin/lttng-sessiond/lttng-sessiond.h
> index 6090e08..aeb0303 100644
> --- a/src/bin/lttng-sessiond/lttng-sessiond.h
> +++ b/src/bin/lttng-sessiond/lttng-sessiond.h
> @@ -66,6 +66,24 @@ struct ust_cmd_queue {
> };
>
> /*
> + * This is the wait queue containing wait nodes during the application
> + * registration process.
> + */
> +struct ust_reg_wait_queue {
> + unsigned long count;
> + struct cds_list_head head;
> +};
> +
> +/*
> + * Use by the dispatch registration to queue UST command socket to wait for the
> + * notify socket.
> + */
> +struct ust_reg_wait_node {
> + struct ust_app *app;
> + struct cds_list_head head;
> +};
> +
> +/*
> * This pipe is used to inform the thread managing application notify
> * communication that a command is queued and ready to be processed.
> */
> diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
> index 007c722..14bdaf6 100644
> --- a/src/bin/lttng-sessiond/main.c
> +++ b/src/bin/lttng-sessiond/main.c
> @@ -1335,6 +1335,91 @@ error:
> }
>
> /*
> + * Sanitize the wait queue of the dispatch registration thread meaning removing
> + * invalid nodes from it. This is to avoid memory leaks for the case the UST
> + * notify socket is never received.
> + */
should be "static".
the rest looks good.
Thanks,
MAthieu
> +void sanitize_wait_queue(struct ust_reg_wait_queue *wait_queue)
> +{
> + int ret, nb_fd = 0, i;
> + unsigned int fd_added = 0;
> + struct lttng_poll_event events;
> + struct ust_reg_wait_node *wait_node = NULL, *tmp_wait_node;
> +
> + assert(wait_queue);
> +
> + lttng_poll_init(&events);
> +
> + /* Just skip everything for an empty queue. */
> + if (!wait_queue->count) {
> + goto end;
> + }
> +
> + ret = lttng_poll_create(&events, wait_queue->count, LTTNG_CLOEXEC);
> + if (ret < 0) {
> + goto error_create;
> + }
> +
> + cds_list_for_each_entry_safe(wait_node, tmp_wait_node,
> + &wait_queue->head, head) {
> + assert(wait_node->app);
> + ret = lttng_poll_add(&events, wait_node->app->sock,
> + LPOLLHUP | LPOLLERR);
> + if (ret < 0) {
> + goto error;
> + }
> +
> + fd_added = 1;
> + }
> +
> + if (!fd_added) {
> + goto end;
> + }
> +
> + /*
> + * Poll but don't block so we can quickly identify the faulty events and
> + * clean them afterwards from the wait queue.
> + */
> + ret = lttng_poll_wait(&events, 0);
> + if (ret < 0) {
> + goto error;
> + }
> + nb_fd = ret;
> +
> + for (i = 0; i < nb_fd; i++) {
> + /* Get faulty FD. */
> + uint32_t revents = LTTNG_POLL_GETEV(&events, i);
> + int pollfd = LTTNG_POLL_GETFD(&events, i);
> +
> + cds_list_for_each_entry_safe(wait_node, tmp_wait_node,
> + &wait_queue->head, head) {
> + if (pollfd == wait_node->app->sock &&
> + (revents & (LPOLLHUP | LPOLLERR))) {
> + cds_list_del(&wait_node->head);
> + wait_queue->count--;
> + ust_app_destroy(wait_node->app);
> + free(wait_node);
> + break;
> + }
> + }
> + }
> +
> + if (nb_fd > 0) {
> + DBG("Wait queue sanitized, %d node were cleaned up", nb_fd);
> + }
> +
> +end:
> + lttng_poll_clean(&events);
> + return;
> +
> +error:
> + lttng_poll_clean(&events);
> +error_create:
> + ERR("Unable to sanitize wait queue");
> + return;
> +}
> +
> +/*
> * Dispatch request from the registration threads to the application
> * communication thread.
> */
> @@ -1343,16 +1428,16 @@ static void *thread_dispatch_ust_registration(void *data)
> int ret, err = -1;
> struct cds_wfq_node *node;
> struct ust_command *ust_cmd = NULL;
> - struct {
> - struct ust_app *app;
> - struct cds_list_head head;
> - } *wait_node = NULL, *tmp_wait_node;
> + struct ust_reg_wait_node *wait_node = NULL, *tmp_wait_node;
> + struct ust_reg_wait_queue wait_queue = {
> + .count = 0,
> + };
>
> health_register(HEALTH_TYPE_APP_REG_DISPATCH);
>
> health_code_update();
>
> - CDS_LIST_HEAD(wait_queue);
> + CDS_INIT_LIST_HEAD(&wait_queue.head);
>
> DBG("[thread] Dispatch UST command started");
>
> @@ -1366,6 +1451,13 @@ static void *thread_dispatch_ust_registration(void *data)
> struct ust_app *app = NULL;
> ust_cmd = NULL;
>
> + /*
> + * Make sure we don't have node(s) that have hung up before receiving
> + * the notify socket. This is to clean the list in order to avoid
> + * memory leaks from notify socket that are never seen.
> + */
> + sanitize_wait_queue(&wait_queue);
> +
> health_code_update();
> /* Dequeue command for registration */
> node = cds_wfq_dequeue_blocking(&ust_cmd_queue.queue);
> @@ -1415,7 +1507,8 @@ static void *thread_dispatch_ust_registration(void *data)
> * Add application to the wait queue so we can set the notify
> * socket before putting this object in the global ht.
> */
> - cds_list_add(&wait_node->head, &wait_queue);
> + cds_list_add(&wait_node->head, &wait_queue.head);
> + wait_queue.count++;
>
> free(ust_cmd);
> /*
> @@ -1430,11 +1523,12 @@ static void *thread_dispatch_ust_registration(void *data)
> * notify socket if found.
> */
> cds_list_for_each_entry_safe(wait_node, tmp_wait_node,
> - &wait_queue, head) {
> + &wait_queue.head, head) {
> health_code_update();
> if (wait_node->app->pid == ust_cmd->reg_msg.pid) {
> wait_node->app->notify_sock = ust_cmd->sock;
> cds_list_del(&wait_node->head);
> + wait_queue.count--;
> app = wait_node->app;
> free(wait_node);
> DBG3("UST app notify socket %d is set", ust_cmd->sock);
> @@ -1529,8 +1623,9 @@ static void *thread_dispatch_ust_registration(void *data)
> error:
> /* Clean up wait queue. */
> cds_list_for_each_entry_safe(wait_node, tmp_wait_node,
> - &wait_queue, head) {
> + &wait_queue.head, head) {
> cds_list_del(&wait_node->head);
> + wait_queue.count--;
> free(wait_node);
> }
>
> diff --git a/src/bin/lttng-sessiond/ust-app.c b/src/bin/lttng-sessiond/ust-app.c
> index 37f6442..12ea705 100644
> --- a/src/bin/lttng-sessiond/ust-app.c
> +++ b/src/bin/lttng-sessiond/ust-app.c
> @@ -4806,3 +4806,15 @@ close_socket:
> call_rcu(&obj->head, close_notify_sock_rcu);
> }
> }
> +
> +/*
> + * Destroy a ust app data structure and free its memory.
> + */
> +void ust_app_destroy(struct ust_app *app)
> +{
> + if (!app) {
> + return;
> + }
> +
> + call_rcu(&app->pid_n.head, delete_ust_app_rcu);
> +}
> diff --git a/src/bin/lttng-sessiond/ust-app.h b/src/bin/lttng-sessiond/ust-app.h
> index 6e6ff02..30835e0 100644
> --- a/src/bin/lttng-sessiond/ust-app.h
> +++ b/src/bin/lttng-sessiond/ust-app.h
> @@ -305,6 +305,7 @@ struct ust_app *ust_app_create(struct ust_register_msg *msg, int sock);
> void ust_app_notify_sock_unregister(int sock);
> ssize_t ust_app_push_metadata(struct ust_registry_session *registry,
> struct consumer_socket *socket, int send_zero_data);
> +void ust_app_destroy(struct ust_app *app);
>
> #else /* HAVE_LIBLTTNG_UST_CTL */
>
> @@ -497,6 +498,11 @@ ssize_t ust_app_push_metadata(struct ust_registry_session *registry,
> {
> return 0;
> }
> +static inline
> +void ust_app_destroy(struct ust_app *app)
> +{
> + return;
> +}
>
> #endif /* HAVE_LIBLTTNG_UST_CTL */
>
> --
> 1.7.10.4
>
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
More information about the lttng-dev
mailing list