Re: [PATCH v5 02/10] KVM: Use a big lock to replace per-kml slots_lock

[Peter Xu]

On Mon, Mar 22, 2021 at 12:27:54PM -0400, Peter Xu wrote:
> On Mon, Mar 22, 2021 at 02:54:30PM +0100, Paolo Bonzini wrote:
> > On 22/03/21 11:47, Keqian Zhu wrote:
> > > > +    qemu_mutex_init(&kml_slots_lock);
> > > As you said, x86 has two address spaces, is it a problem that we may have 
> > > multi initialization for kml_slots_lock?
> > 
> > Agreed, the lock can be initialized (if only for cleanliness) in kvm_init.
> 
> Definitely, I'm surprised why I didn't see this... :) Paolo, do you want me to
> add another patch (as attached)?
> 
> -- 
> Peter Xu

> From 0cb7124d111426f255113814cb8395620276cc1f Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Mon, 22 Mar 2021 12:25:18 -0400
> Subject: [PATCH] qemu-thread: Assert and check mutex re-initialize
> 
> qemu_mutex_post_init() sets mutex->initialized but not check it yet.  Add it,
> so as to detect re-initialization of mutexes.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  util/qemu-thread-common.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/util/qemu-thread-common.h b/util/qemu-thread-common.h
> index 2af6b120853..e02059845d8 100644
> --- a/util/qemu-thread-common.h
> +++ b/util/qemu-thread-common.h
> @@ -15,6 +15,7 @@
>  
>  #include "qemu/thread.h"
>  #include "trace.h"
> +#include <assert.h>
>  
>  static inline void qemu_mutex_post_init(QemuMutex *mutex)
>  {
> @@ -22,6 +23,7 @@ static inline void qemu_mutex_post_init(QemuMutex *mutex)
>      mutex->file = NULL;
>      mutex->line = 0;
>  #endif
> +    assert(!mutex->initialized);
>      mutex->initialized = true;
>  }

I got coredumps after applying this patch when make check:

#1  0x00007fce1090b895 in __GI_abort
#2  0x00007fce1090b769 in __assert_fail_base
#3  0x00007fce1091ae86 in __GI___assert_fail
#4  0x0000563e3e407248 in qemu_mutex_post_init
#5  0x0000563e3e407491 in qemu_mutex_init
#6  0x0000563e3e410ca4 in rcu_init_complete
#7  0x0000563e3e410e13 in rcu_init_child
#8  0x00007fce1096d60b in __run_fork_handlers
#9  0x00007fce109b42cc in __libc_fork
#10 0x0000563e3e3b5006 in qtest_init_without_qmp_handshake
#11 0x0000563e3e3b51a1 in qtest_init
#12 0x0000563e3e3b116b in test_acpi_one
#13 0x0000563e3e3b1264 in test_acpi_piix4_tcg
#14 0x00007fce10ebe29e in g_test_run_suite_internal
#15 0x00007fce10ebe09b in g_test_run_suite_internal
#16 0x00007fce10ebe09b in g_test_run_suite_internal
#17 0x00007fce10ebe78a in g_test_run_suite
#18 0x00007fce10ebe7a5 in g_test_run
#19 0x0000563e3e3b32b4 in main

Then I saw this commit:

    commit 21b7cf9e07e5991c57b461181cfb5bbb6fe7a9d6
    Author: Paolo Bonzini <pbonzini@redhat.com>
    Date:   Thu Mar 5 16:53:48 2015 +0100

    rcu: handle forks safely
    
    After forking, only the calling thread is duplicated in the child process.
    The call_rcu thread has to be recreated in the child.  Exploit the fact
    that only one thread exists (same as when constructors run), and just redo
    the entire initialization to ensure the threads are in the proper state.
    
    The only additional things to do are emptying the list of threads
    registered with RCU, and unlocking the lock that was taken in the prepare
    callback (implementations are allowed to fail pthread_mutex_init()
    if the mutex is still locked).

It seems we depend on the capability to have pthread_mutex_init() be able to
detect an initialized (especially locked) lock?  I didn't dig deeper yet,
instead for simplicity I'll just drop the extra assertion patch.

Thanks,

-- 
Peter Xu