Per-kml slots_lock will bring some trouble if we want to take all slots_lock of
all the KMLs, especially when we're in a context that we could have taken some
of the KML slots_lock, then we even need to figure out what we've taken and
what we need to take.
Make this simple by merging all KML slots_lock into a single slots lock.
Per-kml slots_lock isn't anything that helpful anyway - so far only x86 has two
address spaces (so, two slots_locks). All the rest archs will be having one
address space always, which means there's actually one slots_lock so it will be
the same as before.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
accel/kvm/kvm-all.c | 32 +++++++++++++++++---------------
include/sysemu/kvm_int.h | 2 --
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index f88a52393fe..94e881f123b 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -174,8 +174,10 @@ typedef struct KVMResampleFd KVMResampleFd;
static QLIST_HEAD(, KVMResampleFd) kvm_resample_fd_list =
QLIST_HEAD_INITIALIZER(kvm_resample_fd_list);
-#define kvm_slots_lock(kml) qemu_mutex_lock(&(kml)->slots_lock)
-#define kvm_slots_unlock(kml) qemu_mutex_unlock(&(kml)->slots_lock)
+static QemuMutex kml_slots_lock;
+
+#define kvm_slots_lock() qemu_mutex_lock(&kml_slots_lock)
+#define kvm_slots_unlock() qemu_mutex_unlock(&kml_slots_lock)
static inline void kvm_resample_fd_remove(int gsi)
{
@@ -241,9 +243,9 @@ bool kvm_has_free_slot(MachineState *ms)
bool result;
KVMMemoryListener *kml = &s->memory_listener;
- kvm_slots_lock(kml);
+ kvm_slots_lock();
result = !!kvm_get_free_slot(kml);
- kvm_slots_unlock(kml);
+ kvm_slots_unlock();
return result;
}
@@ -309,7 +311,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram,
KVMMemoryListener *kml = &s->memory_listener;
int i, ret = 0;
- kvm_slots_lock(kml);
+ kvm_slots_lock();
for (i = 0; i < s->nr_slots; i++) {
KVMSlot *mem = &kml->slots[i];
@@ -319,7 +321,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram,
break;
}
}
- kvm_slots_unlock(kml);
+ kvm_slots_unlock();
return ret;
}
@@ -515,7 +517,7 @@ static int kvm_section_update_flags(KVMMemoryListener *kml,
return 0;
}
- kvm_slots_lock(kml);
+ kvm_slots_lock();
while (size && !ret) {
slot_size = MIN(kvm_max_slot_size, size);
@@ -531,7 +533,7 @@ static int kvm_section_update_flags(KVMMemoryListener *kml,
}
out:
- kvm_slots_unlock(kml);
+ kvm_slots_unlock();
return ret;
}
@@ -819,7 +821,7 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
return ret;
}
- kvm_slots_lock(kml);
+ kvm_slots_lock();
for (i = 0; i < s->nr_slots; i++) {
mem = &kml->slots[i];
@@ -845,7 +847,7 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
}
}
- kvm_slots_unlock(kml);
+ kvm_slots_unlock();
return ret;
}
@@ -1150,7 +1152,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
ram = memory_region_get_ram_ptr(mr) + section->offset_within_region +
(start_addr - section->offset_within_address_space);
- kvm_slots_lock(kml);
+ kvm_slots_lock();
if (!add) {
do {
@@ -1208,7 +1210,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
} while (size);
out:
- kvm_slots_unlock(kml);
+ kvm_slots_unlock();
}
static void kvm_region_add(MemoryListener *listener,
@@ -1235,9 +1237,9 @@ static void kvm_log_sync(MemoryListener *listener,
KVMMemoryListener *kml = container_of(listener, KVMMemoryListener, listener);
int r;
- kvm_slots_lock(kml);
+ kvm_slots_lock();
r = kvm_physical_sync_dirty_bitmap(kml, section);
- kvm_slots_unlock(kml);
+ kvm_slots_unlock();
if (r < 0) {
abort();
}
@@ -1337,7 +1339,7 @@ void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
{
int i;
- qemu_mutex_init(&kml->slots_lock);
+ qemu_mutex_init(&kml_slots_lock);
kml->slots = g_malloc0(s->nr_slots * sizeof(KVMSlot));
kml->as_id = as_id;
diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
index ccb8869f01b..1da30e18841 100644
--- a/include/sysemu/kvm_int.h
+++ b/include/sysemu/kvm_int.h
@@ -27,8 +27,6 @@ typedef struct KVMSlot
typedef struct KVMMemoryListener {
MemoryListener listener;
- /* Protects the slots and all inside them */
- QemuMutex slots_lock;
KVMSlot *slots;
int as_id;
} KVMMemoryListener;
--
2.26.2
Hi Peter,
On 2021/3/11 4:32, Peter Xu wrote:
> Per-kml slots_lock will bring some trouble if we want to take all slots_lock of
> all the KMLs, especially when we're in a context that we could have taken some
> of the KML slots_lock, then we even need to figure out what we've taken and
> what we need to take.
>
> Make this simple by merging all KML slots_lock into a single slots lock.
>
> Per-kml slots_lock isn't anything that helpful anyway - so far only x86 has two
> address spaces (so, two slots_locks). All the rest archs will be having one
> address space always, which means there's actually one slots_lock so it will be
> the same as before.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> accel/kvm/kvm-all.c | 32 +++++++++++++++++---------------
> include/sysemu/kvm_int.h | 2 --
> 2 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index f88a52393fe..94e881f123b 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -174,8 +174,10 @@ typedef struct KVMResampleFd KVMResampleFd;
> static QLIST_HEAD(, KVMResampleFd) kvm_resample_fd_list =
> QLIST_HEAD_INITIALIZER(kvm_resample_fd_list);
>
> -#define kvm_slots_lock(kml) qemu_mutex_lock(&(kml)->slots_lock)
> -#define kvm_slots_unlock(kml) qemu_mutex_unlock(&(kml)->slots_lock)
> +static QemuMutex kml_slots_lock;
> +
> +#define kvm_slots_lock() qemu_mutex_lock(&kml_slots_lock)
> +#define kvm_slots_unlock() qemu_mutex_unlock(&kml_slots_lock)
nit: qemu_mutex_lock and qemu_mutex_unlock is not aligned.
>
> static inline void kvm_resample_fd_remove(int gsi)
> {
> @@ -241,9 +243,9 @@ bool kvm_has_free_slot(MachineState *ms)
> bool result;
> KVMMemoryListener *kml = &s->memory_listener;
>
> - kvm_slots_lock(kml);
> + kvm_slots_lock();
> result = !!kvm_get_free_slot(kml);
> - kvm_slots_unlock(kml);
> + kvm_slots_unlock();
>
> return result;
> }
> @@ -309,7 +311,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram,
> KVMMemoryListener *kml = &s->memory_listener;
> int i, ret = 0;
>
> - kvm_slots_lock(kml);
> + kvm_slots_lock();
> for (i = 0; i < s->nr_slots; i++) {
> KVMSlot *mem = &kml->slots[i];
>
> @@ -319,7 +321,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram,
> break;
> }
> }
> - kvm_slots_unlock(kml);
> + kvm_slots_unlock();
>
> return ret;
> }
> @@ -515,7 +517,7 @@ static int kvm_section_update_flags(KVMMemoryListener *kml,
> return 0;
> }
>
> - kvm_slots_lock(kml);
> + kvm_slots_lock();
>
> while (size && !ret) {
> slot_size = MIN(kvm_max_slot_size, size);
> @@ -531,7 +533,7 @@ static int kvm_section_update_flags(KVMMemoryListener *kml,
> }
>
> out:
> - kvm_slots_unlock(kml);
> + kvm_slots_unlock();
> return ret;
> }
>
> @@ -819,7 +821,7 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
> return ret;
> }
>
> - kvm_slots_lock(kml);
> + kvm_slots_lock();
>
> for (i = 0; i < s->nr_slots; i++) {
> mem = &kml->slots[i];
> @@ -845,7 +847,7 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
> }
> }
>
> - kvm_slots_unlock(kml);
> + kvm_slots_unlock();
>
> return ret;
> }
> @@ -1150,7 +1152,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
> ram = memory_region_get_ram_ptr(mr) + section->offset_within_region +
> (start_addr - section->offset_within_address_space);
>
> - kvm_slots_lock(kml);
> + kvm_slots_lock();
>
> if (!add) {
> do {
> @@ -1208,7 +1210,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
> } while (size);
>
> out:
> - kvm_slots_unlock(kml);
> + kvm_slots_unlock();
> }
>
> static void kvm_region_add(MemoryListener *listener,
> @@ -1235,9 +1237,9 @@ static void kvm_log_sync(MemoryListener *listener,
> KVMMemoryListener *kml = container_of(listener, KVMMemoryListener, listener);
> int r;
>
> - kvm_slots_lock(kml);
> + kvm_slots_lock();
> r = kvm_physical_sync_dirty_bitmap(kml, section);
> - kvm_slots_unlock(kml);
> + kvm_slots_unlock();
> if (r < 0) {
> abort();
> }
> @@ -1337,7 +1339,7 @@ void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
> {
> int i;
>
> - qemu_mutex_init(&kml->slots_lock);
> + 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?
Thanks,
Keqian
> kml->slots = g_malloc0(s->nr_slots * sizeof(KVMSlot));
> kml->as_id = as_id;
>
> diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
> index ccb8869f01b..1da30e18841 100644
> --- a/include/sysemu/kvm_int.h
> +++ b/include/sysemu/kvm_int.h
> @@ -27,8 +27,6 @@ typedef struct KVMSlot
>
> typedef struct KVMMemoryListener {
> MemoryListener listener;
> - /* Protects the slots and all inside them */
> - QemuMutex slots_lock;
> KVMSlot *slots;
> int as_id;
> } KVMMemoryListener;
>
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. Paolo
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;
}
--
2.26.2
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
© 2016 - 2025 Red Hat, Inc.