core: Reuse poll fds across calls to handle_events()

Prior to this patch, the array of poll fds given to poll() was
allocated and freed every time handle_events() was called. This
is unnecessary if the list of poll fds has not changed since the
last call to handle_events(). With this patch, the array and count
of poll fds is stored in the context and only reallocated when the
list of poll fds changes, saving any unnecessary overhead.

Signed-off-by: Chris Dickens <christopher.a.dickens@gmail.com>

3 files changed, 61 insertions, 43 deletions

diff --git a/libusb/io.c b/libusb/io.c
index 430f67e..9f7fc51 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -20,6 +20,7 @@
  */
 
 #include "config.h"
+#include <assert.h>
 #include <errno.h>
 #include <stdint.h>
 #include <stdlib.h>
@@ -1115,7 +1116,7 @@ int usbi_io_init(struct libusb_context *ctx)
 	usbi_mutex_init(&ctx->event_waiters_lock, NULL);
 	usbi_cond_init(&ctx->event_waiters_cond, NULL);
 	list_init(&ctx->flying_transfers);
-	list_init(&ctx->pollfds);
+	list_init(&ctx->ipollfds);
 
 	/* FIXME should use an eventfd on kernels that support it */
 	r = usbi_pipe(ctx->ctrl_pipe);
@@ -1194,6 +1195,8 @@ void usbi_io_exit(struct libusb_context *ctx)
 	usbi_mutex_destroy(&ctx->events_lock);
 	usbi_mutex_destroy(&ctx->event_waiters_lock);
 	usbi_cond_destroy(&ctx->event_waiters_cond);
+	if (ctx->pollfds)
+		free(ctx->pollfds);
 }
 
 static int calculate_timeout(struct usbi_transfer *transfer)
@@ -1982,26 +1985,39 @@ static int handle_events(struct libusb_context *ctx, struct timeval *tv)
 	else
 		internal_nfds = 2;
 
+	/* only reallocate the poll fds when the list of poll fds has been modified
+	 * since the last poll, otherwise reuse them to save the additional overhead */
 	usbi_mutex_lock(&ctx->pollfds_lock);
-	list_for_each_entry(ipollfd, &ctx->pollfds, list, struct usbi_pollfd)
-		nfds++;
+	if (ctx->pollfds_modified) {
+		usbi_dbg("poll fds modified, reallocating");
 
-	/* TODO: malloc when number of fd's changes, not on every poll */
-	if (nfds != 0)
-		fds = malloc(sizeof(*fds) * nfds);
-	if (!fds) {
-		usbi_mutex_unlock(&ctx->pollfds_lock);
-		return LIBUSB_ERROR_NO_MEM;
-	}
+		if (ctx->pollfds) {
+			free(ctx->pollfds);
+			ctx->pollfds = NULL;
+		}
+
+		/* sanity check - it is invalid for a context to have fewer than the
+		 * required internal fds (memory corruption?) */
+		assert(ctx->num_pollfds >= internal_nfds);
+
+		ctx->pollfds = calloc(ctx->num_pollfds, sizeof(*ctx->pollfds));
+		if (!ctx->pollfds) {
+			usbi_mutex_unlock(&ctx->pollfds_lock);
+			return LIBUSB_ERROR_NO_MEM;
+		}
+
+		list_for_each_entry(ipollfd, &ctx->ipollfds, list, struct usbi_pollfd) {
+			struct libusb_pollfd *pollfd = &ipollfd->pollfd;
+			i++;
+			ctx->pollfds[i].fd = pollfd->fd;
+			ctx->pollfds[i].events = pollfd->events;
+		}
 
-	list_for_each_entry(ipollfd, &ctx->pollfds, list, struct usbi_pollfd) {
-		struct libusb_pollfd *pollfd = &ipollfd->pollfd;
-		int fd = pollfd->fd;
-		i++;
-		fds[i].fd = fd;
-		fds[i].events = pollfd->events;
-		fds[i].revents = 0;
+		/* reset the flag now that we have the updated list */
+		ctx->pollfds_modified = 0;
 	}
+	fds = ctx->pollfds;
+	nfds = ctx->num_pollfds;
 	usbi_mutex_unlock(&ctx->pollfds_lock);
 
 	timeout_ms = (int)(tv->tv_sec * 1000) + (tv->tv_usec / 1000);
@@ -2014,14 +2030,11 @@ redo_poll:
 	usbi_dbg("poll() %d fds with timeout in %dms", nfds, timeout_ms);
 	r = usbi_poll(fds, nfds, timeout_ms);
 	usbi_dbg("poll() returned %d", r);
-	if (r == 0) {
-		free(fds);
+	if (r == 0)
 		return handle_timeouts(ctx);
-	} else if (r == -1 && errno == EINTR) {
-		free(fds);
+	else if (r == -1 && errno == EINTR)
 		return LIBUSB_ERROR_INTERRUPTED;
-	} else if (r < 0) {
-		free(fds);
+	else if (r < 0) {
 		usbi_err(ctx, "poll failed %d err=%d\n", r, errno);
 		return LIBUSB_ERROR_IO;
 	}
@@ -2096,7 +2109,6 @@ handled:
 		goto redo_poll;
 	}
 
-	free(fds);
 	return r;
 }
 
@@ -2477,7 +2489,9 @@ int usbi_add_pollfd(struct libusb_context *ctx, int fd, short events)
 	ipollfd->pollfd.fd = fd;
 	ipollfd->pollfd.events = events;
 	usbi_mutex_lock(&ctx->pollfds_lock);
