On 2012.06.22 10:47, Hans de Goede wrote:
> While debugging an usbredir issue I noticed the following in libusb/io.c:
>
> int usbi_handle_transfer_completion(struct usbi_transfer *itransfer,
> enum libusb_transfer_status status)
> {
> ...
>
> /* FIXME: could be more intelligent with the timerfd here. we don't
> nee
> * to disarm the timerfd if there was no timer running, and we only
> nee
> * to rearm the timerfd if the transfer that expired was the one
> with
> * the shortest timeout. */
>
> 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)
> return r;
> }
>
> ...
> }
>
> Now maybe I'm crazy, but should we not first disarm the timerfd, and then
> re-arm it for the next timeout? Because right now the re-arm is a nop as
> it gets canceled out by the later disarm?
I'm trying to wrap my head around it too, but I can't see much of a
point for the above either.
Ignoring what might have been the actual intent, and with the error
check on arm_timerfd_for_next_timeout(), which is tied to a failure of
timerfd_settime(), as an unlikely cause for caution, all the code seems
to be trying to do here is make sure that the timerfd is armed (if
needed) when the flying_transfers_lock mutex is released, and then
unconditionally disarm it again.
Logically, the reason one may want to do that could be to address the
possibility of getting a timeout on another transfer when we unlock
(with a resulting call to usbi_handle_transfer_completion() from another
thread), but since we might very well get the disarm in the first thread
occurring right after the second thread rearmed but before the unlock,
if the goal was to ensure that timerfd was armed on flying_transfers
unlock, we don't even seem to be doing a great job at it...
Without any additional perspective, I'd agree with you that this
unconditional disarming of timerfd is likely to have unwanted consequences.
I think the logic should probably be:
- if there's no other transfer with (non infinite) timeout to wait on,
disarm the timerfd altogether
- if there exists another transfer with (non infinite) timeout, the
timerfd should be armed and set to the transfer with shortest timeout
(which should normally be the first in our flying list).
Also, my understanding is that we can arm with a timeout value that is
in the past (in case the next transfer timed out before we get a chance
to re-arm), and we'll still get the timerfd event. So a possible fix
would be to drop the disarm_timerfd() call altogether, and instead only
disarm in arm_timerfd_for_next_timeout(), after we have exhausted the
flying list and found no updated timerfd reason to rearm.
With arm_timerfd_for_next_timeout() only called with flying list locked,
I think also doing the disarming there when needed should OK.
Then again, maybe someone has an alternate take on what we are actually
trying to achieve here...
Regards,
/Pete
------------------------------------------------------------------------------
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
