Fix: Add missing relayd ht cleanup and ht destroy
[lttng-tools.git] / src / common / consumer.c
index dbec177b7328468b278d0205598e04c574609b53..70cde9c1028d978d7deb1865ff84fe9455988a56 100644 (file)
@@ -127,6 +127,12 @@ void consumer_steal_stream_key(int key, struct lttng_ht *ht)
        rcu_read_unlock();
 }
 
+/*
+ * Return a channel object for the given key.
+ *
+ * RCU read side lock MUST be acquired before calling this function and
+ * protects the channel ptr.
+ */
 static struct lttng_consumer_channel *consumer_find_channel(int key)
 {
        struct lttng_ht_iter iter;
@@ -138,8 +144,6 @@ static struct lttng_consumer_channel *consumer_find_channel(int key)
                return NULL;
        }
 
-       rcu_read_lock();
-
        lttng_ht_lookup(consumer_data.channel_ht, (void *)((unsigned long) key),
                        &iter);
        node = lttng_ht_iter_get_node_ulong(&iter);
@@ -147,8 +151,6 @@ static struct lttng_consumer_channel *consumer_find_channel(int key)
                channel = caa_container_of(node, struct lttng_consumer_channel, node);
        }
 
-       rcu_read_unlock();
-
        return channel;
 }
 
@@ -232,6 +234,27 @@ static void destroy_relayd(struct consumer_relayd_sock_pair *relayd)
        call_rcu(&relayd->node.head, consumer_rcu_free_relayd);
 }
 
+/*
+ * Iterate over the relayd hash table and destroy each element. Finally,
+ * destroy the whole hash table.
+ */
+static void cleanup_relayd_ht(void)
+{
+       struct lttng_ht_iter iter;
+       struct consumer_relayd_sock_pair *relayd;
+
+       rcu_read_lock();
+
+       cds_lfht_for_each_entry(consumer_data.relayd_ht->ht, &iter.iter, relayd,
+                       node.node) {
+               destroy_relayd(relayd);
+       }
+
+       lttng_ht_destroy(consumer_data.relayd_ht);
+
+       rcu_read_unlock();
+}
+
 /*
  * Update the end point status of all streams having the given network sequence
  * index (relayd index).
@@ -348,8 +371,8 @@ void consumer_del_stream(struct lttng_consumer_stream *stream,
                goto free_stream;
        }
 
-       pthread_mutex_lock(&stream->lock);
        pthread_mutex_lock(&consumer_data.lock);
+       pthread_mutex_lock(&stream->lock);
 
        switch (consumer_data.type) {
        case LTTNG_CONSUMER_KERNEL:
@@ -441,8 +464,8 @@ void consumer_del_stream(struct lttng_consumer_stream *stream,
 
 end:
        consumer_data.need_update = 1;
-       pthread_mutex_unlock(&consumer_data.lock);
        pthread_mutex_unlock(&stream->lock);
+       pthread_mutex_unlock(&consumer_data.lock);
 
        if (free_chan) {
                consumer_del_channel(free_chan);
@@ -475,6 +498,8 @@ struct lttng_consumer_stream *consumer_allocate_stream(
                goto end;
        }
 
+       rcu_read_lock();
+
        /*
         * Get stream's channel reference. Needed when adding the stream to the
         * global hash table.
@@ -531,9 +556,12 @@ struct lttng_consumer_stream *consumer_allocate_stream(
                        stream->path_name, stream->key, stream->shm_fd, stream->wait_fd,
                        (unsigned long long) stream->mmap_len, stream->out_fd,
                        stream->net_seq_idx, stream->session_id);
+
+       rcu_read_unlock();
        return stream;
 
 error:
+       rcu_read_unlock();
        free(stream);
 end:
        return NULL;
@@ -554,6 +582,7 @@ static int consumer_add_stream(struct lttng_consumer_stream *stream,
        DBG3("Adding consumer stream %d", stream->key);
 
        pthread_mutex_lock(&consumer_data.lock);
+       pthread_mutex_lock(&stream->lock);
        rcu_read_lock();
 
        /* Steal stream identifier to avoid having streams with the same key */
