From: David Goulet <dgoulet@efficios.com>
Date: Mon, 17 Dec 2012 20:46:28 +0000 (-0500)
Subject: Fix: poll and epoll fd set reallocation
X-Git-Tag: v2.1.0~32
X-Git-Url: https://git.lttng.org./?a=commitdiff_plain;h=d21b0d71990ac6ec4272c1f80f0ca544103628b3;p=lttng-tools.git

Fix: poll and epoll fd set reallocation

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: David Goulet <dgoulet@efficios.com>
---

diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index 91fba2643..6f8f1486c 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -697,30 +697,28 @@ static void *thread_manage_kernel(void *data)
 
 	health_code_update(&health_thread_kernel);
 
-	ret = create_thread_poll_set(&events, 2);
-	if (ret < 0) {
-		goto error_poll_create;
-	}
-
-	ret = lttng_poll_add(&events, kernel_poll_pipe[0], LPOLLIN);
-	if (ret < 0) {
-		goto error;
-	}
-
 	if (testpoint(thread_manage_kernel_before_loop)) {
-		goto error;
+		goto error_testpoint;
 	}
 
 	while (1) {
 		health_code_update(&health_thread_kernel);
 
 		if (update_poll_flag == 1) {
-			/*
-			 * Reset number of fd in the poll set. Always 2 since there is the thread
-			 * quit pipe and the kernel pipe.
-			 */
-			events.nb_fd = 2;
+			/* Clean events object. We are about to populate it again. */
+			lttng_poll_clean(&events);
+
+			ret = create_thread_poll_set(&events, 2);
+			if (ret < 0) {
+				goto error_poll_create;
+			}
+
+			ret = lttng_poll_add(&events, kernel_poll_pipe[0], LPOLLIN);
+			if (ret < 0) {
+				goto error;
+			}
 
+			/* This will add the available kernel channel if any. */
 			ret = update_kernel_poll(&events);
 			if (ret < 0) {
 				goto error;
@@ -728,7 +726,7 @@ static void *thread_manage_kernel(void *data)
 			update_poll_flag = 0;
 		}
 
-		DBG("Thread kernel polling on %d fds", events.nb_fd);
+		DBG("Thread kernel polling on %d fds", LTTNG_POLL_GETNB(&events));
 
 		/* Poll infinite value of time */
 	restart:
@@ -1122,7 +1120,7 @@ static void *thread_manage_apps(void *data)
 	health_code_update(&health_thread_app_manage);
 
 	while (1) {
-		DBG("Apps thread polling on %d fds", events.nb_fd);
+		DBG("Apps thread polling on %d fds", LTTNG_POLL_GETNB(&events));
 
 		/* Inifinite blocking call, waiting for transmission */
 	restart:
diff --git a/src/common/compat/compat-epoll.c b/src/common/compat/compat-epoll.c
index e1672c4c3..40451b3d2 100644
--- a/src/common/compat/compat-epoll.c
+++ b/src/common/compat/compat-epoll.c
@@ -16,6 +16,7 @@
  */
 
 #define _GNU_SOURCE
+#include <assert.h>
 #include <fcntl.h>
 #include <limits.h>
 #include <stdlib.h>
@@ -31,6 +32,32 @@
 
 unsigned int poll_max_size;
 
+/*
+ * Resize the epoll events structure of the new size.
+ *
+ * Return 0 on success or else -1 with the current events pointer untouched.
+ */
+static int resize_poll_event(struct lttng_poll_event *events,
+		uint32_t new_size)
+{
+	struct epoll_event *ptr;
+
+	assert(events);
+
+	ptr = realloc(events->events, new_size * sizeof(*ptr));
+	if (ptr == NULL) {
+		PERROR("realloc epoll add");
+		goto error;
+	}
+	events->events = ptr;
+	events->alloc_size = new_size;
+
+	return 0;
+
+error:
+	return -1;
+}
+
 /*
  * Create epoll set and allocate returned events structure.
  */
@@ -63,7 +90,7 @@ int compat_epoll_create(struct lttng_poll_event *events, int size, int flags)
 		goto error_close;
 	}
 
-	events->events_size = size;
+	events->alloc_size = events->init_size = size;
 	events->nb_fd = 0;
 
 	return 0;
@@ -82,8 +109,8 @@ error:
  */
 int compat_epoll_add(struct lttng_poll_event *events, int fd, uint32_t req_events)
 {
-	int ret, new_size;
-	struct epoll_event ev, *ptr;
+	int ret;
+	struct epoll_event ev;
 
 	if (events == NULL || events->events == NULL || fd < 0) {
 		ERR("Bad compat epoll add arguments");
@@ -112,17 +139,6 @@ int compat_epoll_add(struct lttng_poll_event *events, int fd, uint32_t req_event
 
 	events->nb_fd++;
 
-	if (events->nb_fd >= events->events_size) {
-		new_size = 2 * events->events_size;
-		ptr = realloc(events->events, new_size * sizeof(struct epoll_event));
-		if (ptr == NULL) {
-			PERROR("realloc epoll add");
-			goto error;
-		}
-		events->events = ptr;
-		events->events_size = new_size;
-	}
-
 end:
 	return 0;
 
@@ -172,13 +188,39 @@ error:
 int compat_epoll_wait(struct lttng_poll_event *events, int timeout)
 {
 	int ret;
+	uint32_t new_size;
 
-	if (events == NULL || events->events == NULL ||
-			events->events_size < events->nb_fd) {
+	if (events == NULL || events->events == NULL) {
 		ERR("Wrong arguments in compat_epoll_wait");
 		goto error;
 	}
 
+	/*
+	 * Resize if needed before waiting. We could either expand the array or
+	 * shrink it down. It's important to note that after this step, we are
+	 * ensured that the events argument of the epoll_wait call will be large
+	 * enough to hold every possible returned events.
+	 */
+	if (events->nb_fd > events->alloc_size) {
+		/* Expand if the nb_fd is higher than the actual size. */
+		new_size = events->alloc_size << 1UL;
+	} else if ((events->nb_fd << 1UL) <= events->alloc_size &&
+			events->nb_fd >= events->init_size) {
+		/* Shrink if nb_fd multiplied by two is <= than the actual size. */
+		new_size = events->alloc_size >> 1UL;
+	} else {
+		/* Indicate that we don't want to resize. */
+		new_size = 0;
+	}
+
+	if (new_size) {
+		ret = resize_poll_event(events, new_size);
+		if (ret < 0) {
+			/* ENOMEM problem at this point. */
+			goto error;
+		}
+	}
+
 	do {
 		ret = epoll_wait(events->epfd, events->events, events->nb_fd, timeout);
 	} while (ret == -1 && errno == EINTR);
@@ -220,7 +262,7 @@ void compat_epoll_set_max_size(void)
 	}
 
 	poll_max_size = atoi(buf);
-	if (poll_max_size <= 0) {
+	if (poll_max_size == 0) {
 		/* Extra precaution */
 		poll_max_size = DEFAULT_POLL_SIZE;
 	}
diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
index bacd96e84..cff9f44b8 100644
--- a/src/common/compat/compat-poll.c
+++ b/src/common/compat/compat-poll.c
@@ -16,6 +16,7 @@
  */
 
 #define _GNU_SOURCE
+#include <assert.h>
 #include <stdlib.h>
 #include <sys/resource.h>
 #include <sys/time.h>
@@ -27,11 +28,72 @@
 
 unsigned int poll_max_size;
 
+/*
+ * Resize the epoll events structure of the new size.
+ *
+ * Return 0 on success or else -1 with the current events pointer untouched.
+ */
+static int resize_poll_event(struct compat_poll_event_array *array,
+		uint32_t new_size)
+{
+	struct pollfd *ptr;
+
+	assert(array);
+
+	ptr = realloc(array->events, new_size * sizeof(*ptr));
+	if (ptr == NULL) {
+		PERROR("realloc epoll add");
+		goto error;
+	}
+	array->events = ptr;
+	array->alloc_size = new_size;
+
+	return 0;
+
+error:
+	return -1;
+}
+
+/*
+ * Update events with the current events object.
+ */
+static int update_current_events(struct lttng_poll_event *events)
+{
+	int ret;
+	struct compat_poll_event_array *current, *wait;
+
+	assert(events);
+
+	current = &events->current;
+	wait = &events->wait;
+
+	wait->nb_fd = current->nb_fd;
+	if (events->need_realloc) {
+		ret = resize_poll_event(wait, current->alloc_size);
+		if (ret < 0) {
+			goto error;
+		}
+	}
+	memcpy(wait->events, current->events,
+			current->nb_fd * sizeof(*current->events));
+
+	/* Update is done and realloc as well. */
+	events->need_update = 0;
+	events->need_realloc = 0;
+
+	return 0;
+
+error:
+	return -1;
+}
+
 /*
  * Create pollfd data structure.
  */
 int compat_poll_create(struct lttng_poll_event *events, int size)
 {
+	struct compat_poll_event_array *current, *wait;
+
 	if (events == NULL || size <= 0) {
 		ERR("Wrong arguments for poll create");
 		goto error;
@@ -42,15 +104,29 @@ int compat_poll_create(struct lttng_poll_event *events, int size)
 		size = poll_max_size;
 	}
 
+	/* Reset everything before begining the allocation. */
+	memset(events, 0, sizeof(struct lttng_poll_event));
+
+	/* Ease our life a bit. */
+	current = &events->current;
+	wait = &events->wait;
+
 	/* This *must* be freed by using lttng_poll_free() */
-	events->events = zmalloc(size * sizeof(struct pollfd));
-	if (events->events == NULL) {
+	wait->events = zmalloc(size * sizeof(struct pollfd));
+	if (wait->events == NULL) {
 		perror("zmalloc struct pollfd");
 		goto error;
 	}
 
-	events->events_size = size;
-	events->nb_fd = 0;
+	wait->alloc_size = wait->init_size = size;
+
+	current->events = zmalloc(size * sizeof(struct pollfd));
+	if (current->events == NULL) {
+		perror("zmalloc struct current pollfd");
+		goto error;
+	}
+
+	current->alloc_size = current->init_size = size;
 
 	return 0;
 
@@ -64,31 +140,43 @@ error:
 int compat_poll_add(struct lttng_poll_event *events, int fd,
 		uint32_t req_events)
 {
-	int new_size;
-	struct pollfd *ptr;
+	int new_size, ret, i;
+	struct compat_poll_event_array *current;
 
-	if (events == NULL || events->events == NULL || fd < 0) {
+	if (events == NULL || events->current.events == NULL || fd < 0) {
 		ERR("Bad compat poll add arguments");
 		goto error;
 	}
 
-	/* Reallocate pollfd structure by a factor of 2 if needed. */
-	if (events->nb_fd >= events->events_size) {
-		new_size = 2 * events->events_size;
-		ptr = realloc(events->events, new_size * sizeof(struct pollfd));
-		if (ptr == NULL) {
-			perror("realloc poll add");
+	/* Ease our life a bit. */
+	current = &events->current;
+
+	/* Check if fd we are trying to add is already there. */
+	for (i = 0; i < current->nb_fd; i++) {
+		/* Don't put back the fd we want to delete */
+		if (current->events[i].fd == fd) {
+			errno = EEXIST;
 			goto error;
 		}
-		events->events = ptr;
-		events->events_size = new_size;
 	}
 
-	events->events[events->nb_fd].fd = fd;
-	events->events[events->nb_fd].events = req_events;
-	events->nb_fd++;
+	/* Check for a needed resize of the array. */
+	if (current->nb_fd > current->alloc_size) {
+		/* Expand it by a power of two of the current size. */
+		new_size = current->alloc_size << 1UL;
+		ret = resize_poll_event(current, new_size);
+		if (ret < 0) {
+			goto error;
+		}
+		events->need_realloc = 1;
+	}
 
-	DBG("fd %d of %d added to pollfd", fd, events->nb_fd);
+	current->events[current->nb_fd].fd = fd;
+	current->events[current->nb_fd].events = req_events;
+	current->nb_fd++;
+	events->need_update = 1;
+
+	DBG("fd %d of %d added to pollfd", fd, current->nb_fd);
 
 	return 0;
 
@@ -101,42 +189,48 @@ error:
  */
 int compat_poll_del(struct lttng_poll_event *events, int fd)
 {
-	int new_size, i, count = 0;
-	struct pollfd *old = NULL, *new = NULL;
+	int new_size, i, count = 0, ret;
+	struct compat_poll_event_array *current;
 
-	if (events == NULL || events->events == NULL || fd < 0) {
+	if (events == NULL || events->current.events == NULL || fd < 0) {
 		ERR("Wrong arguments for poll del");
 		goto error;
 	}
 
-	old = events->events;
-	new_size = events->events_size - 1;
+	/* Ease our life a bit. */
+	current = &events->current;
 
 	/* Safety check on size */
 	if (new_size > poll_max_size) {
 		new_size = poll_max_size;
 	}
 
-	new = zmalloc(new_size * sizeof(struct pollfd));
-	if (new == NULL) {
-		perror("zmalloc poll del");
-		goto error;
+	/* Check if we need to shrink it down. */
+	if ((current->nb_fd << 1UL) <= current->alloc_size &&
+			current->nb_fd >= current->init_size) {
+		/*
+		 * Shrink if nb_fd multiplied by two is <= than the actual size and we
+		 * are above the initial size.
+		 */
+		new_size = current->alloc_size >> 1UL;
+		ret = resize_poll_event(current, new_size);
+		if (ret < 0) {
+			goto error;
+		}
+		events->need_realloc = 1;
 	}
 
-	for (i = 0; i < events->events_size; i++) {
+	for (i = 0; i < current->nb_fd; i++) {
 		/* Don't put back the fd we want to delete */
-		if (old[i].fd != fd) {
-			new[count].fd = old[i].fd;
-			new[count].events = old[i].events;
+		if (current->events[i].fd != fd) {
+			current->events[count].fd = current->events[i].fd;
+			current->events[count].events = current->events[i].events;
 			count++;
 		}
 	}
 
-	events->events_size = new_size;
-	events->events = new;
-	events->nb_fd--;
-
-	free(old);
+	current->nb_fd--;
+	events->need_update = 1;
 
 	return 0;
 
@@ -151,13 +245,26 @@ int compat_poll_wait(struct lttng_poll_event *events, int timeout)
 {
 	int ret;
 
-	if (events == NULL || events->events == NULL ||
-			events->events_size < events->nb_fd) {
+	if (events == NULL || events->current.events == NULL) {
 		ERR("poll wait arguments error");
 		goto error;
 	}
 
-	ret = poll(events->events, events->nb_fd, timeout);
+	if (events->current.nb_fd == 0) {
+		/* Return an invalid error to be consistent with epoll. */
+		errno = EINVAL;
+		goto error;
+	}
+
+	if (events->need_update) {
+		ret = update_current_events(events);
+		if (ret < 0) {
+			errno = ENOMEM;
+			goto error;
+		}
+	}
+
+	ret = poll(events->wait.events, events->wait.nb_fd, timeout);
 	if (ret < 0) {
 		/* At this point, every error is fatal */
 		perror("poll wait");
@@ -168,7 +275,7 @@ int compat_poll_wait(struct lttng_poll_event *events, int timeout)
 	 * poll() should always iterate on all FDs since we handle the pollset in
 	 * user space and after poll returns, we have to try every fd for a match.
 	 */
-	return events->nb_fd;
+	return events->wait.nb_fd;
 
 error:
 	return -1;
@@ -192,7 +299,7 @@ void compat_poll_set_max_size(void)
 	}
 
 	poll_max_size = lim.rlim_cur;
-	if (poll_max_size <= 0) {
+	if (poll_max_size == 0) {
 		/* Extra precaution */
 		poll_max_size = DEFAULT_POLL_SIZE;
 	}
diff --git a/src/common/compat/poll.h b/src/common/compat/poll.h
index cfde4fc83..2cfad9a25 100644
--- a/src/common/compat/poll.h
+++ b/src/common/compat/poll.h
@@ -78,7 +78,8 @@ enum {
 struct compat_epoll_event {
 	int epfd;
 	uint32_t nb_fd;       /* Current number of fd in events */
-	uint32_t events_size; /* Size of events array */
+	uint32_t alloc_size; /* Size of events array */
+	uint32_t init_size;	/* Initial size of events array */
 	struct epoll_event *events;
 };
 #define lttng_poll_event compat_epoll_event
@@ -202,21 +203,40 @@ enum {
 	LTTNG_CLOEXEC = 0xdead,
 };
 
-struct compat_poll_event {
+struct compat_poll_event_array {
 	uint32_t nb_fd;       /* Current number of fd in events */
-	uint32_t events_size; /* Size of events array */
+	uint32_t alloc_size; /* Size of events array */
+	/* Initial size of the pollset. We never shrink below that. */
+	uint32_t init_size;
 	struct pollfd *events;
 };
+
+struct compat_poll_event {
+	/*
+	 * Modified by the wait action. Updated using current fields if the
+	 * need_update flag is set.
+	 */
+	struct compat_poll_event_array wait;
+	/*
+	 * This is modified by add/del actions being the _current_ flow of
+	 * execution before a poll wait is done.
+	 */
+	struct compat_poll_event_array current;
+	/* Indicate if wait.events needs to be realloc. */
+	int need_realloc:1;
+	/* Indicate if wait.events need to be updated from current. */
+	int need_update:1;
+};
 #define lttng_poll_event compat_poll_event
 
 /*
  * For the following calls, consider 'e' to be a lttng_poll_event pointer and i
  * being the index of the events array.
  */
-#define LTTNG_POLL_GETFD(e, i) LTTNG_REF(e)->events[i].fd
-#define LTTNG_POLL_GETEV(e, i) LTTNG_REF(e)->events[i].revents
-#define LTTNG_POLL_GETNB(e) LTTNG_REF(e)->nb_fd
-#define LTTNG_POLL_GETSZ(e) LTTNG_REF(e)->events_size
+#define LTTNG_POLL_GETFD(e, i) LTTNG_REF(e)->wait.events[i].fd
+#define LTTNG_POLL_GETEV(e, i) LTTNG_REF(e)->wait.events[i].revents
+#define LTTNG_POLL_GETNB(e) LTTNG_REF(e)->wait.nb_fd
+#define LTTNG_POLL_GETSZ(e) LTTNG_REF(e)->wait.events_size
 
 /*
  * Create a pollfd structure of size 'size'.
@@ -270,7 +290,8 @@ static inline void lttng_poll_reset(struct lttng_poll_event *events)
 static inline void lttng_poll_clean(struct lttng_poll_event *events)
 {
 	if (events) {
-		__lttng_poll_free((void *) events->events);
+		__lttng_poll_free((void *) events->wait.events);
+		__lttng_poll_free((void *) events->current.events);
 	}
 }
 
diff --git a/src/common/consumer.c b/src/common/consumer.c
index 1de9cbadb..af99333c8 100644
--- a/src/common/consumer.c
+++ b/src/common/consumer.c
@@ -2102,12 +2102,12 @@ void *consumer_thread_metadata_poll(void *data)
 
 	while (1) {
 		/* Only the metadata pipe is set */
-		if (events.nb_fd == 0 && consumer_quit == 1) {
+		if (LTTNG_POLL_GETNB(&events) == 0 && consumer_quit == 1) {
 			goto end;
 		}
 
 restart:
-		DBG("Metadata poll wait with %d fd(s)", events.nb_fd);
+		DBG("Metadata poll wait with %d fd(s)", LTTNG_POLL_GETNB(&events));
 		ret = lttng_poll_wait(&events, -1);
 		DBG("Metadata event catched in thread");
 		if (ret < 0) {