Re: [PATCH] rpc: fix race in waking up client event loop
On 12/18/23 13:23, Daniel P. Berrangé wrote: > The first thread to issue a client RPC request will own the event > loop execution, sitting in the virNetClientIOEventLoop function. > > It releases the client lock while running: > >virNetClientUnlock() >g_main_loop_run() >virNetClientLock() > > If a second thread arrives with an RPC request, it will queue it > for the first thread to process. To inform the first thread that > there's a new request it calls g_main_loop_quit() to break it out > of the main loop. > > This works if the first thread is in g_main_loop_run() at that > time. There is a small window of opportunity, however, where > the first thread has released the client lock, but not yet got > into g_main_loop_run(). If that happens, the wakeup from the > second thread is lost. > > This patch deals with that by changing the way the wakeup is > performed. Instead of directly calling g_main_loop_quit(), the > second thread creates an idle source to run the quit function > from within the first thread. This guarantees that the first > thread will see the wakeup. > > Signed-off-by: Daniel P. Berrangé > --- > > src/rpc/virnetclient.c | 13 - > 1 file changed, 12 insertions(+), 1 deletion(-) > Reviewed-by: Michal Privoznik Michal ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH] rpc: fix race in waking up client event loop
Hello,
Your patch works on our stand without any freezing.
Tested by: Fima Shevrin
From: Daniel P. Berrangé
Sent: Monday, December 18, 2023 20:23
To: devel@lists.libvirt.org
Cc: Efim Shevrin ; Denis V. Lunev
; Daniel P. Berrangé
Subject: [PATCH] rpc: fix race in waking up client event loop
[You don't often get email from berra...@redhat.com. Learn why this is
important at https://aka.ms/LearnAboutSenderIdentification ]
The first thread to issue a client RPC request will own the event
loop execution, sitting in the virNetClientIOEventLoop function.
It releases the client lock while running:
virNetClientUnlock()
g_main_loop_run()
virNetClientLock()
If a second thread arrives with an RPC request, it will queue it
for the first thread to process. To inform the first thread that
there's a new request it calls g_main_loop_quit() to break it out
of the main loop.
This works if the first thread is in g_main_loop_run() at that
time. There is a small window of opportunity, however, where
the first thread has released the client lock, but not yet got
into g_main_loop_run(). If that happens, the wakeup from the
second thread is lost.
This patch deals with that by changing the way the wakeup is
performed. Instead of directly calling g_main_loop_quit(), the
second thread creates an idle source to run the quit function
from within the first thread. This guarantees that the first
thread will see the wakeup.
Signed-off-by: Daniel P. Berrangé
---
src/rpc/virnetclient.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 4ab8af68c5..68098b1c8d 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1848,6 +1848,15 @@ static void virNetClientIOUpdateCallback(virNetClient
*client,
}
+static gboolean virNetClientIOWakeup(gpointer opaque)
+{
+GMainLoop *loop = opaque;
+
+g_main_loop_quit(loop);
+
+return G_SOURCE_REMOVE;
+}
+
/*
* This function sends a message to remote server and awaits a reply
*
@@ -1925,7 +1934,9 @@ 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);
+GSource *wakeup = g_idle_source_new();
+g_source_set_callback(wakeup, virNetClientIOWakeup, client->eventLoop,
NULL);
+g_source_attach(wakeup, client->eventCtx);
/* If we are non-blocking, detach the thread and keep the call in the
* queue. */
--
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH] rpc: fix race in waking up client event loop
On 12/18/23 13:23, Daniel P. Berrangé wrote:
The first thread to issue a client RPC request will own the event
loop execution, sitting in the virNetClientIOEventLoop function.
It releases the client lock while running:
virNetClientUnlock()
g_main_loop_run()
virNetClientLock()
If a second thread arrives with an RPC request, it will queue it
for the first thread to process. To inform the first thread that
there's a new request it calls g_main_loop_quit() to break it out
of the main loop.
This works if the first thread is in g_main_loop_run() at that
time. There is a small window of opportunity, however, where
the first thread has released the client lock, but not yet got
into g_main_loop_run(). If that happens, the wakeup from the
second thread is lost.
This patch deals with that by changing the way the wakeup is
performed. Instead of directly calling g_main_loop_quit(), the
second thread creates an idle source to run the quit function
from within the first thread. This guarantees that the first
thread will see the wakeup.
Signed-off-by: Daniel P. Berrangé
---
src/rpc/virnetclient.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 4ab8af68c5..68098b1c8d 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1848,6 +1848,15 @@ static void virNetClientIOUpdateCallback(virNetClient
*client,
}
+static gboolean virNetClientIOWakeup(gpointer opaque)
+{
+GMainLoop *loop = opaque;
+
+g_main_loop_quit(loop);
+
+return G_SOURCE_REMOVE;
+}
+
/*
* This function sends a message to remote server and awaits a reply
*
@@ -1925,7 +1934,9 @@ 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);
+GSource *wakeup = g_idle_source_new();
+g_source_set_callback(wakeup, virNetClientIOWakeup, client->eventLoop,
NULL);
+g_source_attach(wakeup, client->eventCtx);
/* If we are non-blocking, detach the thread and keep the call in the
* queue. */
Reviewed-by: Denis V. Lunev
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
