[PATCH 0/3] tests: Fix some deadlocks

[PATCH 0/3] tests: Fix some deadlocks

[Richard Henderson]

The first patch addresses the fact that linux-user blocks all signals
while attempting to handle guest signals (e.g. ABRT), which means that
the default TERM sent by timeout has no effect -- KILL instead.

When a guest aborts (possible nios2 guest bug), sending SIGABRT to
itself, I have caught one thread waiting in start_exclusive, and all
of the others blocked in exclusive_idle.  I look at that and think
there's a bug in our start_exclusive locking, but I can't prove it.

I also wonder whether qemu_plugin_user_exit really needs it, and since
that's the only place that seems to have issues at present, whether
it is in fact using it incorrectly.

Finally, I think we're missing at least a few early tests for
"no plugins registered", like this one, which ought to be fast path,
where the function need do no work whatsoever -- possibly including
taking the plugin lock, but I can't prove that either.

Anyway, this has improved make check-tcg -jX, for large X, which
appears to aggravate things.


r~


Richard Henderson (3):
  tests/tcg: Use SIGKILL for timeout
  plugins: Avoid deadlock in qemu_plugin_user_exit
  plugins: Iterate on cb_lists in qemu_plugin_user_exit

 plugins/core.c            | 13 +++++++------
 tests/tcg/Makefile.target |  4 ++--
 2 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/3] tests/tcg: Use SIGKILL for timeout

[Richard Henderson]

There are some tests for which SIGTERM appears insufficient.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/tcg/Makefile.target | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
index 14bc013181..a3b0aaf8af 100644
--- a/tests/tcg/Makefile.target
+++ b/tests/tcg/Makefile.target
@@ -54,10 +54,10 @@ cc-option = if $(call cc-test, $1); then \
 
 # $1 = test name, $2 = cmd, $3 = desc
 ifeq ($(filter %-softmmu, $(TARGET)),)
-run-test = $(call quiet-command, timeout --foreground $(TIMEOUT) $2 > $1.out, \
+run-test = $(call quiet-command, timeout -s KILL --foreground $(TIMEOUT) $2 > $1.out, \
 	TEST,$(or $3, $*, $<) on $(TARGET_NAME))
 else
-run-test = $(call quiet-command, timeout --foreground $(TIMEOUT) $2, \
+run-test = $(call quiet-command, timeout -s KILL --foreground $(TIMEOUT) $2, \
         TEST,$(or $3, $*, $<) on $(TARGET_NAME))
 endif
 
-- 
2.34.1

Re: [PATCH 1/3] tests/tcg: Use SIGKILL for timeout

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> There are some tests for which SIGTERM appears insufficient.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  tests/tcg/Makefile.target | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
> index 14bc013181..a3b0aaf8af 100644
> --- a/tests/tcg/Makefile.target
> +++ b/tests/tcg/Makefile.target
> @@ -54,10 +54,10 @@ cc-option = if $(call cc-test, $1); then \
>  
>  # $1 = test name, $2 = cmd, $3 = desc
>  ifeq ($(filter %-softmmu, $(TARGET)),)
> -run-test = $(call quiet-command, timeout --foreground $(TIMEOUT) $2 > $1.out, \
> +run-test = $(call quiet-command, timeout -s KILL --foreground $(TIMEOUT) $2 > $1.out, \
>  	TEST,$(or $3, $*, $<) on $(TARGET_NAME))
>  else
> -run-test = $(call quiet-command, timeout --foreground $(TIMEOUT) $2, \
> +run-test = $(call quiet-command, timeout -s KILL --foreground $(TIMEOUT) $2, \
>          TEST,$(or $3, $*, $<) on $(TARGET_NAME))
>  endif

I'll queue this directly into testing/next, thanks. Is this to deal with
the hanging tests on the s390x box?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/3] tests/tcg: Use SIGKILL for timeout

[Richard Henderson]

On 1/16/23 23:36, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> There are some tests for which SIGTERM appears insufficient.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   tests/tcg/Makefile.target | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
>> index 14bc013181..a3b0aaf8af 100644
>> --- a/tests/tcg/Makefile.target
>> +++ b/tests/tcg/Makefile.target
>> @@ -54,10 +54,10 @@ cc-option = if $(call cc-test, $1); then \
>>   
>>   # $1 = test name, $2 = cmd, $3 = desc
>>   ifeq ($(filter %-softmmu, $(TARGET)),)
>> -run-test = $(call quiet-command, timeout --foreground $(TIMEOUT) $2 > $1.out, \
>> +run-test = $(call quiet-command, timeout -s KILL --foreground $(TIMEOUT) $2 > $1.out, \
>>   	TEST,$(or $3, $*, $<) on $(TARGET_NAME))
>>   else
>> -run-test = $(call quiet-command, timeout --foreground $(TIMEOUT) $2, \
>> +run-test = $(call quiet-command, timeout -s KILL --foreground $(TIMEOUT) $2, \
>>           TEST,$(or $3, $*, $<) on $(TARGET_NAME))
>>   endif
> 
> I'll queue this directly into testing/next, thanks. Is this to deal with
> the hanging tests on the s390x box?

No, I saw this hang on x86_64 host, nios2 guest.

Perhaps copy the text from the cover letter, about process_pending_signals blocking all 
signals, and remaining blocked while dump_core_and_abort deadlocked.


r~

[PATCH 2/3] plugins: Avoid deadlock in qemu_plugin_user_exit

[Richard Henderson]

Use of start_exclusive on this exit path leads to deadlock,
in particular when called from dump_core_and_abort.  There
does not appear to be a need for it.

While we're at it, skip the entire function if no plugins.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 plugins/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/plugins/core.c b/plugins/core.c
index ccb770a485..35aca0266d 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -502,7 +502,9 @@ void qemu_plugin_user_exit(void)
 
     QEMU_LOCK_GUARD(&plugin.lock);
 
-    start_exclusive();
+    if (QTAILQ_EMPTY(&plugin.ctxs)) {
+        return;
+    }
 
     /* un-register all callbacks except the final AT_EXIT one */
     for (ev = 0; ev < QEMU_PLUGIN_EV_MAX; ev++) {
@@ -520,8 +522,6 @@ void qemu_plugin_user_exit(void)
         qemu_plugin_disable_mem_helpers(cpu);
     }
 
-    end_exclusive();
-
     /* now it's safe to handle the exit case */
     qemu_plugin_atexit_cb();
 }
-- 
2.34.1

Re: [PATCH 2/3] plugins: Avoid deadlock in qemu_plugin_user_exit

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Use of start_exclusive on this exit path leads to deadlock,
> in particular when called from dump_core_and_abort.  There
> does not appear to be a need for it.

We don't want to be doing any translation while un-registering things
lest things get confused. You could split the patch in two though as the
early return seems reasonable.

>
> While we're at it, skip the entire function if no plugins.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  plugins/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/plugins/core.c b/plugins/core.c
> index ccb770a485..35aca0266d 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -502,7 +502,9 @@ void qemu_plugin_user_exit(void)
>  
>      QEMU_LOCK_GUARD(&plugin.lock);
>  
> -    start_exclusive();
> +    if (QTAILQ_EMPTY(&plugin.ctxs)) {
> +        return;
> +    }
>  
>      /* un-register all callbacks except the final AT_EXIT one */
>      for (ev = 0; ev < QEMU_PLUGIN_EV_MAX; ev++) {
> @@ -520,8 +522,6 @@ void qemu_plugin_user_exit(void)
>          qemu_plugin_disable_mem_helpers(cpu);
>      }
>  
> -    end_exclusive();
> -
>      /* now it's safe to handle the exit case */
>      qemu_plugin_atexit_cb();
>  }


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 3/3] plugins: Iterate on cb_lists in qemu_plugin_user_exit

[Richard Henderson]

Rather than iterate over all plugins for all events,
iterate over plugins that have registered a given event.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 plugins/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/plugins/core.c b/plugins/core.c
index 35aca0266d..f22f8edc74 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -509,9 +509,10 @@ void qemu_plugin_user_exit(void)
     /* un-register all callbacks except the final AT_EXIT one */
     for (ev = 0; ev < QEMU_PLUGIN_EV_MAX; ev++) {
         if (ev != QEMU_PLUGIN_EV_ATEXIT) {
-            struct qemu_plugin_ctx *ctx;
-            QTAILQ_FOREACH(ctx, &plugin.ctxs, entry) {
-                plugin_unregister_cb__locked(ctx, ev);
+            struct qemu_plugin_cb *cb, *next;
+
+            QLIST_FOREACH_SAFE_RCU(cb, &plugin.cb_lists[ev], entry, next) {
+                plugin_unregister_cb__locked(cb->ctx, ev);
             }
         }
     }
-- 
2.34.1

Re: [PATCH 3/3] plugins: Iterate on cb_lists in qemu_plugin_user_exit

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Rather than iterate over all plugins for all events,
> iterate over plugins that have registered a given event.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Queued to plugins/next, thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro