rpc: fix race in waking up client event loop

[PATCH] rpc: fix race in waking up client event loop

Posted by Daniel P. Berrangé 4 months, 2 weeks ago

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é <berrange@redhat.com>
---

 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-leave@lists.libvirt.org

Re: [PATCH] rpc: fix race in waking up client event loop

Posted by Denis V. Lunev 4 months, 2 weeks ago

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é <berrange@redhat.com>
> ---
>
>   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 <den@openvz.org>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH] rpc: fix race in waking up client event loop

Posted by Efim Shevrin 4 months, 2 weeks ago

Hello,

Your patch works on our stand without any freezing.

Tested by: Fima Shevrin <efim.shevrin@virtuozzo.com>
________________________________
From: Daniel P. Berrangé <berrange@redhat.com>
Sent: Monday, December 18, 2023 20:23
To: devel@lists.libvirt.org <devel@lists.libvirt.org>
Cc: Efim Shevrin <efim.shevrin@virtuozzo.com>; Denis V. Lunev <den@openvz.org>; Daniel P. Berrangé <berrange@redhat.com>
Subject: [PATCH] rpc: fix race in waking up client event loop

[You don't often get email from berrange@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é <berrange@redhat.com>
---

 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-leave@lists.libvirt.org

Re: [PATCH] rpc: fix race in waking up client event loop

Posted by Michal Prívozník 4 months, 2 weeks ago

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é <berrange@redhat.com>
> ---
> 
>  src/rpc/virnetclient.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org