[PATCH v2 0/3] Rework main and side threads interaction in rpc
From: "Fima Shevrin" <efim.shevrin@virtuozzo.com> RPC client implementation uses the following paradigm. The critical section is organized via virObjectLock(client)/virObjectUnlock(client) braces. Though this is potentially problematic as main thread: side thread: virObjectUnlock(client); virObjectLock(client); g_main_loop_quit(client->eventLoop); virObjectUnlock(client); g_main_loop_run(client->eventLoop); This means in particular that is the main thread is executing very long request like VM migration, the wakeup from the side thread could be stuck until the main request will be fully completed. Discrubed case is easily reproducible with the simple python scripts doing slow and fast requests in parallel from two different threads. Our idea is to release the lock at the prepare stage and avoid libvirt stuck during the interaction between main and side threads. Changes: Add cover letter and versions From 2d1a5f094101808216970dbf90383ffeadae7fe9 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" <den@openvz.org> Date: Sat, 25 Nov 2023 14:11:56 +0300 Subject: [PATCH v2 1/3] rpc: mark GSourceFuncs functions in vireventglibwatch.c as static They are not exported from the module and thus should be static. Signed-off-by: Denis V. Lunev <den@openvz.org> Signed-off-by: Fima Shevrin <efim.shevrin@virtuozzo.com> --- src/util/vireventglibwatch.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c index b7f3a8786a..b21e505731 100644 --- a/src/util/vireventglibwatch.c +++ b/src/util/vireventglibwatch.c @@ -71,7 +71,7 @@ virEventGLibFDSourceFinalize(GSource *source G_GNUC_UNUSED) } -GSourceFuncs virEventGLibFDSourceFuncs = { +static GSourceFuncs virEventGLibFDSourceFuncs = { .prepare = virEventGLibFDSourcePrepare, .check = virEventGLibFDSourceCheck, .dispatch = virEventGLibFDSourceDispatch, @@ -194,7 +194,7 @@ virEventGLibSocketSourceFinalize(GSource *source) } -GSourceFuncs virEventGLibSocketSourceFuncs = { +static GSourceFuncs virEventGLibSocketSourceFuncs = { .prepare = virEventGLibSocketSourcePrepare, .check = virEventGLibSocketSourceCheck, .dispatch = virEventGLibSocketSourceDispatch, -- 2.34.1
The ability to use virObjectLockable allows to unlock an object at the prepare stage inside the Main Event Loop. Co-authored-by: Denis V. Lunev <den@openvz.org> Co-authored-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> Signed-off-by: Fima Shevrin <efim.shevrin@virtuozzo.com> --- src/rpc/virnetclient.c | 6 +++--- src/util/vireventglib.c | 4 ++-- src/util/vireventglibwatch.c | 15 ++++++++++++--- src/util/vireventglibwatch.h | 5 ++++- 4 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 4ab8af68c5..de8ebc2da9 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -895,7 +895,7 @@ virNetClientTLSHandshake(virNetClient *client) source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock), ev, client->eventCtx, - virNetClientIOEventTLS, client, NULL); + virNetClientIOEventTLS, client, NULL, NULL); return TRUE; } @@ -990,7 +990,7 @@ int virNetClientSetTLSSession(virNetClient *client, source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock), G_IO_IN, client->eventCtx, - virNetClientIOEventTLSConfirm, client, NULL); + virNetClientIOEventTLSConfirm, client, NULL, NULL); #ifndef WIN32 /* Block SIGWINCH from interrupting poll in curses programs */ @@ -1695,7 +1695,7 @@ static int virNetClientIOEventLoop(virNetClient *client, source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock), ev, client->eventCtx, - virNetClientIOEventFD, &data, NULL); + virNetClientIOEventFD, &data, NULL, NULL); /* Release lock while poll'ing so other threads * can stuff themselves on the queue */ diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c index 023dc37445..fd348eaa05 100644 --- a/src/util/vireventglib.c +++ b/src/util/vireventglib.c @@ -149,7 +149,7 @@ virEventGLibHandleAdd(int fd, if (events != 0) { data->source = virEventGLibAddSocketWatch( - fd, cond, NULL, virEventGLibHandleDispatch, data, NULL); + fd, cond, NULL, virEventGLibHandleDispatch, data, NULL, NULL); } g_ptr_array_add(handles, data); @@ -217,7 +217,7 @@ virEventGLibHandleUpdate(int watch, } data->source = virEventGLibAddSocketWatch( - data->fd, cond, NULL, virEventGLibHandleDispatch, data, NULL); + data->fd, cond, NULL, virEventGLibHandleDispatch, data, NULL, NULL); data->events = events; VIR_DEBUG("Added new handle source=%p", data->source); diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c index b21e505731..7680656ba2 100644 --- a/src/util/vireventglibwatch.c +++ b/src/util/vireventglibwatch.c @@ -29,6 +29,7 @@ struct virEventGLibFDSource { GPollFD pollfd; int fd; GIOCondition condition; + virObjectLockable *client; }; @@ -80,7 +81,8 @@ static GSourceFuncs virEventGLibFDSourceFuncs = { GSource *virEventGLibCreateSocketWatch(int fd, - GIOCondition condition) + GIOCondition condition, + virObjectLockable *client) { GSource *source; virEventGLibFDSource *ssource; @@ -95,6 +97,8 @@ GSource *virEventGLibCreateSocketWatch(int fd, ssource->pollfd.fd = fd; ssource->pollfd.events = condition | G_IO_HUP | G_IO_ERR; + ssource->client = client; + g_source_add_poll(source, &ssource->pollfd); return source; @@ -114,6 +118,7 @@ struct virEventGLibSocketSource { HANDLE event; int revents; GIOCondition condition; + virObjectLockable *client; }; @@ -203,7 +208,8 @@ static GSourceFuncs virEventGLibSocketSourceFuncs = { GSource *virEventGLibCreateSocketWatch(int fd, - GIOCondition condition) + GIOCondition condition, + virObjectLockable *client) { GSource *source; virEventGLibSocketSource *ssource; @@ -221,6 +227,8 @@ GSource *virEventGLibCreateSocketWatch(int fd, ssource->pollfd.fd = (gintptr)ssource->event; ssource->pollfd.events = G_IO_IN; + ssource->client = client; + WSAEventSelect(ssource->socket, ssource->event, FD_READ | FD_ACCEPT | FD_CLOSE | FD_CONNECT | FD_WRITE | FD_OOB); @@ -239,11 +247,12 @@ virEventGLibAddSocketWatch(int fd, GMainContext *context, virEventGLibSocketFunc func, gpointer opaque, + virObjectLockable *client, GDestroyNotify notify) { GSource *source = NULL; - source = virEventGLibCreateSocketWatch(fd, condition); + source = virEventGLibCreateSocketWatch(fd, condition, client); g_source_set_callback(source, (GSourceFunc)func, opaque, notify); g_source_attach(source, context); diff --git a/src/util/vireventglibwatch.h b/src/util/vireventglibwatch.h index f57be1f503..87a48f158d 100644 --- a/src/util/vireventglibwatch.h +++ b/src/util/vireventglibwatch.h @@ -21,6 +21,7 @@ #pragma once #include "internal.h" +#include "virobject.h" /** * virEventGLibCreateSocketWatch: @@ -34,7 +35,8 @@ * Returns: the new main loop source */ GSource *virEventGLibCreateSocketWatch(int fd, - GIOCondition condition); + GIOCondition condition, + virObjectLockable *client); typedef gboolean (*virEventGLibSocketFunc)(int fd, GIOCondition condition, @@ -45,5 +47,6 @@ GSource *virEventGLibAddSocketWatch(int fd, GMainContext *context, virEventGLibSocketFunc func, gpointer opaque, + virObjectLockable *client, GDestroyNotify notify) G_GNUC_WARN_UNUSED_RESULT; -- 2.34.1
RPC client implementation uses the following paradigm. The critical section is organized via virObjectLock(client)/virObjectUnlock(client) braces. Though this is potentially problematic as main thread: side thread: virObjectUnlock(client); virObjectLock(client); g_main_loop_quit(client->eventLoop); virObjectUnlock(client); g_main_loop_run(client->eventLoop); This means in particular that is the main thread is executing very long request like VM migration, the wakeup from the side thread could be stuck until the main request will be fully completed. Discrubed case is easily reproducible with the simple python scripts doing slow and fast requests in parallel from two different threads. Our idea is to release the lock at the prepare stage and avoid libvirt stuck during the interaction between main and side threads. Co-authored-by: Denis V. Lunev <den@openvz.org> Co-authored-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> Signed-off-by: Fima Shevrin <efim.shevrin@virtuozzo.com> --- src/rpc/virnetclient.c | 17 ++++++++++++----- src/util/vireventglibwatch.c | 28 ++++++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index de8ebc2da9..63bd42ed3a 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -987,6 +987,9 @@ int virNetClientSetTLSSession(virNetClient *client, * etc. If we make the grade, it will send us a '\1' byte. */ + /* Here we are not passing the client to virEventGLibAddSocketWatch, + * since the entire virNetClientSetTLSSession function requires a lock. + */ source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock), G_IO_IN, client->eventCtx, @@ -1692,14 +1695,18 @@ static int virNetClientIOEventLoop(virNetClient *client, if (client->nstreams) ev |= G_IO_IN; + /* + * We don't need to call virObjectLock(client) here, + * since the .prepare function inside glib Main Loop + * will do this. virEventGLibAddSocketWatch is responsible + * for passing client var in glib .prepare + */ source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock), ev, client->eventCtx, - virNetClientIOEventFD, &data, NULL, NULL); - - /* Release lock while poll'ing so other threads - * can stuff themselves on the queue */ - virObjectUnlock(client); + virNetClientIOEventFD, &data, + (virObjectLockable *)client, + NULL); #ifndef WIN32 /* Block SIGWINCH from interrupting poll in curses programs, diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c index 7680656ba2..641b772995 100644 --- a/src/util/vireventglibwatch.c +++ b/src/util/vireventglibwatch.c @@ -34,11 +34,23 @@ struct virEventGLibFDSource { static gboolean -virEventGLibFDSourcePrepare(GSource *source G_GNUC_UNUSED, +virEventGLibFDSourcePrepare(GSource *source, gint *timeout) { + virEventGLibFDSource *ssource = (virEventGLibFDSource *)source; *timeout = -1; + if (ssource->client != NULL) + virObjectUnlock(ssource->client); + + /* + * Prepare function may be called multiple times + * in glib Main Loop, thus we assign source->client + * a null pointer to avoid calling pthread_mutex_unlock + * on an already unlocked mutex. + * */ + ssource->client = NULL; + return FALSE; } @@ -123,11 +135,23 @@ struct virEventGLibSocketSource { static gboolean -virEventGLibSocketSourcePrepare(GSource *source G_GNUC_UNUSED, +virEventGLibSocketSourcePrepare(GSource *source, gint *timeout) { + virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source; *timeout = -1; + if (ssource->client != NULL) + virObjectUnlock(ssource->client); + + /* + * Prepare function may be called multiple times + * in glib Main Loop, thus we assign source->client + * a null pointer to avoid calling pthread_mutex_unlock + * on an already unlocked mutex. + * */ + ssource->client = NULL; + return FALSE; } -- 2.34.1
participants (1)
-
Fima Shevrin