[Spice-devel] [PATCH v5 05/13] usbredir: Spawn a different thread for device redirection flow

[Dmitry Fleytman]

> On 9 Feb 2016, at 23:43 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> 
> On Thu, 2015-10-29 at 17:27 +0200, Dmitry Fleytman wrote:
>> From: Kirill Moizik <kmoizik at redhat.com>
>> 
>> On Windows when using usbdk, opening and closing USB device handle,
>> i.e. calling libusb_open()/libusb_unref_device() can block for a few
>> seconds (3-5 second more specifically on patch author's HW).
>> 
>> libusb_open() is called by spice_usbredir_channel_open_device().
>> 
>> libusb_unref_device() is called implicitely via
>> usbredirhost_set_device(..., NULL) from
>> spice_usbredir_channel_disconnect_device().
>> 
>> Both these calls happen on the main thread. If it blocks,
>> this causes the UI to freeze.
>> 
>> This commit makes sure libusb_open() is called from a different
>> thread to avoid blocking the mainloop freezes when usbdk is used.
>> 
>> Following commits also move usbredirhost_set_device(..., NULL) call
>> to the different threads.
>> 
>> Since this commit introduces additional execution contexts running in
>> parallell to the main thread threre are thread safety concerns to be secured.
>> Mainly there are 3 types of objects accessed by newly introduced threads:
>>  1. libusb contexts
>>  2. usbredir context
>>  3. redirection channels
>> 
>> Fortunately libusb accesses either thread safe of already performed by
>> a separate thread and protected by locks as needed.
>> 
>> As for channels and usbredir, in order to achieve thread safety this patch
>> introduces
>> additional locks and references, namely:
>> 
>> 1. To make sure channel objects are not disposed while redirection in
>> progress,
>>   they are referenced on flow start and unreferenced on flow completion;
> 
> I don't see this anywhere in this patch
> 
>> 2. Channel objects data accesses from different threads protected with a
>>   new lock (flows_mutex);
> 
> This refers to the earlier patches.
> 
>> 3. Handling usbredir messages protected by the same new lock in order to
>>   ensure there are no messages processing flows in progres when device gets
>>   connected or disconneced.
> 
> is this also referring to a previous patch?

Indeed, this commit message is referring to an earlier series and became outdated.
Fixed in the next version.

> 
>> 
>> Signed-off-by: Kirill Moizik <kmoizik at redhat.com>
>> Signed-off-by: Dmitry Fleytman <dfleytma at redhat.com>
>> ---
>> src/channel-usbredir.c | 41 ++++++++++++++++++++++++++++++-----------
>> 1 file changed, 30 insertions(+), 11 deletions(-)
>> 
>> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
>> index c88322a..302a2aa 100644
>> --- a/src/channel-usbredir.c
>> +++ b/src/channel-usbredir.c
>> @@ -315,6 +315,27 @@ static void spice_usbredir_channel_open_acl_cb(
>> }
>> #endif
>> 
>> +#ifndef USE_POLKIT
>> +static void
>> +_open_device_async_cb(GSimpleAsyncResult *simple,
>> +                      GObject *object,
>> +                      GCancellable *cancellable)
>> +{
>> +    GError *err = NULL;
>> +    SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(object);
>> +    SpiceUsbredirChannelPrivate *priv = channel->priv;
>> +    g_mutex_lock(priv->flows_mutex);
>> +    if (!spice_usbredir_channel_open_device(channel, &err)) {
>> +        g_simple_async_result_take_error(simple, err);
>> +        libusb_unref_device(priv->device);
>> +        priv->device = NULL;
>> +        g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
>> +        priv->spice_device = NULL;
>> +     }
>> +    g_mutex_unlock(priv->flows_mutex);
>> +}
>> +#endif
>> +
>> G_GNUC_INTERNAL
>> void spice_usbredir_channel_connect_device_async(
>>                                           SpiceUsbredirChannel *channel,
>> @@ -324,15 +345,14 @@ void spice_usbredir_channel_connect_device_async(
>>                                           GAsyncReadyCallback   callback,
>>                                           gpointer              user_data)
>> {
>> -    SpiceUsbredirChannelPrivate *priv = channel->priv;
>> +    SpiceUsbredirChannelPrivate *priv;
>>     GSimpleAsyncResult *result;
>> -#if ! USE_POLKIT
>> -    GError *err = NULL;
>> -#endif
>> 
>>     g_return_if_fail(SPICE_IS_USBREDIR_CHANNEL(channel));
>>     g_return_if_fail(device != NULL);
>> 
>> +    priv = channel->priv;
>> +
> 
> I would just drop this hunk. 


Dropped, thanks.

> 
>>     CHANNEL_DEBUG(channel, "connecting device %04x:%04x (%p) to channel %p",
>>                   spice_usb_device_get_vid(spice_device),
>>                   spice_usb_device_get_pid(spice_device),
>> @@ -372,13 +392,12 @@ void spice_usbredir_channel_connect_device_async(
>>                                   channel);
>>     return;
>> #else
>> -    if (!spice_usbredir_channel_open_device(channel, &err)) {
>> -        g_simple_async_result_take_error(result, err);
>> -        libusb_unref_device(priv->device);
>> -        priv->device = NULL;
>> -        g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
>> -        priv->spice_device = NULL;
>> -    }
>> +    g_simple_async_result_run_in_thread(result,
>> +                                        _open_device_async_cb,
>> +                                        G_PRIORITY_DEFAULT,
>> +                                        cancellable);
>> +    g_object_unref(result);
>> +    return;
>> #endif
>> 
>> done:
> 
> 
> The code looks fine, but I think the commit log needs to be revised a little bit
> due to (presumably) patches being split


Done.

> 
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com <mailto:jjongsma at redhat.com>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160228/3cf41230/attachment-0001.html>