Attached is my final proposal then.
On 2012.08.03 10:13, Hans de Goede wrote:
That works for me, I agree the original code is hard to read, maybe move
the refactoring to a separate patch though ?
Well, I don't really see it as refactoring, as we're moving a section of
code that used first into a call that didn't use first, so we're simply
adding the first variable there.
I have also now added a check for timerisset in the code moved to
add_to_flying_list(), which simplifies things further.
Hehe, funny I had the exact same thought when reviewing your patch, the
problem is not with the code, but with the confusing list_add_tail function
name, here is docs on list_add_tail:
http://www.compsoc.man.ac.uk/~moz/kernelnewbies/documents/kdoc/kernel-api/r509.html
"Insert a new entry before the specified head. This is useful for
implementing queues. "
So the original code is fine here.
Agreed. I was trying too hard to see bugs where there weren't any.
I've a slight preference for moving the check outside of the locked
context,
since I prefer always having lock and unlock calls balanced 1:1
That's a good rationale, so I went with that too.
Regards,
/Pete
>From 17243129eea6abe7a80da7674cefe4d9ae0b6e2a 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.
It also avoids calling disarm outside of the above call.
* 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 | 66 ++++++++++++++++++++++-----------------------------
1 files changed, 29 insertions(+), 39 deletions(-)
diff --git a/libusb/io.c b/libusb/io.c
index b8d445c..5e8eb62 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -1166,14 +1166,13 @@ static int add_to_flying_list(struct usbi_transfer
*transfer)
/* if we have no other flying transfers, start the list with this one */
if (list_empty(&ctx->flying_transfers)) {
list_add(&transfer->list, &ctx->flying_transfers);
- if (timerisset(timeout))
- r = 1;
goto out;
}
/* if we have infinite timeout, append to end of list */
if (!timerisset(timeout)) {
list_add_tail(&transfer->list, &ctx->flying_transfers);
+ /* first is irrelevant in this case */
goto out;
}
@@ -1186,15 +1185,33 @@ 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;
goto out;
}
first = 0;
}
+ /* first is 0 at this stage (list not empty) */
/* otherwise we need to be inserted at the end */
list_add_tail(&transfer->list, &ctx->flying_transfers);
out:
+#ifdef USBI_TIMERFD_AVAILABLE
+ if (first && usbi_using_timerfd(ctx) && timerisset(timeout)) {
+ /* 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);
}
-#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;
@@ -1459,14 +1459,8 @@ int usbi_handle_transfer_completion(struct usbi_transfer
*itransfer,
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)
- return r;
- }
+ if (usbi_using_timerfd(ctx) && (r < 0))
+ return r;
if (status == LIBUSB_TRANSFER_COMPLETED
&& transfer->flags & LIBUSB_TRANSFER_SHORT_NOT_OK) {
@@ -1829,10 +1823,6 @@ static int handle_timerfd_trigger(struct libusb_context
*ctx)
{
int r;
- r = disarm_timerfd(ctx);
- if (r < 0)
- return r;
-
usbi_mutex_lock(&ctx->flying_transfers_lock);
/* process the timeout that just happened */
--
1.7.11.msysgit.0
------------------------------------------------------------------------------
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
