From: David Goulet Date: Thu, 6 Feb 2014 21:49:37 +0000 (-0500) Subject: Fix: fd leak when closing metadata stream X-Git-Tag: v2.5.0-rc1~196 X-Git-Url: https://git.lttng.org/?p=lttng-tools.git;a=commitdiff_plain;h=6d574024f868e661ae688ecbc47a110a1311c57e Fix: fd leak when closing metadata stream Make sure to close the metadata poll pipe when the endpoint of the stream dies out (ex: relayd stops unexpectedly). That was causing a fd leak when we were destroying a metadata stream without using the close metadata code path. Signed-off-by: David Goulet --- diff --git a/src/common/consumer-stream.c b/src/common/consumer-stream.c index 745de050d..5b52862c7 100644 --- a/src/common/consumer-stream.c +++ b/src/common/consumer-stream.c @@ -122,7 +122,29 @@ void consumer_stream_close(struct lttng_consumer_stream *stream) break; case LTTNG_CONSUMER32_UST: case LTTNG_CONSUMER64_UST: + { + /* + * Special case for the metadata since the wait fd is an internal pipe + * polled in the metadata thread. + */ + if (stream->metadata_flag && stream->chan->monitor) { + int rpipe = stream->ust_metadata_poll_pipe[0]; + + /* + * This will stop the channel timer if one and close the write side + * of the metadata poll pipe. + */ + lttng_ustconsumer_close_metadata(stream->chan); + if (rpipe >= 0) { + ret = close(rpipe); + if (ret < 0) { + PERROR("closing metadata pipe write side"); + } + stream->ust_metadata_poll_pipe[0] = -1; + } + } break; + } default: ERR("Unknown consumer_data type"); assert(0); @@ -195,9 +217,11 @@ void consumer_stream_delete(struct lttng_consumer_stream *stream, rcu_read_unlock(); - /* Decrement the stream count of the global consumer data. */ - assert(consumer_data.stream_count > 0); - consumer_data.stream_count--; + if (!stream->metadata_flag) { + /* Decrement the stream count of the global consumer data. */ + assert(consumer_data.stream_count > 0); + consumer_data.stream_count--; + } } /* diff --git a/src/common/consumer.c b/src/common/consumer.c index b942778b3..9d5a36970 100644 --- a/src/common/consumer.c +++ b/src/common/consumer.c @@ -225,16 +225,6 @@ struct lttng_consumer_channel *consumer_find_channel(uint64_t key) return channel; } -static void free_stream_rcu(struct rcu_head *head) -{ - struct lttng_ht_node_u64 *node = - caa_container_of(head, struct lttng_ht_node_u64, head); - struct lttng_consumer_stream *stream = - caa_container_of(node, struct lttng_consumer_stream, node); - - free(stream); -} - static void free_channel_rcu(struct rcu_head *head) { struct lttng_ht_node_u64 *node = @@ -1956,7 +1946,7 @@ int lttng_consumer_recv_cmd(struct lttng_consumer_local_data *ctx, } } -void lttng_consumer_close_metadata(void) +void lttng_consumer_close_all_metadata(void) { switch (consumer_data.type) { case LTTNG_CONSUMER_KERNEL: @@ -1974,7 +1964,7 @@ void lttng_consumer_close_metadata(void) * because at this point we are sure that the metadata producer is * either dead or blocked. */ - lttng_ustconsumer_close_metadata(metadata_ht); + lttng_ustconsumer_close_all_metadata(metadata_ht); break; default: ERR("Unknown consumer_data type"); @@ -1988,10 +1978,7 @@ void lttng_consumer_close_metadata(void) void consumer_del_metadata_stream(struct lttng_consumer_stream *stream, struct lttng_ht *ht) { - int ret; - struct lttng_ht_iter iter; struct lttng_consumer_channel *free_chan = NULL; - struct consumer_relayd_sock_pair *relayd; assert(stream); /* @@ -2002,96 +1989,17 @@ void consumer_del_metadata_stream(struct lttng_consumer_stream *stream, DBG3("Consumer delete metadata stream %d", stream->wait_fd); - if (ht == NULL) { - /* Means the stream was allocated but not successfully added */ - goto free_stream_rcu; - } - pthread_mutex_lock(&consumer_data.lock); pthread_mutex_lock(&stream->chan->lock); pthread_mutex_lock(&stream->lock); - switch (consumer_data.type) { - case LTTNG_CONSUMER_KERNEL: - if (stream->mmap_base != NULL) { - ret = munmap(stream->mmap_base, stream->mmap_len); - if (ret != 0) { - PERROR("munmap metadata stream"); - } - } - if (stream->wait_fd >= 0) { - ret = close(stream->wait_fd); - if (ret < 0) { - PERROR("close kernel metadata wait_fd"); - } - } - break; - case LTTNG_CONSUMER32_UST: - case LTTNG_CONSUMER64_UST: - if (stream->monitor) { - /* close the write-side in close_metadata */ - ret = close(stream->ust_metadata_poll_pipe[0]); - if (ret < 0) { - PERROR("Close UST metadata read-side poll pipe"); - } - } - lttng_ustconsumer_del_stream(stream); - break; - default: - ERR("Unknown consumer_data type"); - assert(0); - goto end; - } - - rcu_read_lock(); - iter.iter.node = &stream->node.node; - ret = lttng_ht_del(ht, &iter); - assert(!ret); + /* Remove any reference to that stream. */ + consumer_stream_delete(stream, ht); - iter.iter.node = &stream->node_channel_id.node; - ret = lttng_ht_del(consumer_data.stream_per_chan_id_ht, &iter); - assert(!ret); - - iter.iter.node = &stream->node_session_id.node; - ret = lttng_ht_del(consumer_data.stream_list_ht, &iter); - assert(!ret); - rcu_read_unlock(); - - if (stream->out_fd >= 0) { - ret = close(stream->out_fd); - if (ret) { - PERROR("close"); - } - } - - /* Check and cleanup relayd */ - rcu_read_lock(); - relayd = consumer_find_relayd(stream->net_seq_idx); - if (relayd != NULL) { - uatomic_dec(&relayd->refcount); - assert(uatomic_read(&relayd->refcount) >= 0); - - /* Closing streams requires to lock the control socket. */ - pthread_mutex_lock(&relayd->ctrl_sock_mutex); - ret = relayd_send_close_stream(&relayd->control_sock, - stream->relayd_stream_id, stream->next_net_seq_num - 1); - pthread_mutex_unlock(&relayd->ctrl_sock_mutex); - if (ret < 0) { - DBG("Unable to close stream on the relayd. Continuing"); - /* - * Continue here. There is nothing we can do for the relayd. - * Chances are that the relayd has closed the socket so we just - * continue cleaning up. - */ - } - - /* Both conditions are met, we destroy the relayd. */ - if (uatomic_read(&relayd->refcount) == 0 && - uatomic_read(&relayd->destroy_flag)) { - consumer_destroy_relayd(relayd); - } - } - rcu_read_unlock(); + /* Close down everything including the relayd if one. */ + consumer_stream_close(stream); + /* Destroy tracer buffers of the stream. */ + consumer_stream_destroy_buffers(stream); /* Atomically decrement channel refcount since other threads can use it. */ if (!uatomic_sub_return(&stream->chan->refcount, 1) @@ -2100,11 +2008,10 @@ void consumer_del_metadata_stream(struct lttng_consumer_stream *stream, free_chan = stream->chan; } -end: /* * Nullify the stream reference so it is not used after deletion. The - * channel lock MUST be acquired before being able to check for - * a NULL pointer value. + * channel lock MUST be acquired before being able to check for a NULL + * pointer value. */ stream->chan->metadata_stream = NULL; @@ -2116,8 +2023,7 @@ end: consumer_del_channel(free_chan); } -free_stream_rcu: - call_rcu(&stream->node.head, free_stream_rcu); + consumer_stream_free(stream); } /* @@ -2354,7 +2260,7 @@ restart: /* Add metadata stream to the global poll events list */ lttng_poll_add(&events, stream->wait_fd, - LPOLLIN | LPOLLPRI); + LPOLLIN | LPOLLPRI | LPOLLHUP); } /* Handle other stream */ @@ -3174,7 +3080,7 @@ end: * * NOTE: for now, this only applies to the UST tracer. */ - lttng_consumer_close_metadata(); + lttng_consumer_close_all_metadata(); /* * when all fds have hung up, the polling thread diff --git a/src/common/consumer.h b/src/common/consumer.h index a7517a340..84ef2719d 100644 --- a/src/common/consumer.h +++ b/src/common/consumer.h @@ -622,6 +622,8 @@ ssize_t lttng_consumer_on_read_subbuffer_splice( int lttng_consumer_take_snapshot(struct lttng_consumer_stream *stream); int lttng_consumer_get_produced_snapshot(struct lttng_consumer_stream *stream, unsigned long *pos); +int lttng_ustconsumer_get_wakeup_fd(struct lttng_consumer_stream *stream); +int lttng_ustconsumer_close_wakeup_fd(struct lttng_consumer_stream *stream); void *consumer_thread_metadata_poll(void *data); void *consumer_thread_data_poll(void *data); void *consumer_thread_sessiond_poll(void *data); diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c index bdabb5e49..34344f451 100644 --- a/src/common/ust-consumer/ust-consumer.c +++ b/src/common/ust-consumer/ust-consumer.c @@ -629,45 +629,6 @@ error: rcu_read_unlock(); return ret; } -/* - * Close metadata stream wakeup_fd using the given key to retrieve the channel. - * RCU read side lock MUST be acquired before calling this function. - * - * NOTE: This function does NOT take any channel nor stream lock. - * - * Return 0 on success else LTTng error code. - */ -static int _close_metadata(struct lttng_consumer_channel *channel) -{ - int ret = LTTCOMM_CONSUMERD_SUCCESS; - - assert(channel); - assert(channel->type == CONSUMER_CHANNEL_TYPE_METADATA); - - if (channel->switch_timer_enabled == 1) { - DBG("Deleting timer on metadata channel"); - consumer_timer_switch_stop(channel); - } - - if (channel->metadata_stream) { - ret = ustctl_stream_close_wakeup_fd(channel->metadata_stream->ustream); - if (ret < 0) { - ERR("UST consumer unable to close fd of metadata (ret: %d)", ret); - ret = LTTCOMM_CONSUMERD_ERROR_METADATA; - } - - if (channel->monitor) { - /* Close the read-side in consumer_del_metadata_stream */ - ret = close(channel->metadata_stream->ust_metadata_poll_pipe[1]); - if (ret < 0) { - PERROR("Close UST metadata write-side poll pipe"); - ret = LTTCOMM_CONSUMERD_ERROR_METADATA; - } - } - } - - return ret; -} /* * Close metadata stream wakeup_fd using the given key to retrieve the channel. @@ -702,7 +663,7 @@ static int close_metadata(uint64_t chan_key) goto error_unlock; } - ret = _close_metadata(channel); + lttng_ustconsumer_close_metadata(channel); error_unlock: pthread_mutex_unlock(&channel->lock); @@ -1729,6 +1690,22 @@ void lttng_ustconsumer_del_stream(struct lttng_consumer_stream *stream) ustctl_destroy_stream(stream->ustream); } +int lttng_ustconsumer_get_wakeup_fd(struct lttng_consumer_stream *stream) +{ + assert(stream); + assert(stream->ustream); + + return ustctl_stream_get_wakeup_fd(stream->ustream); +} + +int lttng_ustconsumer_close_wakeup_fd(struct lttng_consumer_stream *stream) +{ + assert(stream); + assert(stream->ustream); + + return ustctl_stream_close_wakeup_fd(stream->ustream); +} + /* * Populate index values of a UST stream. Values are set in big endian order. * @@ -2134,6 +2111,45 @@ end: return ret; } +/* + * Stop a given metadata channel timer if enabled and close the wait fd which + * is the poll pipe of the metadata stream. + * + * This MUST be called with the metadata channel acquired. + */ +void lttng_ustconsumer_close_metadata(struct lttng_consumer_channel *metadata) +{ + int ret; + + assert(metadata); + assert(metadata->type == CONSUMER_CHANNEL_TYPE_METADATA); + + DBG("Closing metadata channel key %" PRIu64, metadata->key); + + if (metadata->switch_timer_enabled == 1) { + consumer_timer_switch_stop(metadata); + } + + if (!metadata->metadata_stream) { + goto end; + } + + /* + * Closing write side so the thread monitoring the stream wakes up if any + * and clean the metadata stream. + */ + if (metadata->metadata_stream->ust_metadata_poll_pipe[1] >= 0) { + ret = close(metadata->metadata_stream->ust_metadata_poll_pipe[1]); + if (ret < 0) { + PERROR("closing metadata pipe write side"); + } + metadata->metadata_stream->ust_metadata_poll_pipe[1] = -1; + } + +end: + return; +} + /* * Close every metadata stream wait fd of the metadata hash table. This * function MUST be used very carefully so not to run into a race between the @@ -2143,7 +2159,7 @@ end: * producer so calling this is safe because we are assured that no state change * can occur in the metadata thread for the streams in the hash table. */ -void lttng_ustconsumer_close_metadata(struct lttng_ht *metadata_ht) +void lttng_ustconsumer_close_all_metadata(struct lttng_ht *metadata_ht) { struct lttng_ht_iter iter; struct lttng_consumer_stream *stream; @@ -2160,13 +2176,7 @@ void lttng_ustconsumer_close_metadata(struct lttng_ht *metadata_ht) health_code_update(); pthread_mutex_lock(&stream->chan->lock); - /* - * Whatever returned value, we must continue to try to close everything - * so ignore it. - */ - (void) _close_metadata(stream->chan); - DBG("Metadata wait fd %d and poll pipe fd %d closed", stream->wait_fd, - stream->ust_metadata_poll_pipe[1]); + lttng_ustconsumer_close_metadata(stream->chan); pthread_mutex_unlock(&stream->chan->lock); } diff --git a/src/common/ust-consumer/ust-consumer.h b/src/common/ust-consumer/ust-consumer.h index 2dfd2e43c..ac29857b3 100644 --- a/src/common/ust-consumer/ust-consumer.h +++ b/src/common/ust-consumer/ust-consumer.h @@ -51,7 +51,8 @@ int lttng_ustctl_get_mmap_read_offset(struct lttng_consumer_stream *stream, unsigned long *off); void *lttng_ustctl_get_mmap_base(struct lttng_consumer_stream *stream); int lttng_ustconsumer_data_pending(struct lttng_consumer_stream *stream); -void lttng_ustconsumer_close_metadata(struct lttng_ht *ht); +void lttng_ustconsumer_close_all_metadata(struct lttng_ht *ht); +void lttng_ustconsumer_close_metadata(struct lttng_consumer_channel *metadata); void lttng_ustconsumer_close_stream_wakeup(struct lttng_consumer_stream *stream); int lttng_ustconsumer_recv_metadata(int sock, uint64_t key, uint64_t offset, uint64_t len, struct lttng_consumer_channel *channel, @@ -162,7 +163,11 @@ void *lttng_ustctl_get_mmap_base(struct lttng_consumer_stream *stream) return NULL; } static inline -void lttng_ustconsumer_close_metadata(struct lttng_ht *ht) +void lttng_ustconsumer_close_all_metadata(struct lttng_ht *ht) +{ +} +static inline +void lttng_ustconsumer_close_metadata(struct lttng_consumer_channel *metadata) { } static inline