@@ -593,6 +622,7 @@ static int consumer_add_stream(struct lttng_consumer_stream *stream,
        consumer_data.need_update = 1;
 
        rcu_read_unlock();
+       pthread_mutex_unlock(&stream->lock);
        pthread_mutex_unlock(&consumer_data.lock);
 
        return ret;
@@ -755,6 +785,8 @@ void consumer_del_channel(struct lttng_consumer_channel *channel)
        int ret;
        struct lttng_ht_iter iter;
 
+       DBG("Consumer delete channel key %d", channel->key);
+
        pthread_mutex_lock(&consumer_data.lock);
 
        switch (consumer_data.type) {
@@ -908,7 +940,7 @@ static int consumer_update_poll_array(
                 * changed where this function will be called back again.
                 */
                if (stream->state != LTTNG_CONSUMER_ACTIVE_STREAM ||
-                               stream->endpoint_status) {
+                               stream->endpoint_status == CONSUMER_ENDPOINT_INACTIVE) {
                        continue;
                }
                DBG("Active FD %d", stream->wait_fd);
@@ -992,8 +1024,8 @@ int lttng_consumer_send_error(
 }
 
 /*
- * Close all the tracefiles and stream fds, should be called when all instances
- * are destroyed.
+ * Close all the tracefiles and stream fds and MUST be called when all
+ * instances are destroyed i.e. when all threads were joined and are ended.
  */
 void lttng_consumer_cleanup(void)
 {
@@ -1012,6 +1044,15 @@ void lttng_consumer_cleanup(void)
        rcu_read_unlock();
 
        lttng_ht_destroy(consumer_data.channel_ht);
+
+       cleanup_relayd_ht();
+
+       /*
+        * This HT contains streams that are freed by either the metadata thread or
+        * the data thread so we do *nothing* on the hash table and simply destroy
+        * it.
+        */
+       lttng_ht_destroy(consumer_data.stream_list_ht);
 }
 
 /*
@@ -1258,6 +1299,8 @@ end:
  * core function for writing trace buffers to either the local filesystem or
  * the network.
  *
+ * It must be called with the stream lock held.
+ *
  * Careful review MUST be put if any changes occur!
  *
  * Returns the number of bytes written
@@ -1278,8 +1321,6 @@ ssize_t lttng_consumer_on_read_subbuffer_mmap(
        /* RCU lock for the relayd pointer */
        rcu_read_lock();
 
-       pthread_mutex_lock(&stream->lock);
-
        /* Flag that the current stream if set for network streaming. */
        if (stream->net_seq_idx != -1) {
                relayd = consumer_find_relayd(stream->net_seq_idx);
@@ -1404,7 +1445,6 @@ end:
        if (relayd && stream->metadata_flag) {
                pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
        }
-       pthread_mutex_unlock(&stream->lock);
 
        rcu_read_unlock();
        return written;
@@ -1413,6 +1453,8 @@ end:
 /*
  * Splice the data from the ring buffer to the tracefile.
  *
+ * It must be called with the stream lock held.
+ *
  * Returns the number of bytes spliced.
  */
 ssize_t lttng_consumer_on_read_subbuffer_splice(
@@ -1445,8 +1487,6 @@ ssize_t lttng_consumer_on_read_subbuffer_splice(
        /* RCU lock for the relayd pointer */
        rcu_read_lock();
 
-       pthread_mutex_lock(&stream->lock);
-
        /* Flag that the current stream if set for network streaming. */
        if (stream->net_seq_idx != -1) {
                relayd = consumer_find_relayd(stream->net_seq_idx);
@@ -1614,7 +1654,6 @@ end:
        if (relayd && stream->metadata_flag) {
                pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
        }
-       pthread_mutex_unlock(&stream->lock);
 
        rcu_read_unlock();
        return written;
@@ -1688,7 +1727,6 @@ int lttng_consumer_recv_cmd(struct lttng_consumer_local_data *ctx,
  */
 static void destroy_data_stream_ht(struct lttng_ht *ht)
 {
-       int ret;
        struct lttng_ht_iter iter;
        struct lttng_consumer_stream *stream;
 
@@ -1698,10 +1736,11 @@ static void destroy_data_stream_ht(struct lttng_ht *ht)
 
        rcu_read_lock();
        cds_lfht_for_each_entry(ht->ht, &iter.iter, stream, node.node) {
-               ret = lttng_ht_del(ht, &iter);
-               assert(!ret);
-
-               call_rcu(&stream->node.head, consumer_free_stream);
+               /*
+                * Ignore return value since we are currently cleaning up so any error
+                * can't be handled.
+                */
+               (void) consumer_del_stream(stream, ht);
        }
        rcu_read_unlock();
 
@@ -1715,7 +1754,6 @@ static void destroy_data_stream_ht(struct lttng_ht *ht)
  */
 static void destroy_stream_ht(struct lttng_ht *ht)
 {
-       int ret;
        struct lttng_ht_iter iter;
        struct lttng_consumer_stream *stream;
 
@@ -1725,10 +1763,11 @@ static void destroy_stream_ht(struct lttng_ht *ht)
 
        rcu_read_lock();
        cds_lfht_for_each_entry(ht->ht, &iter.iter, stream, node.node) {
-               ret = lttng_ht_del(ht, &iter);
-               assert(!ret);
-
-               call_rcu(&stream->node.head, consumer_free_stream);
+               /*
+                * Ignore return value since we are currently cleaning up so any error
+                * can't be handled.
+                */
+               (void) consumer_del_metadata_stream(stream, ht);
        }
        rcu_read_unlock();
 
@@ -1760,9 +1799,9 @@ void consumer_del_metadata_stream(struct lttng_consumer_stream *stream,
                goto free_stream;
        }
 
+       pthread_mutex_lock(&consumer_data.lock);
        pthread_mutex_lock(&stream->lock);
 
-       pthread_mutex_lock(&consumer_data.lock);
        switch (consumer_data.type) {
        case LTTNG_CONSUMER_KERNEL:
                if (stream->mmap_base != NULL) {
@@ -1852,8 +1891,8 @@ void consumer_del_metadata_stream(struct lttng_consumer_stream *stream,
        }
 
 end:
-       pthread_mutex_unlock(&consumer_data.lock);
        pthread_mutex_unlock(&stream->lock);
+       pthread_mutex_unlock(&consumer_data.lock);
 
        if (free_chan) {
                consumer_del_channel(free_chan);
@@ -1872,6 +1911,8 @@ static int consumer_add_metadata_stream(struct lttng_consumer_stream *stream,
 {
        int ret = 0;
        struct consumer_relayd_sock_pair *relayd;
+       struct lttng_ht_iter iter;
+       struct lttng_ht_node_ulong *node;
 
        assert(stream);
        assert(ht);
@@ -1879,6 +1920,7 @@ static int consumer_add_metadata_stream(struct lttng_consumer_stream *stream,
        DBG3("Adding metadata stream %d to hash table", stream->wait_fd);
 
        pthread_mutex_lock(&consumer_data.lock);
+       pthread_mutex_lock(&stream->lock);
 
        /*
         * From here, refcounts are updated so be _careful_ when returning an error
@@ -1886,6 +1928,15 @@ static int consumer_add_metadata_stream(struct lttng_consumer_stream *stream,
         */
 
        rcu_read_lock();
+
+       /*
+        * Lookup the stream just to make sure it does not exist in our internal
+        * state. This should NEVER happen.
+        */
+       lttng_ht_lookup(ht, (void *)((unsigned long) stream->wait_fd), &iter);
+       node = lttng_ht_iter_get_node_ulong(&iter);
+       assert(!node);
+
        /* Find relayd and, if one is found, increment refcount. */
        relayd = consumer_find_relayd(stream->net_seq_idx);
        if (relayd != NULL) {
@@ -1906,9 +1957,6 @@ static int consumer_add_metadata_stream(struct lttng_consumer_stream *stream,
                uatomic_dec(&stream->chan->nb_init_streams);
        }
 
-       /* Steal stream identifier to avoid having streams with the same key */
-       consumer_steal_stream_key(stream->key, ht);
-
        lttng_ht_add_unique_ulong(ht, &stream->node);
 
        /*
@@ -1920,6 +1968,7 @@ static int consumer_add_metadata_stream(struct lttng_consumer_stream *stream,
 
        rcu_read_unlock();
 
+       pthread_mutex_unlock(&stream->lock);
        pthread_mutex_unlock(&consumer_data.lock);
        return ret;
 }
@@ -1937,7 +1986,7 @@ static void validate_endpoint_status_data_stream(void)
        rcu_read_lock();
        cds_lfht_for_each_entry(data_ht->ht, &iter.iter, stream, node.node) {
                /* Validate delete flag of the stream */
-               if (stream->endpoint_status != CONSUMER_ENDPOINT_INACTIVE) {
+               if (stream->endpoint_status == CONSUMER_ENDPOINT_ACTIVE) {
                        continue;
                }
                /* Delete it right now */
@@ -1962,7 +2011,7 @@ static void validate_endpoint_status_metadata_stream(
        rcu_read_lock();
        cds_lfht_for_each_entry(metadata_ht->ht, &iter.iter, stream, node.node) {
                /* Validate delete flag of the stream */
-               if (!stream->endpoint_status) {
+               if (stream->endpoint_status == CONSUMER_ENDPOINT_ACTIVE) {
                        continue;
                }
                /*
@@ -1994,6 +2043,12 @@ void *consumer_thread_metadata_poll(void *data)
 
        rcu_register_thread();
 
+       metadata_ht = lttng_ht_new(0, LTTNG_HT_TYPE_ULONG);
+       if (!metadata_ht) {
+               /* ENOMEM at this point. Better to bail out. */
+               goto error;
+       }
+
        DBG("Thread metadata poll started");
 
        /* Size is set to 1 for the consumer_metadata pipe */
@@ -2160,9 +2215,7 @@ end:
        DBG("Metadata poll thread exiting");
        lttng_poll_clean(&events);
 
-       if (metadata_ht) {
-               destroy_stream_ht(metadata_ht);
-       }
+       destroy_stream_ht(metadata_ht);
 
        rcu_unregister_thread();
        return NULL;
@@ -2187,6 +2240,7 @@ void *consumer_thread_data_poll(void *data)
 
        data_ht = lttng_ht_new(0, LTTNG_HT_TYPE_ULONG);
        if (data_ht == NULL) {
+               /* ENOMEM at this point. Better to bail out. */
                goto end;
        }
 
@@ -2423,9 +2477,7 @@ end:
                PERROR("close data pipe");
        }
 
-       if (data_ht) {
-               destroy_data_stream_ht(data_ht);
-       }
+       destroy_data_stream_ht(data_ht);
 
        rcu_unregister_thread();
        return NULL;
@@ -2437,7 +2489,7 @@ end:
  */
 void *consumer_thread_sessiond_poll(void *data)
 {
-       int sock, client_socket, ret;
+       int sock = -1, client_socket, ret;
        /*
         * structure to poll for incoming data on communication socket avoids
         * making blocking sockets.
@@ -2497,6 +2549,13 @@ void *consumer_thread_sessiond_poll(void *data)
                goto end;
        }
 
+       /* This socket is not useful anymore. */
+       ret = close(client_socket);
+       if (ret < 0) {
+               PERROR("close client_socket");
+       }
+       client_socket = -1;
+
        /* update the polling structure to poll on the established socket */
        consumer_sockpoll[1].fd = sock;
        consumer_sockpoll[1].events = POLLIN | POLLPRI;
@@ -2540,6 +2599,20 @@ end:
         */
        notify_thread_pipe(ctx->consumer_data_pipe[1]);
 
+       /* Cleaning up possibly open sockets. */
+       if (sock >= 0) {
+               ret = close(sock);
+               if (ret < 0) {
+                       PERROR("close sock sessiond poll");
+               }
+       }
+       if (client_socket >= 0) {
+               ret = close(sock);
+               if (ret < 0) {
+                       PERROR("close client_socket sessiond poll");
+               }
+       }
+
        rcu_unregister_thread();
        return NULL;
 }
@@ -2547,17 +2620,27 @@ end:
 ssize_t lttng_consumer_read_subbuffer(struct lttng_consumer_stream *stream,
                struct lttng_consumer_local_data *ctx)
 {
+       ssize_t ret;
+
+       pthread_mutex_lock(&stream->lock);
+
        switch (consumer_data.type) {
        case LTTNG_CONSUMER_KERNEL:
-               return lttng_kconsumer_read_subbuffer(stream, ctx);
+               ret = lttng_kconsumer_read_subbuffer(stream, ctx);
+               break;
        case LTTNG_CONSUMER32_UST:
        case LTTNG_CONSUMER64_UST:
-               return lttng_ustconsumer_read_subbuffer(stream, ctx);
+               ret = lttng_ustconsumer_read_subbuffer(stream, ctx);
+               break;
        default:
                ERR("Unknown consumer_data type");
                assert(0);
-               return -ENOSYS;
+               ret = -ENOSYS;
+               break;
        }
+
+       pthread_mutex_unlock(&stream->lock);
+       return ret;
 }
 
 int lttng_consumer_on_recv_stream(struct lttng_consumer_stream *stream)
@@ -2583,11 +2666,6 @@ void lttng_consumer_init(void)
        consumer_data.channel_ht = lttng_ht_new(0, LTTNG_HT_TYPE_ULONG);
        consumer_data.relayd_ht = lttng_ht_new(0, LTTNG_HT_TYPE_ULONG);
        consumer_data.stream_list_ht = lttng_ht_new(0, LTTNG_HT_TYPE_ULONG);
-
-       metadata_ht = lttng_ht_new(0, LTTNG_HT_TYPE_ULONG);
-       assert(metadata_ht);
-       data_ht = lttng_ht_new(0, LTTNG_HT_TYPE_ULONG);
-       assert(data_ht);
 }
 
 /*
@@ -2600,7 +2678,7 @@ int consumer_add_relayd_socket(int net_seq_idx, int sock_type,
                struct lttng_consumer_local_data *ctx, int sock,
                struct pollfd *consumer_sockpoll, struct lttcomm_sock *relayd_sock)
 {
-       int fd, ret = -1;
+       int fd = -1, ret = -1;
        struct consumer_relayd_sock_pair *relayd;
 
        DBG("Consumer adding relayd socket (idx: %d)", net_seq_idx);
@@ -2627,6 +2705,7 @@ int consumer_add_relayd_socket(int net_seq_idx, int sock_type,
        if (ret != sizeof(fd)) {
                lttng_consumer_send_error(ctx, LTTCOMM_CONSUMERD_ERROR_RECV_FD);
                ret = -1;
+               fd = -1;        /* Just in case it gets set with an invalid value. */
                goto error;
        }
 
@@ -2636,14 +2715,15 @@ int consumer_add_relayd_socket(int net_seq_idx, int sock_type,
                /* Copy received lttcomm socket */
                lttcomm_copy_sock(&relayd->control_sock, relayd_sock);
                ret = lttcomm_create_sock(&relayd->control_sock);
-               if (ret < 0) {
-                       goto error;
+               /* Immediately try to close the created socket if valid. */
+               if (relayd->control_sock.fd >= 0) {
+                       if (close(relayd->control_sock.fd)) {
+                               PERROR("close relayd control socket");
+                       }
                }
-
-               /* Close the created socket fd which is useless */
-               ret = close(relayd->control_sock.fd);
+               /* Handle create_sock error. */
                if (ret < 0) {
-                       PERROR("close relayd control socket");
+                       goto error;
                }
 
                /* Assign new file descriptor */
@@ -2653,14 +2733,15 @@ int consumer_add_relayd_socket(int net_seq_idx, int sock_type,
                /* Copy received lttcomm socket */
                lttcomm_copy_sock(&relayd->data_sock, relayd_sock);
                ret = lttcomm_create_sock(&relayd->data_sock);
-               if (ret < 0) {
-                       goto error;
+               /* Immediately try to close the created socket if valid. */
+               if (relayd->data_sock.fd >= 0) {
+                       if (close(relayd->data_sock.fd)) {
+                               PERROR("close relayd data socket");
+                       }
                }
-
-               /* Close the created socket fd which is useless */
-               ret = close(relayd->data_sock.fd);
+               /* Handle create_sock error. */
                if (ret < 0) {
-                       PERROR("close relayd control socket");
+                       goto error;
                }
 
                /* Assign new file descriptor */
@@ -2682,9 +2763,15 @@ int consumer_add_relayd_socket(int net_seq_idx, int sock_type,
        add_relayd(relayd);
 
        /* All good! */
-       ret = 0;
+       return 0;
 
 error:
+       /* Close received socket if valid. */
+       if (fd >= 0) {
+               if (close(fd)) {
+                       PERROR("close received socket");
+               }
+       }
        return ret;
 }
 
This page took 0.030422 seconds and 4 git commands to generate.