From 4c28cd8593cbbca234a1fa0b9b84f4356fd00796 Mon Sep 17 00:00:00 2001 From: Chris Dickens Date: Sun, 21 Dec 2014 14:58:35 -0800 Subject: [PATCH] core: Improve the handling of the pollfd list This change consists of two parts that must be taken together. Part 1 moves the pollfd list under the protection of event_data_lock and eliminates the pollfd_lock. Since modifications to the pollfd list are considered an event, it makes sense to merge this. Another benefit of doing so is an enhancement to event handling. The event handler can get the updated pollfd list upon entry into handle_events() and can clear the event pipe if no other pending events exist, which saves a needless iteration. Part 2 makes notification of pollfd list changes part of adding or removing a pollfd from the list. Previously this was done in two distinct steps, however nothing prevented a new pollfd from being used by an event handler before an explicit notification was sent out. This change eliminates the need for USBI_TRANSFER_UPDATED_FDS and reverts 9a9ef3ec2b9c691609ec9f8b82ac4436a662df18. Signed-off-by: Chris Dickens --- libusb/core.c | 14 ++------------ libusb/io.c | 46 +++++++++++++++++++++------------------------- libusb/libusbi.h | 36 ++++++++++++++---------------------- libusb/os/darwin_usb.c | 2 +- libusb/os/linux_usbfs.c | 2 +- libusb/os/netbsd_usb.c | 2 +- libusb/os/openbsd_usb.c | 2 +- libusb/os/wince_usb.c | 1 - libusb/os/windows_usb.c | 3 --- 9 files changed, 41 insertions(+), 67 deletions(-) diff --git a/libusb/core.c b/libusb/core.c index 9894209..698f9f5 100644 --- a/libusb/core.c +++ b/libusb/core.c @@ -1196,7 +1196,7 @@ int usbi_clear_event(struct libusb_context *ctx) /* * Interrupt the iteration of the event handling thread, so that it picks - * up the new fd. + * up the fd change. */ void usbi_fd_notification(struct libusb_context *ctx) { @@ -1206,7 +1206,7 @@ void usbi_fd_notification(struct libusb_context *ctx) * Only signal an event if there are no prior pending events. */ usbi_mutex_lock(&ctx->event_data_lock); pending_events = usbi_pending_events(ctx); - ctx->fd_notify = 1; + ctx->pollfds_modified = 1; if (!pending_events) usbi_signal_event(ctx); usbi_mutex_unlock(&ctx->event_data_lock); @@ -1273,16 +1273,6 @@ int API_EXPORTED libusb_open(libusb_device *dev, usbi_mutex_unlock(&ctx->open_devs_lock); *handle = _handle; - if (usbi_backend->caps & USBI_CAP_HAS_POLLABLE_DEVICE_FD) { - /* At this point, we want to interrupt any existing event handlers so - * that they realise the addition of the new device's poll fd. One - * example when this is desirable is if the user is running a separate - * dedicated libusb events handling thread, which is running with a long - * or infinite timeout. We want to interrupt that iteration of the loop, - * so that it picks up the new fd, and then continues. */ - usbi_fd_notification(ctx); - } - return 0; } diff --git a/libusb/io.c b/libusb/io.c index a5fda69..0e219f8 100644 --- a/libusb/io.c +++ b/libusb/io.c @@ -1111,11 +1111,10 @@ int usbi_io_init(struct libusb_context *ctx) int r; usbi_mutex_init(&ctx->flying_transfers_lock, NULL); - usbi_mutex_init(&ctx->pollfds_lock, NULL); usbi_mutex_init_recursive(&ctx->events_lock, NULL); - usbi_mutex_init(&ctx->event_data_lock, NULL); usbi_mutex_init(&ctx->event_waiters_lock, NULL); usbi_cond_init(&ctx->event_waiters_cond, NULL); + usbi_mutex_init_recursive(&ctx->event_data_lock, NULL); list_init(&ctx->flying_transfers); list_init(&ctx->ipollfds); list_init(&ctx->hotplug_msgs); @@ -1157,11 +1156,10 @@ err_close_pipe: usbi_close(ctx->event_pipe[1]); err: usbi_mutex_destroy(&ctx->flying_transfers_lock); - usbi_mutex_destroy(&ctx->pollfds_lock); usbi_mutex_destroy(&ctx->events_lock); - usbi_mutex_destroy(&ctx->event_data_lock); usbi_mutex_destroy(&ctx->event_waiters_lock); usbi_cond_destroy(&ctx->event_waiters_cond); + usbi_mutex_destroy(&ctx->event_data_lock); return r; } @@ -1177,11 +1175,10 @@ void usbi_io_exit(struct libusb_context *ctx) } #endif usbi_mutex_destroy(&ctx->flying_transfers_lock); - usbi_mutex_destroy(&ctx->pollfds_lock); usbi_mutex_destroy(&ctx->events_lock); - usbi_mutex_destroy(&ctx->event_data_lock); usbi_mutex_destroy(&ctx->event_waiters_lock); usbi_cond_destroy(&ctx->event_waiters_cond); + usbi_mutex_destroy(&ctx->event_data_lock); if (ctx->pollfds) free(ctx->pollfds); } @@ -1427,7 +1424,6 @@ int API_EXPORTED libusb_submit_transfer(struct libusb_transfer *transfer) struct usbi_transfer *itransfer = LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer); int r; - int updated_fds; usbi_mutex_lock(&ctx->flying_transfers_lock); usbi_mutex_lock(&itransfer->lock); @@ -1451,11 +1447,8 @@ int API_EXPORTED libusb_submit_transfer(struct libusb_transfer *transfer) libusb_ref_device(transfer->dev_handle->dev); } out: - updated_fds = (itransfer->flags & USBI_TRANSFER_UPDATED_FDS); usbi_mutex_unlock(&itransfer->lock); usbi_mutex_unlock(&ctx->flying_transfers_lock); - if (updated_fds) - usbi_fd_notification(ctx); return r; } @@ -1973,7 +1966,7 @@ static int handle_events(struct libusb_context *ctx, struct timeval *tv) /* 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); + usbi_mutex_lock(&ctx->event_data_lock); if (ctx->pollfds_modified) { usbi_dbg("poll fds modified, reallocating"); @@ -1988,7 +1981,7 @@ static int handle_events(struct libusb_context *ctx, struct timeval *tv) ctx->pollfds = calloc(ctx->pollfds_cnt, sizeof(*ctx->pollfds)); if (!ctx->pollfds) { - usbi_mutex_unlock(&ctx->pollfds_lock); + usbi_mutex_unlock(&ctx->event_data_lock); return LIBUSB_ERROR_NO_MEM; } @@ -2001,10 +1994,15 @@ static int handle_events(struct libusb_context *ctx, struct timeval *tv) /* reset the flag now that we have the updated list */ ctx->pollfds_modified = 0; + + /* if no further pending events, clear the event pipe so that we do + * not immediately return from poll */ + if (!usbi_pending_events(ctx)) + usbi_clear_event(ctx); } fds = ctx->pollfds; nfds = ctx->pollfds_cnt; - usbi_mutex_unlock(&ctx->pollfds_lock); + usbi_mutex_unlock(&ctx->event_data_lock); timeout_ms = (int)(tv->tv_sec * 1000) + (tv->tv_usec / 1000); @@ -2037,10 +2035,8 @@ redo_poll: usbi_mutex_lock(&ctx->event_data_lock); /* check if someone added a new poll fd */ - if (ctx->fd_notify) { + if (ctx->pollfds_modified) usbi_dbg("someone updated the poll fds"); - ctx->fd_notify = 0; - } /* check if someone is closing a device */ if (ctx->device_close) @@ -2484,11 +2480,11 @@ int usbi_add_pollfd(struct libusb_context *ctx, int fd, short events) usbi_dbg("add fd %d events %d", fd, events); ipollfd->pollfd.fd = fd; ipollfd->pollfd.events = events; - usbi_mutex_lock(&ctx->pollfds_lock); + usbi_mutex_lock(&ctx->event_data_lock); list_add_tail(&ipollfd->list, &ctx->ipollfds); ctx->pollfds_cnt++; - ctx->pollfds_modified = 1; - usbi_mutex_unlock(&ctx->pollfds_lock); + usbi_fd_notification(ctx); + usbi_mutex_unlock(&ctx->event_data_lock); if (ctx->fd_added_cb) ctx->fd_added_cb(fd, events, ctx->fd_cb_user_data); @@ -2502,7 +2498,7 @@ void usbi_remove_pollfd(struct libusb_context *ctx, int fd) int found = 0; usbi_dbg("remove fd %d", fd); - usbi_mutex_lock(&ctx->pollfds_lock); + usbi_mutex_lock(&ctx->event_data_lock); list_for_each_entry(ipollfd, &ctx->ipollfds, list, struct usbi_pollfd) if (ipollfd->pollfd.fd == fd) { found = 1; @@ -2511,14 +2507,14 @@ void usbi_remove_pollfd(struct libusb_context *ctx, int fd) if (!found) { usbi_dbg("couldn't find fd %d to remove", fd); - usbi_mutex_unlock(&ctx->pollfds_lock); + usbi_mutex_unlock(&ctx->event_data_lock); return; } list_del(&ipollfd->list); ctx->pollfds_cnt--; - ctx->pollfds_modified = 1; - usbi_mutex_unlock(&ctx->pollfds_lock); + usbi_fd_notification(ctx); + usbi_mutex_unlock(&ctx->event_data_lock); free(ipollfd); if (ctx->fd_removed_cb) ctx->fd_removed_cb(fd, ctx->fd_cb_user_data); @@ -2549,7 +2545,7 @@ const struct libusb_pollfd ** LIBUSB_CALL libusb_get_pollfds( size_t i = 0; USBI_GET_CONTEXT(ctx); - usbi_mutex_lock(&ctx->pollfds_lock); + usbi_mutex_lock(&ctx->event_data_lock); ret = calloc(ctx->pollfds_cnt + 1, sizeof(struct libusb_pollfd *)); if (!ret) @@ -2560,7 +2556,7 @@ const struct libusb_pollfd ** LIBUSB_CALL libusb_get_pollfds( ret[ctx->pollfds_cnt] = NULL; out: - usbi_mutex_unlock(&ctx->pollfds_lock); + usbi_mutex_unlock(&ctx->event_data_lock); return (const struct libusb_pollfd **) ret; #else usbi_err(ctx, "external polling of libusb's internal descriptors "\ diff --git a/libusb/libusbi.h b/libusb/libusbi.h index 2d2f71a..fc3ba19 100644 --- a/libusb/libusbi.h +++ b/libusb/libusbi.h @@ -61,7 +61,6 @@ extern "C" { /* Backend specific capabilities */ #define USBI_CAP_HAS_HID_ACCESS 0x00010000 #define USBI_CAP_SUPPORTS_DETACH_KERNEL_DRIVER 0x00020000 -#define USBI_CAP_HAS_POLLABLE_DEVICE_FD 0x00040000 /* Maximum number of bytes in a log line */ #define USBI_MAX_LOG_LEN 1024 @@ -269,15 +268,6 @@ struct libusb_context { struct list_head flying_transfers; usbi_mutex_t flying_transfers_lock; - /* 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; - struct pollfd *pollfds; - POLL_NFDS_TYPE pollfds_cnt; - unsigned int pollfds_modified; - usbi_mutex_t pollfds_lock; - /* user callbacks for pollfd changes */ libusb_pollfd_added_cb fd_added_cb; libusb_pollfd_removed_cb fd_removed_cb; @@ -289,6 +279,11 @@ struct libusb_context { /* used to see if there is an active thread doing event handling */ int event_handler_active; + /* used to wait for event completion in threads other than the one that is + * event handling */ + usbi_mutex_t event_waiters_lock; + usbi_cond_t event_waiters_cond; + /* A lock to protect internal context event data. */ usbi_mutex_t event_data_lock; @@ -296,18 +291,18 @@ struct libusb_context { * in order to safely close a device. Protected by event_data_lock. */ unsigned int device_close; - /* A flag that is set when we want to interrupt event handling, in order to - * pick up a new fd for polling. Protected by event_data_lock. */ - unsigned int fd_notify; + /* 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. + * Protected by event_data_lock. */ + struct list_head ipollfds; + struct pollfd *pollfds; + POLL_NFDS_TYPE pollfds_cnt; + unsigned int pollfds_modified; /* A list of pending hotplug messages. Protected by event_data_lock. */ struct list_head hotplug_msgs; - /* used to wait for event completion in threads other than the one that is - * event handling */ - usbi_mutex_t event_waiters_lock; - usbi_cond_t event_waiters_cond; - #ifdef USBI_TIMERFD_AVAILABLE /* used for timeout handling, if supported by OS. * this timerfd is maintained to trigger on the next pending timeout */ @@ -319,7 +314,7 @@ struct libusb_context { /* Update the following macro if new event sources are added */ #define usbi_pending_events(ctx) \ - ((ctx)->device_close || (ctx)->fd_notify || !list_empty(&(ctx)->hotplug_msgs)) + ((ctx)->device_close || (ctx)->pollfds_modified || !list_empty(&(ctx)->hotplug_msgs)) #ifdef USBI_TIMERFD_AVAILABLE #define usbi_using_timerfd(ctx) ((ctx)->timerfd >= 0) @@ -422,9 +417,6 @@ enum usbi_transfer_flags { /* Operation on the transfer failed because the device disappeared */ USBI_TRANSFER_DEVICE_DISAPPEARED = 1 << 3, - - /* Set by backend submit_transfer() if the fds in use have been updated */ - USBI_TRANSFER_UPDATED_FDS = 1 << 4, }; #define USBI_TRANSFER_TO_LIBUSB_TRANSFER(transfer) \ diff --git a/libusb/os/darwin_usb.c b/libusb/os/darwin_usb.c index ed5b2ad..19174b1 100644 --- a/libusb/os/darwin_usb.c +++ b/libusb/os/darwin_usb.c @@ -1963,7 +1963,7 @@ static int darwin_free_streams (struct libusb_device_handle *dev_handle, unsigne const struct usbi_os_backend darwin_backend = { .name = "Darwin", - .caps = USBI_CAP_HAS_POLLABLE_DEVICE_FD, + .caps = 0, .init = darwin_init, .exit = darwin_exit, .get_device_list = NULL, /* not needed */ diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c index d92e3dd..63f4105 100644 --- a/libusb/os/linux_usbfs.c +++ b/libusb/os/linux_usbfs.c @@ -2645,7 +2645,7 @@ static clockid_t op_get_timerfd_clockid(void) const struct usbi_os_backend linux_usbfs_backend = { .name = "Linux usbfs", - .caps = USBI_CAP_HAS_HID_ACCESS|USBI_CAP_SUPPORTS_DETACH_KERNEL_DRIVER|USBI_CAP_HAS_POLLABLE_DEVICE_FD, + .caps = USBI_CAP_HAS_HID_ACCESS|USBI_CAP_SUPPORTS_DETACH_KERNEL_DRIVER, .init = op_init, .exit = op_exit, .get_device_list = NULL, diff --git a/libusb/os/netbsd_usb.c b/libusb/os/netbsd_usb.c index 7b0abf9..eaac21c 100644 --- a/libusb/os/netbsd_usb.c +++ b/libusb/os/netbsd_usb.c @@ -90,7 +90,7 @@ static int _access_endpoint(struct libusb_transfer *); const struct usbi_os_backend netbsd_backend = { "Synchronous NetBSD backend", - USBI_CAP_HAS_POLLABLE_DEVICE_FD, + 0, NULL, /* init() */ NULL, /* exit() */ netbsd_get_device_list, diff --git a/libusb/os/openbsd_usb.c b/libusb/os/openbsd_usb.c index c1e786c..93dfda1 100644 --- a/libusb/os/openbsd_usb.c +++ b/libusb/os/openbsd_usb.c @@ -93,7 +93,7 @@ static int _bus_open(int); const struct usbi_os_backend openbsd_backend = { "Synchronous OpenBSD backend", - USBI_CAP_HAS_POLLABLE_DEVICE_FD, + 0, NULL, /* init() */ NULL, /* exit() */ obsd_get_device_list, diff --git a/libusb/os/wince_usb.c b/libusb/os/wince_usb.c index bd699b7..fa8389c 100644 --- a/libusb/os/wince_usb.c +++ b/libusb/os/wince_usb.c @@ -668,7 +668,6 @@ static int wince_submit_control_or_bulk_transfer(struct usbi_transfer *itransfer return libusbErr; } usbi_add_pollfd(ctx, transfer_priv->pollable_fd.fd, direction_in ? POLLIN : POLLOUT); - itransfer->flags |= USBI_TRANSFER_UPDATED_FDS; return LIBUSB_SUCCESS; } diff --git a/libusb/os/windows_usb.c b/libusb/os/windows_usb.c index 0ce0dfc..a36ab45 100644 --- a/libusb/os/windows_usb.c +++ b/libusb/os/windows_usb.c @@ -2085,7 +2085,6 @@ static int submit_bulk_transfer(struct usbi_transfer *itransfer) usbi_add_pollfd(ctx, transfer_priv->pollable_fd.fd, (short)(IS_XFERIN(transfer) ? POLLIN : POLLOUT)); - itransfer->flags |= USBI_TRANSFER_UPDATED_FDS; return LIBUSB_SUCCESS; } @@ -2105,7 +2104,6 @@ static int submit_iso_transfer(struct usbi_transfer *itransfer) usbi_add_pollfd(ctx, transfer_priv->pollable_fd.fd, (short)(IS_XFERIN(transfer) ? POLLIN : POLLOUT)); - itransfer->flags |= USBI_TRANSFER_UPDATED_FDS; return LIBUSB_SUCCESS; } @@ -2124,7 +2122,6 @@ static int submit_control_transfer(struct usbi_transfer *itransfer) usbi_add_pollfd(ctx, transfer_priv->pollable_fd.fd, POLLIN); - itransfer->flags |= USBI_TRANSFER_UPDATED_FDS; return LIBUSB_SUCCESS; } -- 2.7.4