[Spice-devel] [PATCH v7 06/14] usbredir: Spawn a different thread for device redirection flow
Jonathon Jongsma
jjongsma at redhat.com
Tue Mar 15 19:31:03 UTC 2016
On Thu, 2016-03-10 at 14:08 -0600, Jonathon Jongsma wrote:
> Looks good now
>
> Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
...Except I rebased this series on top of Fabiano's GTask work, and here's the
patch after switching from GSimpleAsyncResult to GTask:
---------------
>From a6769470dfbf9bcc8243d9db1039b8b1121de7a0 Mon Sep 17 00:00:00 2001
From: Kirill Moizik <kmoizik at redhat.com>
Date: Tue, 8 Mar 2016 16:05:53 +0200
Subject: [PATCH spice-gtk 06/14] usbredir: Spawn a different thread for device
redirection flow
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>
Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
---
src/channel-usbredir.c | 38 ++++++++++++++++++++++++++------------
1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index 7febe0e..f492fc5 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -320,6 +320,31 @@ static void spice_usbredir_channel_open_acl_cb(
}
#endif
+#ifndef USE_POLKIT
+static void
+_open_device_async_cb(GTask *task,
+ gpointer object,
+ gpointer task_data,
+ 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_task_return_error(task, 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,
@@ -331,9 +356,6 @@ void spice_usbredir_channel_connect_device_async(
{
SpiceUsbredirChannelPrivate *priv = channel->priv;
GTask *task;
-#ifndef USE_POLKIT
- GError *err = NULL;
-#endif
g_return_if_fail(SPICE_IS_USBREDIR_CHANNEL(channel));
g_return_if_fail(device != NULL);
@@ -376,15 +398,7 @@ void spice_usbredir_channel_connect_device_async(
channel);
return;
#else
- if (!spice_usbredir_channel_open_device(channel, &err)) {
- g_task_return_error(task, err);
- libusb_unref_device(priv->device);
- priv->device = NULL;
- g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
- priv->spice_device = NULL;
- } else {
- g_task_return_boolean(task, TRUE);
- }
+ g_task_run_in_thread(task, _open_device_async_cb);
#endif
done:
>
> 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:
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list