From f5204c9440be52e1b17bbb2c82450277ec02a583 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Wed, 29 Feb 2012 22:30:17 -0500 Subject: [PATCH] Fix: make ust consumer posix compliant for poll flags poll flags can have both POLLHUP and POLLIN set, which is not expected by the consumer. Do not depend on this implementation-specific behavior. Signed-off-by: Mathieu Desnoyers Signed-off-by: David Goulet --- src/common/consumer.c | 138 +++++++++++-------- src/common/consumer.h | 16 ++- src/common/kernel-consumer/kernel-consumer.c | 12 +- src/common/kernel-consumer/kernel-consumer.h | 11 +- src/common/ust-consumer/ust-consumer.c | 6 +- src/common/ust-consumer/ust-consumer.h | 10 +- 6 files changed, 109 insertions(+), 84 deletions(-) diff --git a/src/common/consumer.c b/src/common/consumer.c index ec5940105..026d9f0f8 100644 --- a/src/common/consumer.c +++ b/src/common/consumer.c @@ -647,7 +647,7 @@ void lttng_consumer_sync_trace_file( */ struct lttng_consumer_local_data *lttng_consumer_create( enum lttng_consumer_type type, - int (*buffer_ready)(struct lttng_consumer_stream *stream, + ssize_t (*buffer_ready)(struct lttng_consumer_stream *stream, struct lttng_consumer_local_data *ctx), int (*recv_channel)(struct lttng_consumer_channel *channel), int (*recv_stream)(struct lttng_consumer_stream *stream), @@ -735,7 +735,7 @@ void lttng_consumer_destroy(struct lttng_consumer_local_data *ctx) * * Returns the number of bytes written */ -int lttng_consumer_on_read_subbuffer_mmap( +ssize_t lttng_consumer_on_read_subbuffer_mmap( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len) { @@ -756,7 +756,7 @@ int lttng_consumer_on_read_subbuffer_mmap( * * Returns the number of bytes spliced. */ -int lttng_consumer_on_read_subbuffer_splice( +ssize_t lttng_consumer_on_read_subbuffer_splice( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len) { @@ -902,6 +902,10 @@ void *lttng_consumer_thread_poll_fds(void *data) } pthread_mutex_unlock(&consumer_data.lock); + /* No FDs and consumer_quit, consumer_cleanup the thread */ + if (nb_fd == 0 && consumer_quit == 1) { + goto end; + } /* poll on the array of fds */ restart: DBG("polling on %d fd", nb_fd + 1); @@ -922,11 +926,6 @@ void *lttng_consumer_thread_poll_fds(void *data) goto end; } - /* No FDs and consumer_quit, consumer_cleanup the thread */ - if (nb_fd == 0 && consumer_quit == 1) { - goto end; - } - /* * If the consumer_poll_pipe triggered poll go * directly to the beginning of the loop to update the @@ -945,69 +944,90 @@ void *lttng_consumer_thread_poll_fds(void *data) /* Take care of high priority channels first. */ for (i = 0; i < nb_fd; i++) { if (pollfd[i].revents & POLLPRI) { + ssize_t len; + DBG("Urgent read on fd %d", pollfd[i].fd); high_prio = 1; - ret = ctx->on_buffer_ready(local_stream[i], ctx); + len = ctx->on_buffer_ready(local_stream[i], ctx); /* it's ok to have an unavailable sub-buffer */ - if (ret == EAGAIN) { - ret = 0; + if (len < 0 && len != -EAGAIN) { + goto end; + } else if (len > 0) { + local_stream[i]->data_read = 1; } - } else if (pollfd[i].revents & POLLERR) { - ERR("Error returned in polling fd %d.", pollfd[i].fd); - rcu_read_lock(); - consumer_del_stream_rcu(&local_stream[i]->node.head); - rcu_read_unlock(); - num_hup++; - } else if (pollfd[i].revents & POLLNVAL) { - ERR("Polling fd %d tells fd is not open.", pollfd[i].fd); - rcu_read_lock(); - consumer_del_stream_rcu(&local_stream[i]->node.head); - rcu_read_unlock(); - num_hup++; - } else if ((pollfd[i].revents & POLLHUP) && - !(pollfd[i].revents & POLLIN)) { - if (consumer_data.type == LTTNG_CONSUMER32_UST - || consumer_data.type == LTTNG_CONSUMER64_UST) { - DBG("Polling fd %d tells it has hung up. Attempting flush and read.", - pollfd[i].fd); - if (!local_stream[i]->hangup_flush_done) { - lttng_ustconsumer_on_stream_hangup(local_stream[i]); - /* read after flush */ - do { - ret = ctx->on_buffer_ready(local_stream[i], ctx); - } while (ret == EAGAIN); - } - } else { - DBG("Polling fd %d tells it has hung up.", pollfd[i].fd); - } - rcu_read_lock(); - consumer_del_stream_rcu(&local_stream[i]->node.head); - rcu_read_unlock(); - num_hup++; } } - /* If every buffer FD has hung up, we end the read loop here */ - if (nb_fd > 0 && num_hup == nb_fd) { - DBG("every buffer FD has hung up\n"); - if (consumer_quit == 1) { - goto end; - } + /* + * If we read high prio channel in this loop, try again + * for more high prio data. + */ + if (high_prio) { continue; } /* Take care of low priority channels. */ - if (high_prio == 0) { - for (i = 0; i < nb_fd; i++) { - if (pollfd[i].revents & POLLIN) { - DBG("Normal read on fd %d", pollfd[i].fd); - ret = ctx->on_buffer_ready(local_stream[i], ctx); - /* it's ok to have an unavailable subbuffer */ - if (ret == EAGAIN) { - ret = 0; - } + for (i = 0; i < nb_fd; i++) { + if ((pollfd[i].revents & POLLIN) || + local_stream[i]->hangup_flush_done) { + ssize_t len; + + assert(!(pollfd[i].revents & POLLERR)); + assert(!(pollfd[i].revents & POLLNVAL)); + DBG("Normal read on fd %d", pollfd[i].fd); + len = ctx->on_buffer_ready(local_stream[i], ctx); + /* it's ok to have an unavailable sub-buffer */ + if (len < 0 && len != -EAGAIN) { + goto end; + } else if (len > 0) { + local_stream[i]->data_read = 1; + } + } + } + + /* Handle hangup and errors */ + for (i = 0; i < nb_fd; i++) { + if (!local_stream[i]->hangup_flush_done + && (pollfd[i].revents & (POLLHUP | POLLERR | POLLNVAL)) + && (consumer_data.type == LTTNG_CONSUMER32_UST + || consumer_data.type == LTTNG_CONSUMER64_UST)) { + DBG("fd %d is hup|err|nval. Attempting flush and read.", + pollfd[i].fd); + lttng_ustconsumer_on_stream_hangup(local_stream[i]); + /* Attempt read again, for the data we just flushed. */ + local_stream[i]->data_read = 1; + } + /* + * If the poll flag is HUP/ERR/NVAL and we have + * read no data in this pass, we can remove the + * stream from its hash table. + */ + if ((pollfd[i].revents & POLLHUP)) { + DBG("Polling fd %d tells it has hung up.", pollfd[i].fd); + if (!local_stream[i]->data_read) { + rcu_read_lock(); + consumer_del_stream_rcu(&local_stream[i]->node.head); + rcu_read_unlock(); + num_hup++; + } + } else if (pollfd[i].revents & POLLERR) { + ERR("Error returned in polling fd %d.", pollfd[i].fd); + if (!local_stream[i]->data_read) { + rcu_read_lock(); + consumer_del_stream_rcu(&local_stream[i]->node.head); + rcu_read_unlock(); + num_hup++; + } + } else if (pollfd[i].revents & POLLNVAL) { + ERR("Polling fd %d tells fd is not open.", pollfd[i].fd); + if (!local_stream[i]->data_read) { + rcu_read_lock(); + consumer_del_stream_rcu(&local_stream[i]->node.head); + rcu_read_unlock(); + num_hup++; } } + local_stream[i]->data_read = 0; } } end: @@ -1139,7 +1159,7 @@ end: return NULL; } -int lttng_consumer_read_subbuffer(struct lttng_consumer_stream *stream, +ssize_t lttng_consumer_read_subbuffer(struct lttng_consumer_stream *stream, struct lttng_consumer_local_data *ctx) { switch (consumer_data.type) { diff --git a/src/common/consumer.h b/src/common/consumer.h index dc5fc9968..83b86a39e 100644 --- a/src/common/consumer.h +++ b/src/common/consumer.h @@ -113,6 +113,7 @@ struct lttng_consumer_stream { /* For UST */ struct lttng_ust_lib_ring_buffer *buf; int cpu; + int data_read; int hangup_flush_done; /* UID/GID of the user owning the session to which stream belongs */ uid_t uid; @@ -124,8 +125,11 @@ struct lttng_consumer_stream { * process. */ struct lttng_consumer_local_data { - /* function to call when data is available on a buffer */ - int (*on_buffer_ready)(struct lttng_consumer_stream *stream, + /* + * Function to call when data is available on a buffer. + * Returns the number of bytes read, or negative error value. + */ + ssize_t (*on_buffer_ready)(struct lttng_consumer_stream *stream, struct lttng_consumer_local_data *ctx); /* * function to call when we receive a new channel, it receives a @@ -284,16 +288,16 @@ int consumer_add_channel(struct lttng_consumer_channel *channel); extern struct lttng_consumer_local_data *lttng_consumer_create( enum lttng_consumer_type type, - int (*buffer_ready)(struct lttng_consumer_stream *stream, + ssize_t (*buffer_ready)(struct lttng_consumer_stream *stream, struct lttng_consumer_local_data *ctx), int (*recv_channel)(struct lttng_consumer_channel *channel), int (*recv_stream)(struct lttng_consumer_stream *stream), int (*update_stream)(int sessiond_key, uint32_t state)); extern void lttng_consumer_destroy(struct lttng_consumer_local_data *ctx); -extern int lttng_consumer_on_read_subbuffer_mmap( +extern ssize_t lttng_consumer_on_read_subbuffer_mmap( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len); -extern int lttng_consumer_on_read_subbuffer_splice( +extern ssize_t lttng_consumer_on_read_subbuffer_splice( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len); extern int lttng_consumer_take_snapshot(struct lttng_consumer_local_data *ctx, @@ -307,7 +311,7 @@ extern void *lttng_consumer_thread_receive_fds(void *data); extern int lttng_consumer_recv_cmd(struct lttng_consumer_local_data *ctx, int sock, struct pollfd *consumer_sockpoll); -int lttng_consumer_read_subbuffer(struct lttng_consumer_stream *stream, +ssize_t lttng_consumer_read_subbuffer(struct lttng_consumer_stream *stream, struct lttng_consumer_local_data *ctx); int lttng_consumer_on_recv_stream(struct lttng_consumer_stream *stream); diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c index bb9552953..2054a865f 100644 --- a/src/common/kernel-consumer/kernel-consumer.c +++ b/src/common/kernel-consumer/kernel-consumer.c @@ -44,12 +44,12 @@ extern volatile int consumer_quit; * * Returns the number of bytes written */ -int lttng_kconsumer_on_read_subbuffer_mmap( +ssize_t lttng_kconsumer_on_read_subbuffer_mmap( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len) { unsigned long mmap_offset; - long ret = 0; + ssize_t ret = 0; off_t orig_offset = stream->out_fd_offset; int fd = stream->wait_fd; int outfd = stream->out_fd; @@ -90,11 +90,11 @@ end: * * Returns the number of bytes spliced. */ -int lttng_kconsumer_on_read_subbuffer_splice( +ssize_t lttng_kconsumer_on_read_subbuffer_splice( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len) { - long ret = 0; + ssize_t ret = 0; loff_t offset = 0; off_t orig_offset = stream->out_fd_offset; int fd = stream->wait_fd; @@ -307,12 +307,12 @@ end_nosignal: /* * Consume data on a file descriptor and write it on a trace file. */ -int lttng_kconsumer_read_subbuffer(struct lttng_consumer_stream *stream, +ssize_t lttng_kconsumer_read_subbuffer(struct lttng_consumer_stream *stream, struct lttng_consumer_local_data *ctx) { unsigned long len; int err; - long ret = 0; + ssize_t ret = 0; int infd = stream->wait_fd; DBG("In read_subbuffer (infd : %d)", infd); diff --git a/src/common/kernel-consumer/kernel-consumer.h b/src/common/kernel-consumer/kernel-consumer.h index 6e820044b..f4f46169d 100644 --- a/src/common/kernel-consumer/kernel-consumer.h +++ b/src/common/kernel-consumer/kernel-consumer.h @@ -25,18 +25,19 @@ /* * Mmap the ring buffer, read it and write the data to the tracefile. * - * Returns the number of bytes written. + * Returns the number of bytes written, or negative value on error. */ -extern int lttng_kconsumer_on_read_subbuffer_mmap( +extern ssize_t lttng_kconsumer_on_read_subbuffer_mmap( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len); /* * Splice the data from the ring buffer to the tracefile. * - * Returns the number of bytes spliced. + * Returns the number of bytes spliced, or negative error value on + * error. */ -extern int lttng_kconsumer_on_read_subbuffer_splice( +extern ssize_t lttng_kconsumer_on_read_subbuffer_splice( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len); @@ -62,7 +63,7 @@ int lttng_kconsumer_recv_cmd(struct lttng_consumer_local_data *ctx, int sock, struct pollfd *consumer_sockpoll); -int lttng_kconsumer_read_subbuffer(struct lttng_consumer_stream *stream, +ssize_t lttng_kconsumer_read_subbuffer(struct lttng_consumer_stream *stream, struct lttng_consumer_local_data *ctx); int lttng_kconsumer_on_recv_stream(struct lttng_consumer_stream *stream); diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c index 27601463c..04c6157e5 100644 --- a/src/common/ust-consumer/ust-consumer.c +++ b/src/common/ust-consumer/ust-consumer.c @@ -42,9 +42,9 @@ extern volatile int consumer_quit; /* * Mmap the ring buffer, read it and write the data to the tracefile. * - * Returns the number of bytes written + * Returns the number of bytes written, else negative value on error. */ -int lttng_ustconsumer_on_read_subbuffer_mmap( +ssize_t lttng_ustconsumer_on_read_subbuffer_mmap( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len) { @@ -89,7 +89,7 @@ end: * * Returns the number of bytes spliced. */ -int lttng_ustconsumer_on_read_subbuffer_splice( +ssize_t lttng_ustconsumer_on_read_subbuffer_splice( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len) { diff --git a/src/common/ust-consumer/ust-consumer.h b/src/common/ust-consumer/ust-consumer.h index 76b4eeece..0089ddf17 100644 --- a/src/common/ust-consumer/ust-consumer.h +++ b/src/common/ust-consumer/ust-consumer.h @@ -30,14 +30,14 @@ /* * Mmap the ring buffer, read it and write the data to the tracefile. * - * Returns the number of bytes written. + * Returns the number of bytes written, else negative value on error. */ -extern int lttng_ustconsumer_on_read_subbuffer_mmap( +extern ssize_t lttng_ustconsumer_on_read_subbuffer_mmap( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len); /* Not implemented */ -extern int lttng_ustconsumer_on_read_subbuffer_splice( +extern ssize_t lttng_ustconsumer_on_read_subbuffer_splice( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len); @@ -76,7 +76,7 @@ void lttng_ustconsumer_on_stream_hangup(struct lttng_consumer_stream *stream); #else /* HAVE_LIBLTTNG_UST_CTL */ static inline -int lttng_ustconsumer_on_read_subbuffer_mmap( +ssize_t lttng_ustconsumer_on_read_subbuffer_mmap( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *stream, unsigned long len) { @@ -84,7 +84,7 @@ int lttng_ustconsumer_on_read_subbuffer_mmap( } static inline -int lttng_ustconsumer_on_read_subbuffer_splice( +ssize_t lttng_ustconsumer_on_read_subbuffer_splice( struct lttng_consumer_local_data *ctx, struct lttng_consumer_stream *uststream, unsigned long len) { -- 2.34.1