Hi,
On 06/26/2013 06:57 PM, Chris Dickens wrote:
> Hi,
>
> On Wed, Jun 26, 2013 at 8:34 AM, Hans de Goede <hdego...@redhat.com
> <mailto:hdego...@redhat.com>> wrote:
>
> Hi,
>
> Hmm, the libusb_poll call in get_device_list() should make this quite
> hard to trigger, this will only
> happen if udev has not yet read the kernel event, and send an event on
> its own socket. Which likely
> means that your setup is somehow cpu starved.
>
>
> Are you talking about the hotplug_poll() call? My system has two Intel Xeon
> X5560 CPUs, for a total of 16 cores. I'd be very surprised if this were a
> resource issue. The application at the time this occurs is running between 6
> and 8 threads, and there are plenty of blocking calls in these threads to
> allow others to be scheduled. To verify this, I added a short timeout (10ms)
> to the linux_udev_event_thread_main() function's poll() call and added a
> usbi_dbg() call in the loop, and I see that the thread is getting scheduled
> regularly before the udev monitor event is received. Given this, I think
> it may be safe to rule out CPU starvation. For whatever reason, the udev
> event is just not being delivered on the socket.
>
> Although I'm not really a fan of having 2 device removed codepaths, I
> think this is probably a
> simple and useful patch. Can you post the patch for review please?
>
>
> diff --git a/libusb/os/linux_netlink.c b/libusb/os/linux_netlink.c
> index 3a68f69..6581ebb 100644
> --- a/libusb/os/linux_netlink.c
> +++ b/libusb/os/linux_netlink.c
> @@ -214,7 +214,7 @@ static int linux_netlink_read_message(void)
> /* signal device is available (or not) to all contexts */
> if (detached)
> -linux_hotplug_disconnected(busnum, devaddr, sys_name);
> +linux_device_disconnected(busnum, devaddr, sys_name);
> else
> linux_hotplug_enumerate(busnum, devaddr, sys_name);
> diff --git a/libusb/os/linux_udev.c b/libusb/os/linux_udev.c
> index 5a2aadf..08b2d70 100644
> --- a/libusb/os/linux_udev.c
> +++ b/libusb/os/linux_udev.c
> @@ -207,7 +207,7 @@ static void udev_hotplug_event(struct udev_device*
> udev_dev)
> if (strncmp(udev_action, "add", 3) == 0) {
> linux_hotplug_enumerate(busnum, devaddr, sys_name);
> } else if (detached) {
> -linux_hotplug_disconnected(busnum, devaddr, sys_name);
> +linux_device_disconnected(busnum, devaddr, sys_name);
> } else {
> usbi_err(NULL, "ignoring udev action %s", udev_action);
> }
> diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c
> index 09288af..50f2d22 100644
> --- a/libusb/os/linux_usbfs.c
> +++ b/libusb/os/linux_usbfs.c
> @@ -1072,7 +1072,7 @@ void linux_hotplug_enumerate(uint8_t busnum, uint8_t
> devaddr, const char *sys_na
> usbi_mutex_static_unlock(&active_contexts_lock);
> }
> -void linux_hotplug_disconnected(uint8_t busnum, uint8_t devaddr, const char
> *sys_name)
> +void linux_device_disconnected(uint8_t busnum, uint8_t devaddr, const char
> *sys_name)
> {
> struct libusb_context *ctx;
> struct libusb_device *dev;
> @@ -1247,8 +1247,18 @@ static int op_open(struct libusb_device_handle *handle)
> int r;
> hpriv->fd = _get_usbfs_fd(handle->dev, O_RDWR, 0);
> -if (hpriv->fd < 0)
> +if (hpriv->fd < 0) {
> +if (hpriv->fd == LIBUSB_ERROR_NO_DEVICE) {
> +/* device will still be marked as attached if hotplug monitor thread
> +* hasn't processed remove event yet */
> +if (handle->dev->attached) {
This check is racy, you need to take the hotplug lock before making this
check, also linux_device_disconnected expects the hotplug lock to be
taken when it is called, so simply but a check around the entire block.
> +usbi_dbg("open failed with no device, but device still attached");
> +linux_device_disconnected(handle->dev->bus_number,
> +handle->dev->device_address, NULL);
> +}
> +}
> return hpriv->fd;
> +}
> r = ioctl(hpriv->fd, IOCTL_USBFS_GET_CAPABILITIES, &hpriv->caps);
> if (r < 0) {
> @@ -2482,6 +2492,11 @@ static int op_handle_events(struct libusb_context *ctx,
> if (pollfd->revents & POLLERR) {
> usbi_remove_pollfd(HANDLE_CTX(handle), hpriv->fd);
> usbi_handle_disconnect(handle);
> +/* device will still be marked as attached if hotplug monitor thread
> +* hasn't processed remove event yet */
> +if (handle->dev->attached)
> +linux_device_disconnected(handle->dev->bus_number,
> +handle->dev->device_address, NULL);
Again the check + call need to happen under the lock.
> continue;
> }
> diff --git a/libusb/os/linux_usbfs.h b/libusb/os/linux_usbfs.h
> index 499bab7..1f5b191 100644
> --- a/libusb/os/linux_usbfs.h
> +++ b/libusb/os/linux_usbfs.h
> @@ -170,7 +170,7 @@ void linux_netlink_hotplug_poll(void);
> #endif
> void linux_hotplug_enumerate(uint8_t busnum, uint8_t devaddr, const char
> *sys_name);
> -void linux_hotplug_disconnected(uint8_t busnum, uint8_t devaddr, const char
> *sys_name);
> +void linux_device_disconnected(uint8_t busnum, uint8_t devaddr, const char
> *sys_name);
> int linux_get_device_address (struct libusb_context *ctx, int detached,
> uint8_t *busnum, uint8_t *devaddr, const char *dev_node,
>
> This sounds like a lot of dark magic for little gain, I would not bother
> with attempting something
> like this, I don't want to add device_disconnect calls to each place were
> we can get a ENOENT or
> ENODEV error, just to speed up detection of device removal by a few ms.
>
>
> My proposal is to only add this check in two places:
>
> 1) In linux op_handle_events()
> 2) In op_open()
>
> In all other places that an ENOENT/ENODEV error can occur, a libusb handle is
> required, so we can assume that the device's fd is already in the poll fd set
> and will be caught by the next thread handling events.
Ok, that makes sense, I can live with having a check in 2 places, please respin
with the suggested
fixes and send with git send-email, or attach, so that your mail client does
not foobar the patch.
Regards,
Hans
------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
