[Spice-devel] [PATCH v7 06/14] usbredir: Spawn a different thread for device redirection flow

[Jonathon Jongsma]

Looks good now

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

On Tue, 2016-03-08 at 16:05 +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 implicitly 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 when usbdk is used.
> 
> Following commits also move usbredirhost_set_device(..., NULL) call
> to separate threads.
> 
> Since this commit introduces additional execution contexts running in
> parallel to the main thread there 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 are either thread safe or already
> performed by a separate thread and protected by locks as needed.
> 
> As for channels and usbredir, in order to achieve thread safety
> additional locks were introduced by previous patches
> in preparation to adding asynchronous contexts:
> 
> 1. Channel objects data accesses from different threads protected with a
>    new lock (device_connect_mutex);
> 2. Handling usbredir messages protected by the same new lock in order to
>    ensure there are no messages processing flows in progress when device gets
>    connected or disconnected.
> 
> Signed-off-by: Kirill Moizik <kmoizik at redhat.com>
> Signed-off-by: Dmitry Fleytman <dfleytma at redhat.com>
> ---
>  src/channel-usbredir.c | 40 ++++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 9491f60..b33122a 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -313,6 +313,30 @@ 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;
> +
> +    spice_usbredir_channel_lock(channel);
> +
> +    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;
> +    }
> +
> +    spice_usbredir_channel_unlock(channel);
> +}
> +#endif
> +
>  G_GNUC_INTERNAL
>  void spice_usbredir_channel_connect_device_async(
>                                            SpiceUsbredirChannel *channel,
> @@ -324,9 +348,6 @@ void spice_usbredir_channel_connect_device_async(
>  {
>      SpiceUsbredirChannelPrivate *priv = channel->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);
> @@ -370,13 +391,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: