[v2] tcg plugins pre-PR (deprecations, mem apis, contrib plugins)

[PATCH v2 14/18] util/timer: avoid deadlock when shutting down

Posted by Alex Bennée 10 months ago

When we shut down a guest we disable the timers. However this can
cause deadlock if the guest has queued some async work that is trying
to advance system time and spins forever trying to wind time forward.
Pay attention to the return code and bail early if we can't wind time
forward.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reported-by: Elisha Hollander <just4now666666@gmail.com>
---
 util/qemu-timer.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 213114be68..6b1533bc2a 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -685,10 +685,17 @@ int64_t qemu_clock_advance_virtual_time(int64_t dest)
 {
     int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     AioContext *aio_context;
+    int64_t deadline;
+
     aio_context = qemu_get_aio_context();
-    while (clock < dest) {
-        int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
+
+    deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
                                                       QEMU_TIMER_ATTR_ALL);
+    /*
+     * A deadline of < 0 indicates this timer is not enabled, so we
+     * won't get far trying to run it forward.
+     */
+    while (deadline >= 0 && clock < dest) {
         int64_t warp = qemu_soonest_timeout(dest - clock, deadline);
 
         qemu_virtual_clock_set_ns(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + warp);
@@ -696,6 +703,9 @@ int64_t qemu_clock_advance_virtual_time(int64_t dest)
         qemu_clock_run_timers(QEMU_CLOCK_VIRTUAL);
         timerlist_run_timers(aio_context->tlg.tl[QEMU_CLOCK_VIRTUAL]);
         clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+
+        deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
+                                              QEMU_TIMER_ATTR_ALL);
     }
     qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
 
-- 
2.39.5

Re: [PATCH v2 14/18] util/timer: avoid deadlock when shutting down

Posted by Michael Tokarev 9 months, 2 weeks ago

On 16.09.2024 11:53, Alex Bennée wrote:
> When we shut down a guest we disable the timers. However this can
> cause deadlock if the guest has queued some async work that is trying
> to advance system time and spins forever trying to wind time forward.
> Pay attention to the return code and bail early if we can't wind time
> forward.

I wonder if this is another qemu-stable piece?

Thanks,

/mjt

Re: [PATCH v2 14/18] util/timer: avoid deadlock when shutting down

Posted by Alex Bennée 9 months, 2 weeks ago

Michael Tokarev <mjt@tls.msk.ru> writes:

> On 16.09.2024 11:53, Alex Bennée wrote:
>> When we shut down a guest we disable the timers. However this can
>> cause deadlock if the guest has queued some async work that is trying
>> to advance system time and spins forever trying to wind time forward.
>> Pay attention to the return code and bail early if we can't wind time
>> forward.
>
> I wonder if this is another qemu-stable piece?

I guess. The code that causes this particular mode was added to 9.1.

>
> Thanks,
>
> /mjt

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 14/18] util/timer: avoid deadlock when shutting down

Posted by Pierrick Bouvier 10 months ago

On 9/16/24 01:53, Alex Bennée wrote:
> When we shut down a guest we disable the timers. However this can
> cause deadlock if the guest has queued some async work that is trying
> to advance system time and spins forever trying to wind time forward.
> Pay attention to the return code and bail early if we can't wind time
> forward.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reported-by: Elisha Hollander <just4now666666@gmail.com>
> ---
>   util/qemu-timer.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
> index 213114be68..6b1533bc2a 100644
> --- a/util/qemu-timer.c
> +++ b/util/qemu-timer.c
> @@ -685,10 +685,17 @@ int64_t qemu_clock_advance_virtual_time(int64_t dest)
>   {
>       int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>       AioContext *aio_context;
> +    int64_t deadline;
> +
>       aio_context = qemu_get_aio_context();
> -    while (clock < dest) {
> -        int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
> +
> +    deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
>                                                         QEMU_TIMER_ATTR_ALL);
> +    /*
> +     * A deadline of < 0 indicates this timer is not enabled, so we
> +     * won't get far trying to run it forward.
> +     */
> +    while (deadline >= 0 && clock < dest) {
>           int64_t warp = qemu_soonest_timeout(dest - clock, deadline);
>   
>           qemu_virtual_clock_set_ns(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + warp);
> @@ -696,6 +703,9 @@ int64_t qemu_clock_advance_virtual_time(int64_t dest)
>           qemu_clock_run_timers(QEMU_CLOCK_VIRTUAL);
>           timerlist_run_timers(aio_context->tlg.tl[QEMU_CLOCK_VIRTUAL]);
>           clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +
> +        deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
> +                                              QEMU_TIMER_ATTR_ALL);
>       }
>       qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>   

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>