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
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(-) > Reviewed-by: Michal Privoznik Michal ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH] rpc: fix race in waking up client event loop
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