* [PATCH lttng-tools 2/3] lttng-ctl: notifications: use epoll()/poll() instead of select()
[not found] <20190516190852.16946-1-mathieu.desnoyers@efficios.com>
@ 2019-05-16 19:08 ` Mathieu Desnoyers
2019-05-16 19:08 ` [PATCH lttng-tools 3/3] sessiond: " Mathieu Desnoyers
2019-09-05 20:57 ` [PATCH lttng-tools 1/3] epoll/poll compat: expose interruptible API Jérémie Galarneau
2 siblings, 0 replies; 3+ messages in thread
From: Mathieu Desnoyers @ 2019-05-16 19:08 UTC (permalink / raw)
To: jgalar, joraj; +Cc: lttng-dev
The select(2) system call is an ancient ABI limited to processes
containing at most FD_SETSIZE file descriptors overall (typically
1024).
Those notification APIs will fail if the target file descriptor
is above FD_SETSIZE in a process containing many file descriptors.
Never use select, use the lttng epoll/poll wrapper instead.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
src/lib/lttng-ctl/channel.c | 76 ++++++++++++++++++++++---------------
1 file changed, 45 insertions(+), 31 deletions(-)
diff --git a/src/lib/lttng-ctl/channel.c b/src/lib/lttng-ctl/channel.c
index 5271aa13..bcecc65f 100644
--- a/src/lib/lttng-ctl/channel.c
+++ b/src/lib/lttng-ctl/channel.c
@@ -26,8 +26,7 @@
#include <common/defaults.h>
#include <assert.h>
#include "lttng-ctl-helper.h"
-#include <sys/select.h>
-#include <sys/time.h>
+#include <common/compat/poll.h>
static
int handshake(struct lttng_notification_channel *channel);
@@ -211,7 +210,7 @@ lttng_notification_channel_get_next_notification(
struct lttng_notification *notification = NULL;
enum lttng_notification_channel_status status =
LTTNG_NOTIFICATION_CHANNEL_STATUS_OK;
- fd_set read_fds;
+ struct lttng_poll_event events;
if (!channel || !_notification) {
status = LTTNG_NOTIFICATION_CHANNEL_STATUS_INVALID;
@@ -241,9 +240,9 @@ lttng_notification_channel_get_next_notification(
}
/*
- * Block on select() instead of the message reception itself as the
- * recvmsg() wrappers always restard on EINTR. We choose to wait
- * using select() in order to:
+ * Block on interruptible epoll/poll() instead of the message reception
+ * itself as the recvmsg() wrappers always restart on EINTR. We choose
+ * to wait using interruptible epoll/poll() in order to:
* 1) Return if a signal occurs,
* 2) Not deal with partially received messages.
*
@@ -252,20 +251,28 @@ lttng_notification_channel_get_next_notification(
* announced length, receive_message() will block on recvmsg()
* and never return (even if a signal is received).
*/
- FD_ZERO(&read_fds);
- FD_SET(channel->socket, &read_fds);
- ret = select(channel->socket + 1, &read_fds, NULL, NULL, NULL);
- if (ret == -1) {
- status = errno == EINTR ?
+ ret = lttng_poll_create(&events, 1, LTTNG_CLOEXEC);
+ if (ret < 0) {
+ status = LTTNG_NOTIFICATION_CHANNEL_STATUS_ERROR;
+ goto end_unlock;
+ }
+ ret = lttng_poll_add(&events, channel->socket, LPOLLIN | LPOLLERR);
+ if (ret < 0) {
+ status = LTTNG_NOTIFICATION_CHANNEL_STATUS_ERROR;
+ goto end_clean_poll;
+ }
+ ret = lttng_poll_wait_interruptible(&events, -1);
+ if (ret <= 0) {
+ status = (ret == -1 && errno == EINTR) ?
LTTNG_NOTIFICATION_CHANNEL_STATUS_INTERRUPTED :
LTTNG_NOTIFICATION_CHANNEL_STATUS_ERROR;
- goto end_unlock;
+ goto end_clean_poll;
}
ret = receive_message(channel);
if (ret) {
status = LTTNG_NOTIFICATION_CHANNEL_STATUS_ERROR;
- goto end_unlock;
+ goto end_clean_poll;
}
switch (get_current_message_type(channel)) {
@@ -274,7 +281,7 @@ lttng_notification_channel_get_next_notification(
channel);
if (!notification) {
status = LTTNG_NOTIFICATION_CHANNEL_STATUS_ERROR;
- goto end_unlock;
+ goto end_clean_poll;
}
break;
case LTTNG_NOTIFICATION_CHANNEL_MESSAGE_TYPE_NOTIFICATION_DROPPED:
@@ -284,9 +291,11 @@ lttng_notification_channel_get_next_notification(
default:
/* Protocol error. */
status = LTTNG_NOTIFICATION_CHANNEL_STATUS_ERROR;
- goto end_unlock;
+ goto end_clean_poll;
}
+end_clean_poll:
+ lttng_poll_clean(&events);
end_unlock:
pthread_mutex_unlock(&channel->lock);
*_notification = notification;
@@ -387,11 +396,7 @@ lttng_notification_channel_has_pending_notification(
int ret;
enum lttng_notification_channel_status status =
LTTNG_NOTIFICATION_CHANNEL_STATUS_OK;
- fd_set read_fds;
- struct timeval timeout;
-
- FD_ZERO(&read_fds);
- memset(&timeout, 0, sizeof(timeout));
+ struct lttng_poll_event events;
if (!channel || !_notification_pending) {
status = LTTNG_NOTIFICATION_CHANNEL_STATUS_INVALID;
@@ -426,48 +431,57 @@ lttng_notification_channel_has_pending_notification(
* message if we see data available on the socket. If the peer does
* not respect the protocol, this may block indefinitely.
*/
- FD_SET(channel->socket, &read_fds);
- do {
- ret = select(channel->socket + 1, &read_fds, NULL, NULL, &timeout);
- } while (ret < 0 && errno == EINTR);
-
+ ret = lttng_poll_create(&events, 1, LTTNG_CLOEXEC);
+ if (ret < 0) {
+ status = LTTNG_NOTIFICATION_CHANNEL_STATUS_ERROR;
+ goto end_unlock;
+ }
+ ret = lttng_poll_add(&events, channel->socket, LPOLLIN | LPOLLERR);
+ if (ret < 0) {
+ status = LTTNG_NOTIFICATION_CHANNEL_STATUS_ERROR;
+ goto end_clean_poll;
+ }
+ /* timeout = 0: return immediately. */
+ ret = lttng_poll_wait_interruptible(&events, 0);
if (ret == 0) {
/* No data available. */
*_notification_pending = false;
- goto end_unlock;
+ goto end_clean_poll;
} else if (ret < 0) {
status = LTTNG_NOTIFICATION_CHANNEL_STATUS_ERROR;
- goto end_unlock;
+ goto end_clean_poll;
}
/* Data available on socket. */
ret = receive_message(channel);
if (ret) {
status = LTTNG_NOTIFICATION_CHANNEL_STATUS_ERROR;
- goto end_unlock;
+ goto end_clean_poll;
}
switch (get_current_message_type(channel)) {
case LTTNG_NOTIFICATION_CHANNEL_MESSAGE_TYPE_NOTIFICATION:
ret = enqueue_notification_from_current_message(channel);
if (ret) {
- goto end_unlock;
+ goto end_clean_poll;
}
*_notification_pending = true;
break;
case LTTNG_NOTIFICATION_CHANNEL_MESSAGE_TYPE_NOTIFICATION_DROPPED:
ret = enqueue_dropped_notification(channel);
if (ret) {
- goto end_unlock;
+ goto end_clean_poll;
}
*_notification_pending = true;
break;
default:
/* Protocol error. */
status = LTTNG_NOTIFICATION_CHANNEL_STATUS_ERROR;
- goto end_unlock;
+ goto end_clean_poll;
}
+end_clean_poll:
+ lttng_poll_clean(&events);
end_unlock:
pthread_mutex_unlock(&channel->lock);
end:
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH lttng-tools 3/3] sessiond: use epoll()/poll() instead of select()
[not found] <20190516190852.16946-1-mathieu.desnoyers@efficios.com>
2019-05-16 19:08 ` [PATCH lttng-tools 2/3] lttng-ctl: notifications: use epoll()/poll() instead of select() Mathieu Desnoyers
@ 2019-05-16 19:08 ` Mathieu Desnoyers
2019-09-05 20:57 ` [PATCH lttng-tools 1/3] epoll/poll compat: expose interruptible API Jérémie Galarneau
2 siblings, 0 replies; 3+ messages in thread
From: Mathieu Desnoyers @ 2019-05-16 19:08 UTC (permalink / raw)
To: jgalar, joraj; +Cc: lttng-dev
The select(2) system call is an ancient ABI limited to processes
containing at most FD_SETSIZE file descriptors overall (typically
1024).
This select call will fail if the target file descriptor is above
FD_SETSIZE in a session daemon containing many file descriptors.
This is unlikely to happen in normal use given than
sessiond_init_thread_quit_pipe() is called early by main(). Odd
scenarios could trigger this, for instance if the parent process leaves
a large number of file descriptors open, or if a library which
allocates file descriptors is LD_PRELOADed with the sessiond.
Never use select, use the lttng epoll/poll wrapper instead.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
src/bin/lttng-sessiond/lttng-sessiond.h | 2 +-
src/bin/lttng-sessiond/main.c | 2 +-
src/bin/lttng-sessiond/thread-utils.c | 39 +++++++++++--------------
3 files changed, 19 insertions(+), 24 deletions(-)
diff --git a/src/bin/lttng-sessiond/lttng-sessiond.h b/src/bin/lttng-sessiond/lttng-sessiond.h
index d321fced..6810e896 100644
--- a/src/bin/lttng-sessiond/lttng-sessiond.h
+++ b/src/bin/lttng-sessiond/lttng-sessiond.h
@@ -161,7 +161,7 @@ extern struct consumer_data kconsumer_data;
int sessiond_init_thread_quit_pipe(void);
int sessiond_check_thread_quit_pipe(int fd, uint32_t events);
-int sessiond_wait_for_quit_pipe(unsigned int timeout_us);
+int sessiond_wait_for_quit_pipe(int timeout_ms);
int sessiond_notify_quit_pipe(void);
void sessiond_close_quit_pipe(void);
diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index 57324820..a6719a4d 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -1833,7 +1833,7 @@ int main(int argc, char **argv)
*/
/* Initiate teardown once activity occurs on the quit pipe. */
- sessiond_wait_for_quit_pipe(-1U);
+ sessiond_wait_for_quit_pipe(-1);
stop_threads:
/*
diff --git a/src/bin/lttng-sessiond/thread-utils.c b/src/bin/lttng-sessiond/thread-utils.c
index 16ae9d69..e1c02290 100644
--- a/src/bin/lttng-sessiond/thread-utils.c
+++ b/src/bin/lttng-sessiond/thread-utils.c
@@ -73,41 +73,36 @@ int sessiond_check_thread_quit_pipe(int fd, uint32_t events)
* Returns 1 if the caller should quit, 0 if the timeout was reached, and
* -1 if an error was encountered.
*/
-int sessiond_wait_for_quit_pipe(unsigned int timeout_us)
+int sessiond_wait_for_quit_pipe(int timeout_ms)
{
int ret;
- fd_set read_fds;
- struct timeval timeout;
-
- FD_ZERO(&read_fds);
- FD_SET(thread_quit_pipe[0], &read_fds);
- memset(&timeout, 0, sizeof(timeout));
- timeout.tv_sec = timeout_us / USEC_PER_SEC;
- timeout.tv_usec = timeout_us % USEC_PER_SEC;
-
- while (true) {
- ret = select(thread_quit_pipe[0] + 1, &read_fds, NULL, NULL,
- timeout_us != -1U ? &timeout : NULL);
- if (ret < 0 && errno == EINTR) {
- /* Retry on interrupt. */
- continue;
- } else {
- break;
- }
- }
+ struct lttng_poll_event events;
+ ret = lttng_poll_create(&events, 1, LTTNG_CLOEXEC);
+ if (ret < 0) {
+ PERROR("Failure in lttng_poll_create");
+ return -1;
+ }
+ ret = lttng_poll_add(&events, thread_quit_pipe[0], LPOLLIN | LPOLLERR);
+ if (ret < 0) {
+ PERROR("Failure in lttng_poll_add");
+ ret = -1;
+ goto end;
+ }
+ ret = lttng_poll_wait(&events, timeout_ms);
if (ret > 0) {
/* Should quit. */
ret = 1;
} else if (ret < 0 && errno != EINTR) {
/* Unknown error. */
- PERROR("Failed to select() thread quit pipe");
+ PERROR("Failed to epoll()/poll() thread quit pipe");
ret = -1;
} else {
/* Timeout reached. */
ret = 0;
}
-
+end:
+ lttng_poll_clean(&events);
return ret;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH lttng-tools 1/3] epoll/poll compat: expose interruptible API
[not found] <20190516190852.16946-1-mathieu.desnoyers@efficios.com>
2019-05-16 19:08 ` [PATCH lttng-tools 2/3] lttng-ctl: notifications: use epoll()/poll() instead of select() Mathieu Desnoyers
2019-05-16 19:08 ` [PATCH lttng-tools 3/3] sessiond: " Mathieu Desnoyers
@ 2019-09-05 20:57 ` Jérémie Galarneau
2 siblings, 0 replies; 3+ messages in thread
From: Jérémie Galarneau @ 2019-09-05 20:57 UTC (permalink / raw)
To: Mathieu Desnoyers; +Cc: lttng-dev, jgalar, joraj
Series merged in master and stable-2.11.
Thanks,
Jérémie
On Thu, May 16, 2019 at 03:08:50PM -0400, Mathieu Desnoyers wrote:
> Some use of the epoll/poll wrapper require interruption
> by signals to make the poll call return -1, errno EINTR.
> Expose a new lttng_poll_wait_interruptible API for this
> purpose.
>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
> src/common/compat/compat-epoll.c | 9 +++++----
> src/common/compat/compat-poll.c | 9 +++++----
> src/common/compat/poll.h | 12 ++++++++----
> 3 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/src/common/compat/compat-epoll.c b/src/common/compat/compat-epoll.c
> index 6a781c7a..7108b717 100644
> --- a/src/common/compat/compat-epoll.c
> +++ b/src/common/compat/compat-epoll.c
> @@ -241,7 +241,7 @@ error:
> /*
> * Wait on epoll set. This is a blocking call of timeout value.
> */
> -int compat_epoll_wait(struct lttng_poll_event *events, int timeout)
> +int compat_epoll_wait(struct lttng_poll_event *events, int timeout, int interruptible)
> {
> int ret;
> uint32_t new_size;
> @@ -273,10 +273,11 @@ int compat_epoll_wait(struct lttng_poll_event *events, int timeout)
>
> do {
> ret = epoll_wait(events->epfd, events->events, events->nb_fd, timeout);
> - } while (ret == -1 && errno == EINTR);
> + } while (!interruptible && ret == -1 && errno == EINTR);
> if (ret < 0) {
> - /* At this point, every error is fatal */
> - PERROR("epoll_wait");
> + if (errno != EINTR) {
> + PERROR("epoll_wait");
> + }
> goto error;
> }
>
> diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
> index 0220b278..dc507db5 100644
> --- a/src/common/compat/compat-poll.c
> +++ b/src/common/compat/compat-poll.c
> @@ -291,7 +291,7 @@ error:
> /*
> * Wait on poll() with timeout. Blocking call.
> */
> -int compat_poll_wait(struct lttng_poll_event *events, int timeout)
> +int compat_poll_wait(struct lttng_poll_event *events, int timeout, int interruptible)
> {
> int ret, active_fd_count;
> int idle_pfd_index = 0;
> @@ -320,10 +320,11 @@ int compat_poll_wait(struct lttng_poll_event *events, int timeout)
>
> do {
> ret = poll(events->wait.events, events->wait.nb_fd, timeout);
> - } while (ret == -1 && errno == EINTR);
> + } while (!interruptible && ret == -1 && errno == EINTR);
> if (ret < 0) {
> - /* At this point, every error is fatal */
> - PERROR("poll wait");
> + if (errno != EINTR) {
> + PERROR("poll wait");
> + }
> goto error;
> }
>
> diff --git a/src/common/compat/poll.h b/src/common/compat/poll.h
> index d7020f36..da7d6970 100644
> --- a/src/common/compat/poll.h
> +++ b/src/common/compat/poll.h
> @@ -152,9 +152,11 @@ static inline int compat_glibc_epoll_create(int size, int flags)
> * Wait on epoll set with the number of fd registered to the lttng_poll_event
> * data structure (events).
> */
> -extern int compat_epoll_wait(struct lttng_poll_event *events, int timeout);
> +extern int compat_epoll_wait(struct lttng_poll_event *events, int timeout, int interruptible);
> #define lttng_poll_wait(events, timeout) \
> - compat_epoll_wait(events, timeout)
> + compat_epoll_wait(events, timeout, 0)
> +#define lttng_poll_wait_interruptible(events, timeout) \
> + compat_epoll_wait(events, timeout, 1)
>
> /*
> * Add a fd to the epoll set and resize the epoll_event structure if needed.
> @@ -336,9 +338,11 @@ extern int compat_poll_create(struct lttng_poll_event *events, int size);
> * Wait on poll(2) event with nb_fd registered to the lttng_poll_event data
> * structure.
> */
> -extern int compat_poll_wait(struct lttng_poll_event *events, int timeout);
> +extern int compat_poll_wait(struct lttng_poll_event *events, int timeout, int interruptible);
> #define lttng_poll_wait(events, timeout) \
> - compat_poll_wait(events, timeout)
> + compat_poll_wait(events, timeout, 0)
> +#define lttng_poll_wait_interruptible(events, timeout) \
> + compat_poll_wait(events, timeout, 1)
>
> /*
> * Add the fd to the pollfd structure. Resize if needed.
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-05 20:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20190516190852.16946-1-mathieu.desnoyers@efficios.com>
2019-05-16 19:08 ` [PATCH lttng-tools 2/3] lttng-ctl: notifications: use epoll()/poll() instead of select() Mathieu Desnoyers
2019-05-16 19:08 ` [PATCH lttng-tools 3/3] sessiond: " Mathieu Desnoyers
2019-09-05 20:57 ` [PATCH lttng-tools 1/3] epoll/poll compat: expose interruptible API Jérémie Galarneau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).