[PATCH 1/3] rpc: mark GSourceFuncs functions in vireventglibwatch.c as static
From: "Denis V. Lunev" <den@openvz.org> 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
On Fri, Dec 15, 2023 at 02:32:19AM +0800, Fima Shevrin wrote:
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.
Can you explain this in more detail, with call traces illustration for the two threads. You're not saying where the main thread is doing work with the 'client' lock hold for a long time. Generally the goal should be for the main thread to only hold the lock for a short time. Also if the side thread is already holding a reference on 'client', then potentially we should consider if it is possible to terminate the event loop without acquiring the mutex, as GMainLoop protects itself wrt concurrent usage already, provided all threads hold a reference directly or indirectly.
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 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Hello, here are call traces with two threads generated by python script // ============================== [root@dandreev-vz-6-0-0-243-master libvirt]# gdb -p 288985 (gdb) t a a bt Thread 2 (Thread 0x7f2112862640 (LWP 288986) "python3"): #0 0x00007f2121d4296f in poll () at /lib64/libc.so.6 #1 0x00007f211444650c in g_main_context_iterate.constprop () at /lib64/libglib-2.0.so.0 #2 0x00007f21143f1483 in g_main_loop_run () at /lib64/libglib-2.0.so.0 #3 0x00007f211406800b in virNetClientIOEventLoop () at /lib64/libvirt.so.0 #4 0x00007f2114068a0a in virNetClientIO () at /lib64/libvirt.so.0 #5 0x00007f21140692c1 in virNetClientSendInternal () at /lib64/libvirt.so.0 #6 0x00007f211406936d in virNetClientSendWithReply () at /lib64/libvirt.so.0 #7 0x00007f2114061a1d in virNetClientProgramCall () at /lib64/libvirt.so.0 #8 0x00007f21140f79ac in callFull () at /lib64/libvirt.so.0 #9 0x00007f21140f7a2f in call () at /lib64/libvirt.so.0 #10 0x00007f21140d8435 in remoteDomainCreate () at /lib64/libvirt.so.0 #11 0x00007f21141dd60c in virDomainCreate () at /lib64/libvirt.so.0 #12 0x00007f21145c8114 in libvirt_virDomainCreate () at /usr/lib64/python3.9/site-packages/libvirtmod.cpython-39-x86_64-linux-gnu.so #13 0x00007f21221268a8 in cfunction_call () at /lib64/libpython3.9.so.1.0 #14 0x00007f2122118814 in _PyObject_MakeTpCall () at /lib64/libpython3.9.so.1.0 #15 0x00007f212211566e in _PyEval_EvalFrameDefault () at /lib64/libpython3.9.so.1.0 #16 0x00007f212211d013 in function_code_fastcall () at /lib64/libpython3.9.so.1.0 #17 0x00007f2122110650 in _PyEval_EvalFrameDefault () at /lib64/libpython3.9.so.1.0 #18 0x00007f212211d013 in function_code_fastcall () at /lib64/libpython3.9.so.1.0 #19 0x00007f21221103e8 in _PyEval_EvalFrameDefault () at /lib64/libpython3.9.so.1.0 #20 0x00007f212211d013 in function_code_fastcall () at /lib64/libpython3.9.so.1.0 #21 0x00007f21221133d2 in _PyEval_EvalFrameDefault () at /lib64/libpython3.9.so.1.0 #22 0x00007f212211d013 in function_code_fastcall () at /lib64/libpython3.9.so.1.0 #23 0x00007f2122110650 in _PyEval_EvalFrameDefault () at /lib64/libpython3.9.so.1.0 #24 0x00007f212211d013 in function_code_fastcall () at /lib64/libpython3.9.so.1.0 #25 0x00007f2122110650 in _PyEval_EvalFrameDefault () at /lib64/libpython3.9.so.1.0 #26 0x00007f212211d013 in function_code_fastcall () at /lib64/libpython3.9.so.1.0 #27 0x00007f2122125382 in method_vectorcall () at /lib64/libpython3.9.so.1.0 #28 0x00007f21221d8c4a in t_bootstrap () at /lib64/libpython3.9.so.1.0 #29 0x00007f21221d8bf8 in pythread_wrapper () at /lib64/libpython3.9.so.1.0 #30 0x00007f2121c9f802 in start_thread () at /lib64/libc.so.6 #31 0x00007f2121c3f450 in clone3 () at /lib64/libc.so.6 Thread 1 (Thread 0x7f21223cf740 (LWP 288985) "python3"): #0 0x00007f2121c9c39a in __futex_abstimed_wait_common () at /lib64/libc.so.6 #1 0x00007f2121c9eba0 in pthread_cond_wait@@GLIBC_2.3.2 () at /lib64/libc.so.6 #2 0x00007f2113f4982a in virCondWait () at /lib64/libvirt.so.0 #3 0x00007f2113f1fee3 in virObjectWait () at /lib64/libvirt.so.0 #4 0x00007f211406882a in virNetClientIO () at /lib64/libvirt.so.0 #5 0x00007f21140692c1 in virNetClientSendInternal () at /lib64/libvirt.so.0 #6 0x00007f211406936d in virNetClientSendWithReply () at /lib64/libvirt.so.0 #7 0x00007f2114061a1d in virNetClientProgramCall () at /lib64/libvirt.so.0 #8 0x00007f21140f79ac in callFull () at /lib64/libvirt.so.0 #9 0x00007f21140f7a2f in call () at /lib64/libvirt.so.0 #10 0x00007f21140f24eb in remoteNodeDeviceNumOfCaps () at /lib64/libvirt.so.0 #11 0x00007f2114207a00 in virNodeDeviceNumOfCaps () at /lib64/libvirt.so.0 #12 0x00007f21145d8edf in libvirt_virNodeDeviceListCaps.lto_priv.0 () at /usr/lib64/python3.9/site-packages/libvirtmod.cpython-39-x86_64-linux-gnu.so #13 0x00007f21221268a8 in cfunction_call () at /lib64/libpython3.9.so.1.0 #14 0x00007f2122118814 in _PyObject_MakeTpCall () at /lib64/libpython3.9.so.1.0 #15 0x00007f212211566e in _PyEval_EvalFrameDefault () at /lib64/libpython3.9.so.1.0 #16 0x00007f212211d013 in function_code_fastcall () at /lib64/libpython3.9.so.1.0 #17 0x00007f2122110650 in _PyEval_EvalFrameDefault () at /lib64/libpython3.9.so.1.0 #18 0x00007f212210f06d in _PyEval_EvalCode () at /lib64/libpython3.9.so.1.0 #19 0x00007f212218c495 in _PyEval_EvalCodeWithName () at /lib64/libpython3.9.so.1.0 --Type <RET> for more, q to quit, c to continue without paging-- #20 0x00007f212218c42d in PyEval_EvalCodeEx () at /lib64/libpython3.9.so.1.0 #21 0x00007f212218c3df in PyEval_EvalCode () at /lib64/libpython3.9.so.1.0 #22 0x00007f21221b7524 in run_eval_code_obj () at /lib64/libpython3.9.so.1.0 #23 0x00007f21221b5da6 in run_mod () at /lib64/libpython3.9.so.1.0 #24 0x00007f212208f0cb in pyrun_file.cold () at /lib64/libpython3.9.so.1.0 #25 0x00007f21221bb253 in PyRun_SimpleFileExFlags () at /lib64/libpython3.9.so.1.0 #26 0x00007f21221b7ee8 in Py_RunMain () at /lib64/libpython3.9.so.1.0 #27 0x00007f212217f02d in Py_BytesMain () at /lib64/libpython3.9.so.1.0 #28 0x00007f2121c3feb0 in __libc_start_call_main () at /lib64/libc.so.6 #29 0x00007f2121c3ff60 in __libc_start_main_impl () at /lib64/libc.so.6 #30 0x000056013181a095 in _start () // ========================================================================= just in case here is python script [root@dandreev-vz-6-0-0-243-master ~]# cat a.py import libvirt import time from threading import Thread def startVM(connection, vm_name): try: # Find the virtual machine by name print('starting VM') connection.lookupByName(vm_name).create() print('done starting VM') except libvirt.libvirtError as e: print(f'Libvirt Error: {e}') # Replace 'qemu+tcp://10.34.66.13/system' with your actual connection URI connection_uri = 'qemu+tcp://localhost/system' connection = libvirt.open(connection_uri) if connection is None: print(f'Failed to open connection to {connection_uri}') exit(1) try: # Replace 'your_vm_name' with the actual name of your virtual machine # startVM(connection, 'instance-00000002') thread = Thread(target=lambda: startVM(connection, 'instance-00000002')) thread.start() time.sleep(3) devs = connection.listAllDevices() for i in range(100000): for dev in devs: print('listing caps for dev %i %s' % (i, dev)) try: dev.listCaps() except Exception as e: print('ERROR: %s' % e) print('DONE listing caps') connection.lookupByName('instance-00000002').destroy() except: connection.lookupByName('instance-00000002').destroy() finally: thread.join() # Close the connection outside the function connection.close() // ================================================================= We use virtual machine startup as a loaded operation, but migration or something similar could be used. Fima Shevrin ________________________________ From: Daniel P. Berrangé <berrange@redhat.com> Sent: Friday, December 15, 2023 19:09 To: Efim Shevrin <efim.shevrin@virtuozzo.com> Cc: devel@lists.libvirt.org <devel@lists.libvirt.org>; den@openvz.org <den@openvz.org> Subject: Re: [PATCH 3/3] rpc: Rework rpc notifications in main and side thread [You don't often get email from berrange@redhat.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] On Fri, Dec 15, 2023 at 02:32:19AM +0800, Fima Shevrin wrote:
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.
Can you explain this in more detail, with call traces illustration for the two threads. You're not saying where the main thread is doing work with the 'client' lock hold for a long time. Generally the goal should be for the main thread to only hold the lock for a short time. Also if the side thread is already holding a reference on 'client', then potentially we should consider if it is possible to terminate the event loop without acquiring the mutex, as GMainLoop protects itself wrt concurrent usage already, provided all threads hold a reference directly or indirectly.
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 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Dec 15, 2023 at 02:32:19AM +0800, Fima Shevrin wrote:
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.
Ah, I understand this now. The flag set from g_main_loop_quit() is ignored and overwritten by g_main_loop_run(). So the interruption from the side thread never happens. Your approach to solving this is by delaying the virObjectUnlock call until g_main_loop_run is running, which is clever but I don't much like playing games with the mutex locking. We need a way to interrupt the main loop, even if it hasn't started running yet. The previous impl in libvirt used a pipe for this trick, effectively as a dummy event source that would be watched. I think we can do the same here, though without needing an actual pipe which causes Windows portability problems. Glib already has an internal mechansim for breaking out of poll(), via its (private) GWakeup object which encapsulates a pipe. We just need a way of triggering this mechanism. I believe this can be achieved using just an idle source ie static gboolean dummy_cb(void *opaque) { return G_SOURCE_REMOVE; } ... GSource *dummy = g_idle_source_new() g_source_set_callback(dummy, dummy_cb, NULL, NULL); g_source_attach(dummy, client->eventCtx); The g_source_attach() method is what breaks the main loop out of its poll() sleep. If it wasn't currently in poll, it is effectively a no-op.
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 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Dec 18, 2023 at 10:32:07AM +0000, Daniel P. Berrangé wrote:
On Fri, Dec 15, 2023 at 02:32:19AM +0800, Fima Shevrin wrote:
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.
Ah, I understand this now. The flag set from g_main_loop_quit() is ignored and overwritten by g_main_loop_run(). So the interruption from the side thread never happens.
Your approach to solving this is by delaying the virObjectUnlock call until g_main_loop_run is running, which is clever but I don't much like playing games with the mutex locking.
We need a way to interrupt the main loop, even if it hasn't started running yet. The previous impl in libvirt used a pipe for this trick, effectively as a dummy event source that would be watched.
I think we can do the same here, though without needing an actual pipe which causes Windows portability problems.
Glib already has an internal mechansim for breaking out of poll(), via its (private) GWakeup object which encapsulates a pipe. We just need a way of triggering this mechanism.
I believe this can be achieved using just an idle source ie
static gboolean dummy_cb(void *opaque) {
Opps, would still need a g_main_loop_quit() call here
return G_SOURCE_REMOVE; }
...
GSource *dummy = g_idle_source_new() g_source_set_callback(dummy, dummy_cb, NULL, NULL); g_source_attach(dummy, client->eventCtx);
The g_source_attach() method is what breaks the main loop out of its poll() sleep. If it wasn't currently in poll, it is effectively a no-op.
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 12/18/23 12:03, Daniel P. Berrangé wrote:
On Mon, Dec 18, 2023 at 10:32:07AM +0000, Daniel P. Berrangé wrote:
On Fri, Dec 15, 2023 at 02:32:19AM +0800, Fima Shevrin wrote:
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. Ah, I understand this now. The flag set from g_main_loop_quit() is ignored and overwritten by g_main_loop_run(). So the interruption from the side thread never happens.
Your approach to solving this is by delaying the virObjectUnlock call until g_main_loop_run is running, which is clever but I don't much like playing games with the mutex locking.
We need a way to interrupt the main loop, even if it hasn't started running yet. The previous impl in libvirt used a pipe for this trick, effectively as a dummy event source that would be watched.
I think we can do the same here, though without needing an actual pipe which causes Windows portability problems.
Glib already has an internal mechansim for breaking out of poll(), via its (private) GWakeup object which encapsulates a pipe. We just need a way of triggering this mechanism.
I believe this can be achieved using just an idle source ie
static gboolean dummy_cb(void *opaque) { Opps, would still need a g_main_loop_quit() call here
I have sent dirty (but working) series with a pipe approach to come to the decision which approach would be better - with prepare callback or via pipes. I am not against any of them :) We need just to come to the decision which one would be better. Thank you in advance, Den
This is a dirty working code using pipes. Sent for discussion of the approach. Made against our downstream based on libvirt 8.5. Signed-off-by: Denis V. Lunev <den@openvz.org>
This functionality is to be extended, simple call to g_main_loop_quit() is not enough. In order to make changes convinient, the helper is required. Signed-off-by: Denis V. Lunev <den@openvz.org> --- src/rpc/virnetclient.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index d29917df27..d9a508246f 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -150,6 +150,13 @@ void virNetClientSetCloseCallback(virNetClient *client, } +static void +virNetClientIOWakeup(virNetClient *client) +{ + g_main_loop_quit(client->eventLoop); +} + + static void virNetClientIncomingEvent(virNetSocket *sock, int events, void *opaque); @@ -851,7 +858,7 @@ static void virNetClientCloseInternal(virNetClient *client, * queue and close the client because we set client->wantClose. */ if (client->haveTheBuck) { - g_main_loop_quit(client->eventLoop); + virNetClientIOWakeup(client); } else { virNetClientIOEventLoopPassTheBuck(client, NULL); } @@ -918,7 +925,7 @@ virNetClientIOEventTLS(int fd G_GNUC_UNUSED, virNetClient *client = opaque; if (!virNetClientTLSHandshake(client)) - g_main_loop_quit(client->eventLoop); + virNetClientIOWakeup(client); return G_SOURCE_REMOVE; } @@ -931,7 +938,7 @@ virNetClientIOEventTLSConfirm(int fd G_GNUC_UNUSED, { virNetClient *client = opaque; - g_main_loop_quit(client->eventLoop); + virNetClientIOWakeup(client); return G_SOURCE_REMOVE; } @@ -1675,7 +1682,7 @@ virNetClientIOEventFD(int fd G_GNUC_UNUSED, { struct virNetClientIOEventData *data = opaque; data->rev = ev; - g_main_loop_quit(data->client->eventLoop); + virNetClientIOWakeup(data->client); return G_SOURCE_REMOVE; } @@ -2006,8 +2013,7 @@ static int virNetClientIO(virNetClient *client, /* Check to see if another thread is dispatching */ if (client->haveTheBuck) { - /* Force other thread to wakeup from poll */ - g_main_loop_quit(client->eventLoop); + virNetClientIOWakeup(client); /* If we are non-blocking, detach the thread and keep the call in the * queue. */ -- 2.34.1
They are not exported from the module and thus should be static. Signed-off-by: Denis V. Lunev <den@openvz.org> --- 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
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. For us this means that Nova was reported as inaccessible. Anyway, this case is easily reproducible with the simple python scripts doing slow and fast requests in parallel from two different threads. This problem has been introduced with the rework for dropping gnulib inside the following commit: commit 7d4350bcac251bab2ecf85bd19eb1181db87fd07 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Thu Jan 16 11:21:44 2020 +0000 rpc: convert RPC client to use GMainLoop instead of poll The cure is to revert back to old roots and use file descriptor for wakeup notifications. The code written is well suited for Linux while it is completely unclear how it will behave on Windows. Signed-off-by: Denis V. Lunev <den@openvz.org> --- src/rpc/virnetclient.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index d9a508246f..7fff5a7017 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -90,6 +90,7 @@ struct _virNetClient { GMainLoop *eventLoop; GMainContext *eventCtx; + int pipeLoopNotify[2]; /* * List of calls currently waiting for dispatch @@ -150,10 +151,25 @@ void virNetClientSetCloseCallback(virNetClient *client, } +static gboolean +virNetClientIOEventNotify(int fd G_GNUC_UNUSED, + GIOCondition ev G_GNUC_UNUSED, + gpointer opaque) +{ + virNetClient *client = opaque; + char buf[1024]; + + while (saferead(client->pipeLoopNotify[0], buf, sizeof(buf)) > 0); + g_main_loop_quit(client->eventLoop); + + return G_SOURCE_CONTINUE; +} + static void virNetClientIOWakeup(virNetClient *client) { - g_main_loop_quit(client->eventLoop); + char c = 0; + ignore_value(safewrite(client->pipeLoopNotify[1], &c, sizeof(c))); } @@ -305,10 +321,15 @@ static virNetClient *virNetClientNew(virNetSocket *sock, const char *hostname) { virNetClient *client = NULL; + int pipenotify[2] = {-1, -1}; + g_autoptr(GSource) G_GNUC_UNUSED source = NULL; if (virNetClientInitialize() < 0) goto error; + if (virPipeNonBlock(pipenotify) < 0) + goto error; + if (!(client = virObjectLockableNew(virNetClientClass))) goto error; @@ -320,12 +341,25 @@ static virNetClient *virNetClientNew(virNetSocket *sock, client->hostname = g_strdup(hostname); + client->pipeLoopNotify[0] = pipenotify[0]; + client->pipeLoopNotify[1] = pipenotify[1]; + + /* FIXME: good for Unix, buggy for Windows */ + source = virEventGLibAddSocketWatch(pipenotify[0], + G_IO_IN | G_IO_ERR | G_IO_HUP, + client->eventCtx, + virNetClientIOEventNotify, + client, NULL); + PROBE(RPC_CLIENT_NEW, "client=%p sock=%p", client, client->sock); return client; error: + VIR_FORCE_CLOSE(pipenotify[0]); + VIR_FORCE_CLOSE(pipenotify[1]); + virObjectUnref(client); virObjectUnref(sock); return NULL; @@ -759,6 +793,9 @@ void virNetClientDispose(void *obj) g_main_loop_unref(client->eventLoop); g_main_context_unref(client->eventCtx); + VIR_FORCE_CLOSE(client->pipeLoopNotify[0]); + VIR_FORCE_CLOSE(client->pipeLoopNotify[1]); + g_free(client->hostname); if (client->sock) -- 2.34.1
On 12/14/23 19:32, Fima Shevrin wrote:
From: "Denis V. Lunev" <den@openvz.org>
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, Can you pls do submission work properly and carefully.
1. While you are sending more than 1 patch in a row, please write cover-letter aka "PATCH 0/3" with a description of the whole series and motivation of it 2. Please also do not forget to increment version at least inside cover letter. If you are sending patches 2nd time you should use "PATCH v2 0/3" 3. Please track changes inside cover letter, writing what has been changed during this particular submission. Thank you in advance, Den
participants (5)
-
Daniel P. Berrangé -
Denis V. Lunev -
Denis V. Lunev -
Efim Shevrin -
Fima Shevrin