Re: [libusb] [RFC PATCH] Deadlock with some clients

[Dmitry F. <dm...@da...>]

On Aug 19, 2014, at 11:19 AM, Chris Dickens <chr...@gm...> wrote:

> Hi,

Hi Chris,
Thanks for the answer, see my comments below.

Dmitry

> 
> On Sunday, August 17, 2014, Dmitry Fleytman <dm...@da...> wrote:
> 
> So, our questions are:
> 
>   1. Do we understand the code right and the problem takes place indeed.
> 
> There will be no deadlock within or caused by libusb. The sending thread will call usbi_fd_notification(), which will write to the control pipe. If the transfer completion thread is already in handle_events(), it will be interrupted by the control pipe and will release the events lock, thereby allowing the sending thread to obtain it. If the transfer completion thread is not in handle_events(), the lock will be obtained and released before the transfer completion thread can enter. In either case, the events lock in usbi_fd_notification() is only taken long enough to ensure that no other thread is handling events until the most up-to-date list of fds is used. The transfer completion callback won't be called unless events are being handled, and it will only be called by the thread that is handling events, which is holding the events lock. Therefore it is safe to hold the events lock within the transfer completion callback.

Let’s consider the following events flow:

1. THREAD1 usbredir obtains its HOST lock
2. THREAD2 libusb’s completion thread wakes up (without relation to previous step)
3. THREAD2 libusb’s completion thread calls completion callback for some transfer while holding the events lock
4. THREAD2 transfer’s completion callback tries to obtain HOST lock which is locked on step #1
5. THREAD1 thread that obtained HOST lock on step one calls submit transfer API and enters usbi_fd_notification()
6. THREAD1 usbi_fd_notification() tries to obtain events lock locked by completion thread
7. both threads are waiting one for another at this stage

Looks like the problem is symmetrical - both libraries call callbacks of each other while holding their internal locks, each of those libraries has good reasons for that.
Am I missing something here?

> 
> However in your above scenario, there will certainly be deadlock caused by usbredir. usbredir must release its own lock after submitting the transfer, or it will block the transfer completion thread in the transfer completion callback. This is not something that libusb can fix.
> 
>   2. Should we consider an events locking scheme redesign to fix the problem properly
>      i.e. do transfer completion without holding "events" lock
> 
> Transfer completion is, by definition, an event within libusb. Therefore it cannot be handled without holding the events lock, lest two or more threads try to handle completion of the same transfer.

Can’t libusb do “internal” part of transfer completion under the lock and call transfer completion callback without the lock after the transfer removed from internal lists etc.?

> 
>   3. Should the following patch be applied anyway/is it true that usbi_fd_notification()
>      doesn't need to hold the "events" lock.
> 
> No, it is necessary to hold the events lock during usbi_fd_notification(). The reason is that the control pipe will remain triggered until it is cleared, and it is not safe to clear it until it is known that no other thread is trying to handle events. Otherwise the fd notification may go unnoticed. By taking the events lock, this ensures that no thread will attempt to handle events with an outdated list of fds to poll.

Ok, got your point.

> 
> Regards,
> Chris