Hi, On 07/22/2012 01:27 PM, Pete Batard wrote: > On 2012.07.13 01:27, Pete Batard wrote: >> What will happen then is I'll submit a proposal to this list with all >> the enhancements we talked about sometime next week > > Here we go. > > I'm still not entirely sure disarming the timerfd in handle_timerfd_trigger() > is that beneficial
Hmm, I hadn't noticed the explicit disarm there, since the handle_timeouts_locked code just cancels transfers and does not do anything io-intensive, the run-time for handle_timeouts_locked will be short, so I see no value in the disarm there, and vote for removing it. > The other item I'm also pondering about is the aborting of the transfer > submission in libusb_submit_transfer(), > if an error returned from add_to_flying_list(itransfer) as the only error > currently returned by this call is if we failed to arm the timer, > which isn't consider critical. Then again, if we alter add_to_flying_list() > in the future, we may return more consequential errors... I think this behavior is fine, arming the timer fd really is something which should never fail, so aborting if it does is no problem. So on to the review of the actual patch: >>From 198bea16c161289f6caba857219677f7be56fe16 Mon Sep 17 00:00:00 2001 > From: Pete Batard<p...@akeo.ie> > Date: Tue, 10 Jul 2012 01:59:45 +0100 > Subject: [PATCH] Core: Fix unconditional disarming of timerfd > > * Existing code appears disarms the timerfd always, which cancels > pending timeouts as soon as one packet completes. > * This fix moves the disarming of the timerfd to > arm_timerfd_for_next_timeout(), where it is now done conditionally. > * This patch also ensures that all handling of the timerfd is done > under the flying transfers lock. > * Issue reported by Hans de Goede. For more info, see: > https://sourceforge.net/mailarchive/message.php?msg_id=29442693 > > --- > libusb/io.c | 73 > ++++++++++++++++++++++++--------------------------- > 1 file changed, 36 insertions(+), 39 deletions(-) > > diff --git a/libusb/io.c b/libusb/io.c > index b8d445c..e034a6e 100644 > --- a/libusb/io.c > +++ b/libusb/io.c > @@ -1159,7 +1159,7 @@ static int add_to_flying_list(struct usbi_transfer > *transfer) > struct timeval *timeout = &transfer->timeout; > struct libusb_context *ctx = ITRANSFER_CTX(transfer); > int r = 0; > - int first = 1; > + int first = 0; > > usbi_mutex_lock(&ctx->flying_transfers_lock); > > @@ -1167,7 +1167,7 @@ static int add_to_flying_list(struct usbi_transfer > *transfer) > if (list_empty(&ctx->flying_transfers)) { > list_add(&transfer->list, &ctx->flying_transfers); > if (timerisset(timeout)) > - r = 1; > + first = 1; > goto out; > } > > @@ -1186,15 +1186,32 @@ static int add_to_flying_list(struct usbi_transfer > *transfer) > (cur_tv->tv_sec == timeout->tv_sec && > cur_tv->tv_usec > timeout->tv_usec)) { > list_add_tail(&transfer->list, &cur->list); > - r = first; > + first = 1; > goto out; > } > - first = 0; > } > > /* otherwise we need to be inserted at the end */ > list_add_tail(&transfer->list, &ctx->flying_transfers); > out: > +#ifdef USBI_TIMERFD_AVAILABLE The changes to the usage of first / r above introduce a bug, if the transfer being added to the list is not the first one added, first will now still get set to 1, causing us to re-arm the timerfd to a timeout of a transfer which times-out later, which we of course should not do. I suggest just keeping the above code the same and changing: > + if (first && usbi_using_timerfd(ctx)) { To: > + if (r == 1 && usbi_using_timerfd(ctx)) { > + /* if this transfer has the lowest timeout of all active > transfers, > + * rearm the timerfd with this transfer's timeout */ > + const struct itimerspec it = { {0, 0}, > + { transfer->timeout.tv_sec, transfer->timeout.tv_usec * > 1000 } }; > + usbi_dbg("arm timerfd for timeout in %dms (first in line)", > + USBI_TRANSFER_TO_LIBUSB_TRANSFER(transfer)->timeout); > + r = timerfd_settime(ctx->timerfd, TFD_TIMER_ABSTIME, &it, NULL); > + if (r < 0) { > + usbi_warn(ctx, "failed to arm first timerfd (error > %d)", r); > + r = LIBUSB_ERROR_OTHER; > + } > + } > +#else > + UNUSED(first); > +#endif > + > usbi_mutex_unlock(&ctx->flying_transfers_lock); > return r; > } > @@ -1290,7 +1307,6 @@ int API_EXPORTED libusb_submit_transfer(struct > libusb_transfer *transfer) > struct usbi_transfer *itransfer = > LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer); > int r; > - int first; > int updated_fds; > > usbi_mutex_lock(&itransfer->lock); > @@ -1302,27 +1318,15 @@ int API_EXPORTED libusb_submit_transfer(struct > libusb_transfer *transfer) > goto out; > } > > - first = add_to_flying_list(itransfer); > + r = add_to_flying_list(itransfer); > + if (r) > + goto out; > r = usbi_backend->submit_transfer(itransfer); > if (r) { > usbi_mutex_lock(&ctx->flying_transfers_lock); > list_del(&itransfer->list); > usbi_mutex_unlock(&ctx->flying_transfers_lock); > } We should call arm_timerfd_for_next_timeout() after the list_del here to adjust the timer for the changes to the list. > -#ifdef USBI_TIMERFD_AVAILABLE > - else if (first && usbi_using_timerfd(ctx)) { > - /* if this transfer has the lowest timeout of all active > transfers, > - * rearm the timerfd with this transfer's timeout */ > - const struct itimerspec it = { {0, 0}, > - { itransfer->timeout.tv_sec, itransfer->timeout.tv_usec > * 1000 } }; > - usbi_dbg("arm timerfd for timeout in %dms (first in line)", > transfer->timeout); > - r = timerfd_settime(ctx->timerfd, TFD_TIMER_ABSTIME, &it, NULL); > - if (r < 0) > - r = LIBUSB_ERROR_OTHER; > - } > -#else > - (void)first; > -#endif > > out: > updated_fds = (itransfer->flags & USBI_TRANSFER_UPDATED_FDS); > @@ -1402,7 +1406,7 @@ static int arm_timerfd_for_next_timeout(struct > libusb_context *ctx) > /* if we've reached transfers of infinite timeout, then we have > no > * arming to do */ > if (!timerisset(cur_tv)) > - return 0; > + goto disarm; > > /* act on first transfer that is not already cancelled */ > if (!(transfer->flags & USBI_TRANSFER_TIMED_OUT)) { > @@ -1417,14 +1421,10 @@ static int arm_timerfd_for_next_timeout(struct > libusb_context *ctx) > } > } > > - return 0; > +disarm: > + return disarm_timerfd(ctx); > } > #else > -static int disarm_timerfd(struct libusb_context *ctx) > -{ > - (void)ctx; > - return 0; > -} > static int arm_timerfd_for_next_timeout(struct libusb_context *ctx) > { > (void)ctx; > @@ -1456,17 +1456,14 @@ int usbi_handle_transfer_completion(struct > usbi_transfer *itransfer, > > usbi_mutex_lock(&ctx->flying_transfers_lock); > list_del(&itransfer->list); > - if (usbi_using_timerfd(ctx)) > - r = arm_timerfd_for_next_timeout(ctx); > - usbi_mutex_unlock(&ctx->flying_transfers_lock); > - > if (usbi_using_timerfd(ctx)) { > - if (r < 0) > - return r; > - r = disarm_timerfd(ctx); > - if (r < 0) > + r = arm_timerfd_for_next_timeout(ctx); > + if (r < 0) { > + usbi_mutex_unlock(&ctx->flying_transfers_lock); > return r; > + } > } > + usbi_mutex_unlock(&ctx->flying_transfers_lock);\ Maybe move the if (r < 0) return r; block outside of the locked code block, and avoid having the unlock twice? > > if (status == LIBUSB_TRANSFER_COMPLETED > && transfer->flags & LIBUSB_TRANSFER_SHORT_NOT_OK) { > @@ -1829,11 +1826,11 @@ static int handle_timerfd_trigger(struct > libusb_context *ctx) > { > int r; > > + usbi_mutex_lock(&ctx->flying_transfers_lock); > + > r = disarm_timerfd(ctx); > if (r < 0) > - return r; > - > - usbi_mutex_lock(&ctx->flying_transfers_lock); > + goto out; > > /* process the timeout that just happened */ > r = handle_timeouts_locked(ctx); Thanks for working on this! Regards, Hans ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
