From 1787c81fadb1ccadde451094a306537fba4b86e3 Mon Sep 17 00:00:00 2001 From: Victor Toso Date: Thu, 5 Jan 2023 18:25:08 +0100 Subject: [PATCH] Fixes 100% CPU usage with usbredirect as TCP Server Backporting this fix too to avoid reworking too much the upstream patch for the related bug below. Related: https://bugzilla.redhat.com/show_bug.cgi?id=2157520 Signed-off-by: Victor Toso --- ...edirect-structure-to-simplify-some-s.patch | 135 ++++++++++++++++++ ...tor-out-a-function-to-create-watches.patch | 74 ++++++++++ 0004-Recreate-watch-if-needed.patch | 106 ++++++++++++++ usbredir.spec | 9 +- 4 files changed, 323 insertions(+), 1 deletion(-) create mode 100644 0002-Use-typedef-on-redirect-structure-to-simplify-some-s.patch create mode 100644 0003-Factor-out-a-function-to-create-watches.patch create mode 100644 0004-Recreate-watch-if-needed.patch diff --git a/0002-Use-typedef-on-redirect-structure-to-simplify-some-s.patch b/0002-Use-typedef-on-redirect-structure-to-simplify-some-s.patch new file mode 100644 index 0000000..2fc7ab3 --- /dev/null +++ b/0002-Use-typedef-on-redirect-structure-to-simplify-some-s.patch @@ -0,0 +1,135 @@ +From c1246d5d8332890df0dab7b29de86a42c2b7b36a Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Fri, 16 Sep 2022 20:14:28 +0100 +Subject: [PATCH 2/4] Use typedef on redirect structure to simplify some + statements + +Signed-off-by: Frediano Ziglio +--- + tools/usbredirect.c | 26 +++++++++++++------------- + 1 file changed, 13 insertions(+), 13 deletions(-) + +diff --git a/tools/usbredirect.c b/tools/usbredirect.c +index ff910ab..a479c55 100644 +--- a/tools/usbredirect.c ++++ b/tools/usbredirect.c +@@ -22,7 +22,7 @@ + #include + #endif + +-struct redirect { ++typedef struct redirect { + struct { + int vendor; + int product; +@@ -40,7 +40,7 @@ struct redirect { + int watch_server_id; + + GMainLoop *main_loop; +-}; ++} redirect; + + static bool + parse_opt_device(const char *device, int *vendor, int *product) +@@ -125,7 +125,7 @@ parse_opt_uri(const char *uri, char **adr, int *port) + return true; + } + +-static struct redirect * ++static redirect * + parse_opts(int *argc, char ***argv) + { + char *device = NULL; +@@ -133,7 +133,7 @@ parse_opts(int *argc, char ***argv) + char *localaddr = NULL; + gboolean keepalive = FALSE; + gint verbosity = 0; /* none */ +- struct redirect *self = NULL; ++ redirect *self = NULL; + + GOptionEntry entries[] = { + { "device", 0, 0, G_OPTION_ARG_STRING, &device, "Local USB device to be redirected", NULL }, +@@ -162,7 +162,7 @@ parse_opts(int *argc, char ***argv) + goto end; + } + +- self = g_new0(struct redirect, 1); ++ self = g_new0(redirect, 1); + if (!parse_opt_device(device, &self->device.vendor, &self->device.product)) { + g_printerr("Failed to parse device: '%s' - expected: vendor:product or busnum-devnum\n", device); + g_clear_pointer(&self, g_free); +@@ -202,7 +202,7 @@ end: + static gpointer + thread_handle_libusb_events(gpointer user_data) + { +- struct redirect *self = (struct redirect *) user_data; ++ redirect *self = (redirect *) user_data; + + int res = 0; + const char *desc = ""; +@@ -280,7 +280,7 @@ usbredir_log_cb(void *priv, int level, const char *msg) + static int + usbredir_read_cb(void *priv, uint8_t *data, int count) + { +- struct redirect *self = (struct redirect *) priv; ++ redirect *self = (redirect *) priv; + GIOStream *iostream = G_IO_STREAM(self->connection); + GError *err = NULL; + +@@ -308,7 +308,7 @@ usbredir_read_cb(void *priv, uint8_t *data, int count) + static int + usbredir_write_cb(void *priv, uint8_t *data, int count) + { +- struct redirect *self = (struct redirect *) priv; ++ redirect *self = (redirect *) priv; + GIOStream *iostream = G_IO_STREAM(self->connection); + GError *err = NULL; + +@@ -336,7 +336,7 @@ usbredir_write_cb(void *priv, uint8_t *data, int count) + static void + usbredir_write_flush_cb(void *user_data) + { +- struct redirect *self = (struct redirect *) user_data; ++ redirect *self = (redirect *) user_data; + if (!self || !self->usbredirhost) { + return; + } +@@ -387,7 +387,7 @@ usbredir_unlock_lock(void *user_data) + static gboolean + connection_handle_io_cb(GIOChannel *source, GIOCondition condition, gpointer user_data) + { +- struct redirect *self = (struct redirect *) user_data; ++ redirect *self = (redirect *) user_data; + + if (condition & G_IO_ERR || condition & G_IO_HUP) { + g_warning("Connection: err=%d, hup=%d - exiting", (condition & G_IO_ERR), (condition & G_IO_HUP)); +@@ -419,7 +419,7 @@ end: + static gboolean + signal_handler(gpointer user_data) + { +- struct redirect *self = (struct redirect *) user_data; ++ redirect *self = (redirect *) user_data; + g_main_loop_quit(self->main_loop); + return G_SOURCE_REMOVE; + } +@@ -431,7 +431,7 @@ connection_incoming_cb(GSocketService *service, + GObject *source_object, + gpointer user_data) + { +- struct redirect *self = (struct redirect *) user_data; ++ redirect *self = (redirect *) user_data; + self->connection = g_object_ref(client_connection); + + /* Add a GSource watch to handle polling for us and handle IO in the callback */ +@@ -456,7 +456,7 @@ main(int argc, char *argv[]) + goto err_init; + } + +- struct redirect *self = parse_opts(&argc, &argv); ++ redirect *self = parse_opts(&argc, &argv); + if (!self) { + /* specific issues logged in parse_opts() */ + return 1; +-- +2.39.0 + diff --git a/0003-Factor-out-a-function-to-create-watches.patch b/0003-Factor-out-a-function-to-create-watches.patch new file mode 100644 index 0000000..7e27e1c --- /dev/null +++ b/0003-Factor-out-a-function-to-create-watches.patch @@ -0,0 +1,74 @@ +From 307747e2a73cf68a239ddd7b70333bbddf7f3e3b Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Fri, 16 Sep 2022 20:14:28 +0100 +Subject: [PATCH 3/4] Factor out a function to create watches + +--- + tools/usbredirect.c | 37 ++++++++++++++++++++----------------- + 1 file changed, 20 insertions(+), 17 deletions(-) + +diff --git a/tools/usbredirect.c b/tools/usbredirect.c +index a479c55..afe9dee 100644 +--- a/tools/usbredirect.c ++++ b/tools/usbredirect.c +@@ -415,6 +415,24 @@ end: + return G_SOURCE_REMOVE; + } + ++static void ++create_watch(redirect *self) ++{ ++ GSocket *socket = g_socket_connection_get_socket(self->connection); ++ int socket_fd = g_socket_get_fd(socket); ++ GIOChannel *io_channel = ++#ifdef G_OS_UNIX ++ g_io_channel_unix_new(socket_fd); ++#else ++ g_io_channel_win32_new_socket(socket_fd); ++#endif ++ ++ self->watch_server_id = g_io_add_watch(io_channel, ++ G_IO_IN | G_IO_OUT | G_IO_HUP | G_IO_ERR, ++ connection_handle_io_cb, ++ self); ++} ++ + #ifdef G_OS_UNIX + static gboolean + signal_handler(gpointer user_data) +@@ -437,12 +455,7 @@ connection_incoming_cb(GSocketService *service, + /* Add a GSource watch to handle polling for us and handle IO in the callback */ + GSocket *connection_socket = g_socket_connection_get_socket(self->connection); + g_socket_set_keepalive(connection_socket, self->keepalive); +- int socket_fd = g_socket_get_fd(connection_socket); +- GIOChannel *io_channel = g_io_channel_unix_new(socket_fd); +- self->watch_server_id = g_io_add_watch(io_channel, +- G_IO_IN | G_IO_OUT | G_IO_HUP | G_IO_ERR, +- connection_handle_io_cb, +- self); ++ create_watch(self); + return G_SOURCE_REMOVE; + } + +@@ -552,17 +565,7 @@ main(int argc, char *argv[]) + + GSocket *connection_socket = g_socket_connection_get_socket(self->connection); + g_socket_set_keepalive(connection_socket, self->keepalive); +- int socket_fd = g_socket_get_fd(connection_socket); +- GIOChannel *io_channel = +-#ifdef G_OS_UNIX +- g_io_channel_unix_new(socket_fd); +-#else +- g_io_channel_win32_new_socket(socket_fd); +-#endif +- self->watch_server_id = g_io_add_watch(io_channel, +- G_IO_IN | G_IO_OUT | G_IO_HUP | G_IO_ERR, +- connection_handle_io_cb, +- self); ++ create_watch(self); + } else { + GSocketService *socket_service; + +-- +2.39.0 + diff --git a/0004-Recreate-watch-if-needed.patch b/0004-Recreate-watch-if-needed.patch new file mode 100644 index 0000000..91f3a34 --- /dev/null +++ b/0004-Recreate-watch-if-needed.patch @@ -0,0 +1,106 @@ +From 3fcbd4a2569f227ae6fad6a37c8864d33271e5f4 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Sat, 17 Sep 2022 09:28:08 +0100 +Subject: [PATCH 4/4] Recreate watch if needed + +Do not always watch for output buffer. +Watching for output buffer if we don't have nothing to write +(which is the usual case) is consuming a lot of CPU. + +This fixes https://gitlab.freedesktop.org/spice/usbredir/-/issues/24. + +Signed-off-by: Frediano Ziglio +--- + tools/usbredirect.c | 28 ++++++++++++++++++++++++++-- + 1 file changed, 26 insertions(+), 2 deletions(-) + +diff --git a/tools/usbredirect.c b/tools/usbredirect.c +index afe9dee..59452aa 100644 +--- a/tools/usbredirect.c ++++ b/tools/usbredirect.c +@@ -29,6 +29,7 @@ typedef struct redirect { + } device; + bool is_client; + bool keepalive; ++ bool watch_inout; + char *addr; + int port; + int verbosity; +@@ -42,6 +43,8 @@ typedef struct redirect { + GMainLoop *main_loop; + } redirect; + ++static void create_watch(redirect *self); ++ + static bool + parse_opt_device(const char *device, int *vendor, int *product) + { +@@ -163,6 +166,7 @@ parse_opts(int *argc, char ***argv) + } + + self = g_new0(redirect, 1); ++ self->watch_inout = true; + if (!parse_opt_device(device, &self->device.vendor, &self->device.product)) { + g_printerr("Failed to parse device: '%s' - expected: vendor:product or busnum-devnum\n", device); + g_clear_pointer(&self, g_free); +@@ -277,6 +281,20 @@ usbredir_log_cb(void *priv, int level, const char *msg) + g_log_structured(G_LOG_DOMAIN, glog_level, "MESSAGE", msg); + } + ++static void ++update_watch(redirect *self) ++{ ++ const bool watch_inout = usbredirhost_has_data_to_write(self->usbredirhost) != 0; ++ if (watch_inout == self->watch_inout) { ++ return; ++ } ++ g_source_remove(self->watch_server_id); ++ self->watch_server_id = 0; ++ self->watch_inout = watch_inout; ++ ++ create_watch(self); ++} ++ + static int + usbredir_read_cb(void *priv, uint8_t *data, int count) + { +@@ -322,6 +340,7 @@ usbredir_write_cb(void *priv, uint8_t *data, int count) + if (g_error_matches(err, G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK)) { + /* Try again later */ + nbytes = 0; ++ update_watch(self); + } else { + if (err != NULL) { + g_warning("Failure at %s: %s", __func__, err->message); +@@ -401,13 +420,18 @@ connection_handle_io_cb(GIOChannel *source, GIOCondition condition, gpointer use + goto end; + } + } +- if (condition & G_IO_OUT) { ++ // try to write data in any case, to avoid having another iteration and ++ // creation of another watch if there is space in output buffer ++ if (usbredirhost_has_data_to_write(self->usbredirhost) != 0) { + int ret = usbredirhost_write_guest_data(self->usbredirhost); + if (ret < 0) { + g_critical("%s: Failed to write to guest", __func__); + goto end; + } + } ++ ++ // update the watch if needed ++ update_watch(self); + return G_SOURCE_CONTINUE; + + end: +@@ -428,7 +452,7 @@ create_watch(redirect *self) + #endif + + self->watch_server_id = g_io_add_watch(io_channel, +- G_IO_IN | G_IO_OUT | G_IO_HUP | G_IO_ERR, ++ G_IO_IN | G_IO_HUP | G_IO_ERR | (self->watch_inout ? G_IO_OUT : 0), + connection_handle_io_cb, + self); + } +-- +2.39.0 + diff --git a/usbredir.spec b/usbredir.spec index ad7c23f..ed913bd 100644 --- a/usbredir.spec +++ b/usbredir.spec @@ -1,11 +1,14 @@ Name: usbredir Version: 0.13.0 -Release: 1%{?dist} +Release: 2%{?dist} Summary: USB network redirection protocol libraries License: LGPLv2+ URL: https://spice-space.org/usbredir.html Source0: http://spice-space.org/download/%{name}/%{name}-%{version}.tar.xz Patch0001: 0001-Revert-remove-usbredirserver.patch +Patch0002: 0002-Use-typedef-on-redirect-structure-to-simplify-some-s.patch +Patch0003: 0003-Factor-out-a-function-to-create-watches.patch +Patch0004: 0004-Recreate-watch-if-needed.patch BuildRequires: gcc BuildRequires: glib2-devel BuildRequires: libusb1-devel >= 1.0.9 @@ -85,6 +88,10 @@ A simple USB host TCP server, using libusbredirhost. %changelog +* Thu Jan 05 2023 Victor Toso - 0.13.0-2 +- Fixes 100% CPU usage when usbredirect used as TCP server + Related: rhbz#2157520 + * Wed Nov 30 2022 Victor Toso - 0.13.0-1 - Rebase to latest upstream: 0.13.0 - Keeps usbredirserver binary (removed upstream)