-	list_add_tail(&ipollfd->list, &ctx->pollfds);
+	list_add_tail(&ipollfd->list, &ctx->ipollfds);
+	ctx->num_pollfds++;
+	ctx->pollfds_modified = 1;
 	usbi_mutex_unlock(&ctx->pollfds_lock);
 
 	if (ctx->fd_added_cb)
@@ -2493,7 +2507,7 @@ void usbi_remove_pollfd(struct libusb_context *ctx, int fd)
 
 	usbi_dbg("remove fd %d", fd);
 	usbi_mutex_lock(&ctx->pollfds_lock);
-	list_for_each_entry(ipollfd, &ctx->pollfds, list, struct usbi_pollfd)
+	list_for_each_entry(ipollfd, &ctx->ipollfds, list, struct usbi_pollfd)
 		if (ipollfd->pollfd.fd == fd) {
 			found = 1;
 			break;
@@ -2506,6 +2520,8 @@ void usbi_remove_pollfd(struct libusb_context *ctx, int fd)
 	}
 
 	list_del(&ipollfd->list);
+	ctx->num_pollfds--;
+	ctx->pollfds_modified = 1;
 	usbi_mutex_unlock(&ctx->pollfds_lock);
 	free(ipollfd);
 	if (ctx->fd_removed_cb)
@@ -2535,20 +2551,17 @@ const struct libusb_pollfd ** LIBUSB_CALL libusb_get_pollfds(
 	struct libusb_pollfd **ret = NULL;
 	struct usbi_pollfd *ipollfd;
 	size_t i = 0;
-	size_t cnt = 0;
 	USBI_GET_CONTEXT(ctx);
 
 	usbi_mutex_lock(&ctx->pollfds_lock);
-	list_for_each_entry(ipollfd, &ctx->pollfds, list, struct usbi_pollfd)
-		cnt++;
 
-	ret = calloc(cnt + 1, sizeof(struct libusb_pollfd *));
+	ret = calloc(ctx->num_pollfds + 1, sizeof(struct libusb_pollfd *));
 	if (!ret)
 		goto out;
 
-	list_for_each_entry(ipollfd, &ctx->pollfds, list, struct usbi_pollfd)
+	list_for_each_entry(ipollfd, &ctx->ipollfds, list, struct usbi_pollfd)
 		ret[i++] = (struct libusb_pollfd *) ipollfd;
-	ret[cnt] = NULL;
+	ret[ctx->num_pollfds] = NULL;
 
 out:
 	usbi_mutex_unlock(&ctx->pollfds_lock);
diff --git a/libusb/libusbi.h b/libusb/libusbi.h
index f4e9973..6e7c5ff 100644
--- a/libusb/libusbi.h
+++ b/libusb/libusbi.h
@@ -220,6 +220,14 @@ static inline void usbi_dbg(const char *format, ...)
 #define IS_XFERIN(xfer) (0 != ((xfer)->endpoint & LIBUSB_ENDPOINT_IN))
 #define IS_XFEROUT(xfer) (!IS_XFERIN(xfer))
 
+/* Internal abstraction for poll (needs struct usbi_transfer on Windows) */
+#if defined(OS_LINUX) || defined(OS_DARWIN) || defined(OS_OPENBSD) || defined(OS_NETBSD)
+#include <unistd.h>
+#include "os/poll_posix.h"
+#elif defined(OS_WINDOWS) || defined(OS_WINCE)
+#include "os/poll_windows.h"
+#endif
+
 /* Internal abstraction for thread synchronization */
 #if defined(THREADS_POSIX)
 #include "os/threads_posix.h"
@@ -257,8 +265,13 @@ struct libusb_context {
 	struct list_head flying_transfers;
 	usbi_mutex_t flying_transfers_lock;
 
-	/* list of poll fds */
-	struct list_head pollfds;
+	/* list and count of poll fds and an array of poll fd structures that is
+	 * (re)allocated as necessary prior to polling, and a flag to indicate
+	 * when the list of poll fds has changed since the last poll. */
+	struct list_head ipollfds;
+	POLL_NFDS_TYPE num_pollfds;
+	struct pollfd *pollfds;
+	unsigned int pollfds_modified;
 	usbi_mutex_t pollfds_lock;
 
 	/* a counter that is set when we want to interrupt event handling, in order
@@ -445,14 +458,6 @@ int usbi_get_config_index_by_value(struct libusb_device *dev,
 void usbi_connect_device (struct libusb_device *dev);
 void usbi_disconnect_device (struct libusb_device *dev);
 
-/* Internal abstraction for poll (needs struct usbi_transfer on Windows) */
-#if defined(OS_LINUX) || defined(OS_DARWIN) || defined(OS_OPENBSD) || defined(OS_NETBSD)
-#include <unistd.h>
-#include "os/poll_posix.h"
-#elif defined(OS_WINDOWS) || defined(OS_WINCE)
-#include "os/poll_windows.h"
-#endif
-
 #if (defined(OS_WINDOWS) || defined(OS_WINCE)) && !defined(__GNUC__)
 #define snprintf _snprintf
 #define vsnprintf _vsnprintf
diff --git a/libusb/version_nano.h b/libusb/version_nano.h
index 42fac54..ada73a8 100644
--- a/libusb/version_nano.h
+++ b/libusb/version_nano.h
@@ -1 +1 @@
-#define LIBUSB_NANO 10905
+#define LIBUSB_NANO 10911