[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v4 03/71] cpu: introduce cpu_mutex_lock/unlock
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [RFC v4 03/71] cpu: introduce cpu_mutex_lock/unlock |
Date: |
Mon, 29 Oct 2018 15:54:30 +0000 |
User-agent: |
mu4e 1.1.0; emacs 26.1.50 |
Emilio G. Cota <address@hidden> writes:
> The few direct users of &cpu->lock will be converted soon.
>
> Signed-off-by: Emilio G. Cota <address@hidden>
> ---
> include/qom/cpu.h | 33 +++++++++++++++++++++++++++++++
> cpus.c | 48 +++++++++++++++++++++++++++++++++++++++++++--
> stubs/cpu-lock.c | 20 +++++++++++++++++++
> stubs/Makefile.objs | 1 +
> 4 files changed, 100 insertions(+), 2 deletions(-)
> create mode 100644 stubs/cpu-lock.c
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index b813ca28fa..7fdb5a2be0 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -452,6 +452,39 @@ extern struct CPUTailQ cpus;
>
> extern __thread CPUState *current_cpu;
>
> +/**
> + * cpu_mutex_lock - lock a CPU's mutex
> + * @cpu: the CPU whose mutex is to be locked
> + *
> + * To avoid deadlock, a CPU's mutex must be acquired after the BQL.
> + */
> +#define cpu_mutex_lock(cpu) \
> + cpu_mutex_lock_impl(cpu, __FILE__, __LINE__)
> +void cpu_mutex_lock_impl(CPUState *cpu, const char *file, int line);
> +
> +/**
> + * cpu_mutex_unlock - unlock a CPU's mutex
> + * @cpu: the CPU whose mutex is to be unlocked
> + */
> +#define cpu_mutex_unlock(cpu) \
> + cpu_mutex_unlock_impl(cpu, __FILE__, __LINE__)
> +void cpu_mutex_unlock_impl(CPUState *cpu, const char *file, int line);
> +
> +/**
> + * cpu_mutex_locked - check whether a CPU's mutex is locked
> + * @cpu: the CPU of interest
> + *
> + * Returns true if the calling thread is currently holding the CPU's mutex.
> + */
> +bool cpu_mutex_locked(const CPUState *cpu);
> +
> +/**
> + * no_cpu_mutex_locked - check whether any CPU mutex is held
> + *
> + * Returns true if the calling thread is not holding any CPU mutex.
> + */
> +bool no_cpu_mutex_locked(void);
> +
> static inline void cpu_tb_jmp_cache_clear(CPUState *cpu)
> {
> unsigned int i;
> diff --git a/cpus.c b/cpus.c
> index b2a9698dc0..38cc9e1278 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -83,6 +83,47 @@ static unsigned int throttle_percentage;
> #define CPU_THROTTLE_PCT_MAX 99
> #define CPU_THROTTLE_TIMESLICE_NS 10000000
>
> +/* XXX: is this really the max number of CPUs? */
> +#define CPU_LOCK_BITMAP_SIZE 2048
> +
> +/*
> + * Note: we index the bitmap with cpu->cpu_index + 1 so that the logic
> + * also works during early CPU initialization, when cpu->cpu_index is set to
> + * UNASSIGNED_CPU_INDEX == -1.
> + */
> +static __thread DECLARE_BITMAP(cpu_lock_bitmap, CPU_LOCK_BITMAP_SIZE);
> +
> +bool no_cpu_mutex_locked(void)
> +{
> + return bitmap_empty(cpu_lock_bitmap, CPU_LOCK_BITMAP_SIZE);
> +}
> +
> +void cpu_mutex_lock_impl(CPUState *cpu, const char *file, int line)
> +{
> +/* coverity gets confused by the indirect function call */
> +#ifdef __COVERITY__
> + qemu_mutex_lock_impl(&cpu->lock, file, line);
> +#else
> + QemuMutexLockFunc f = atomic_read(&qemu_mutex_lock_func);
> +
> + g_assert(!cpu_mutex_locked(cpu));
> + set_bit(cpu->cpu_index + 1, cpu_lock_bitmap);
> + f(&cpu->lock, file, line);
> +#endif
> +}
> +
> +void cpu_mutex_unlock_impl(CPUState *cpu, const char *file, int line)
> +{
> + g_assert(cpu_mutex_locked(cpu));
> + qemu_mutex_unlock_impl(&cpu->lock, file, line);
> + clear_bit(cpu->cpu_index + 1, cpu_lock_bitmap);
> +}
> +
> +bool cpu_mutex_locked(const CPUState *cpu)
> +{
> + return test_bit(cpu->cpu_index + 1, cpu_lock_bitmap);
> +}
> +
> bool cpu_is_stopped(CPUState *cpu)
> {
> return cpu->stopped || !runstate_is_running();
> @@ -92,9 +133,9 @@ static inline bool cpu_work_list_empty(CPUState *cpu)
> {
> bool ret;
>
> - qemu_mutex_lock(&cpu->lock);
> + cpu_mutex_lock(cpu);
> ret = QSIMPLEQ_EMPTY(&cpu->work_list);
> - qemu_mutex_unlock(&cpu->lock);
> + cpu_mutex_unlock(cpu);
> return ret;
> }
>
> @@ -1843,6 +1884,9 @@ void qemu_mutex_lock_iothread_impl(const char *file,
> int line)
> {
> QemuMutexLockFunc bql_lock = atomic_read(&qemu_bql_mutex_lock_func);
>
> + /* prevent deadlock with CPU mutex */
> + g_assert(no_cpu_mutex_locked());
> +
> g_assert(!qemu_mutex_iothread_locked());
> bql_lock(&qemu_global_mutex, file, line);
> iothread_locked = true;
> diff --git a/stubs/cpu-lock.c b/stubs/cpu-lock.c
> new file mode 100644
> index 0000000000..7c09af3768
> --- /dev/null
> +++ b/stubs/cpu-lock.c
> @@ -0,0 +1,20 @@
> +#include "qemu/osdep.h"
> +#include "qom/cpu.h"
> +
> +void cpu_mutex_lock_impl(CPUState *cpu, const char *file, int line)
> +{
> +}
> +
> +void cpu_mutex_unlock_impl(CPUState *cpu, const char *file, int line)
> +{
> +}
> +
> +bool cpu_mutex_locked(const CPUState *cpu)
> +{
> + return true;
> +}
> +
> +bool no_cpu_mutex_locked(void)
> +{
> + return true;
> +}
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 53d3f32cb2..fbcdc0256d 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -8,6 +8,7 @@ stub-obj-y += blockdev-close-all-bdrv-states.o
> stub-obj-y += clock-warp.o
> stub-obj-y += cpu-get-clock.o
> stub-obj-y += cpu-get-icount.o
> +stub-obj-y += cpu-lock.o
This is the wrong place because:
c++ -I/usr/include/pixman-1 -Werror -pthread -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fPIE -DPIE -m64 -mcx16
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes
-fno-strict-aliasing -fno-common -fwrapv -Wexpansion-to-defined -Wendif-labels
-Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body
-Wnested-externs -Wformat-security -Wformat-y2k -Winit-self
-Wignored-qualifiers -Wold-style-declaration -Wold-style-definition
-Wtype-limits
-fstack-protector-strong -I/usr/include/p11-kit-1
-I/usr/include/libpng16 -I/usr/include/spice-server -I/usr/include/spice-1
-I/home/alex.bennee/lsrc/qemu.git/capstone/include -I../linux-headers -iquote
.. -iquote /home/alex.bennee/lsrc/qemu.git/target/arm -DNEED_CPU_H -iquote
/home/alex.bennee/lsrc/qemu.git/include
-I/home/alex.bennee/lsrc/qemu.git/linux-user/aarch64
-I/home/alex.bennee/lsrc/qemu.git/linux-user/host/x86_64
-I/home/alex.bennee/lsrc/qemu.git/linux-user -O2 -U_FORTIFY_SOURCE
-D_FORTIFY_SOURCE=2 -g -Wl,--warn-common -lxenctrl -lxenstore -lxenguest
-lxenforeignmemory -lxengnttab -lxenevtchn -lxendevicemodel -Wl,-z,relro
-Wl,-z,now -pie -m64 -g -o qemu-aarch64 exec.o tcg/tcg.o tcg/tcg-op.o
tcg/tcg-op-vec.o tcg/tcg-op-gvec.o tcg/tcg-common.o tcg/optimize.o
fpu/softfloat.o disas.o gdbstub-xml.o gdbstub.o thunk.o accel/stubs/hax-stub.o
accel/stubs/hvf-stub.o accel/stubs/whpx-stub.o accel/stubs/kvm-stub.o
accel/tcg/tcg-runtime.o accel/tcg/tcg-runtime-gvec.o accel/tcg/cpu-exec.o
accel/tcg/cpu-exec-common.o accel/tcg/translate-all.o accel/tcg/translator.o
accel/tcg/user-exec.o accel/tcg/user-exec-stub.o linux-user/main.o
linux-user/syscall.o linux-user/strace.o linux-user/mmap.o linux-user/signal.o
linux-user/elfload.o linux-user/linuxload.o linux-user/uaccess.o
linux-user/uname.o linux-user/safe-syscall.o linux-user/aarch64/signal.o
linux-user/aarch64/cpu_loop.o linux-user/exit.o linux-user/fd-trans.o
linux-user/flatload.o target/arm/arm-semi.o target/arm/kvm-stub.o
target/arm/translate.o target/arm/op_helper.o target/arm/helper.o
target/arm/cpu.o target/arm/neon_helper.o target/arm/iwmmxt_helper.o
target/arm/vec_helper.o target/arm/gdbstub.o target/arm/cpu64.o
target/arm/translate-a64.o target/arm/helper-a64.o target/arm/gdbstub64.o
target/arm/crypto_helper.o target/arm/translate-sve.o target/arm/sve_helper.o
../cpus-common.o ../disas/arm.o ../disas/arm-a64.o ../disas/i386.o
../disas/libvixl/vixl/utils.o ../disas/libvixl/vixl/compiler-intrinsics.o
../disas/libvixl/vixl/a64/instructions-a64.o
../disas/libvixl/vixl/a64/decoder-a64.o ../disas/libvixl/vixl/a64/disasm-a64.o
../hw/core/qdev.o ../hw/core/qdev-properties.o ../hw/core/bus.o
../hw/core/reset.o ../hw/core/irq.o ../hw/core/hotplug.o ../qom/cpu.o
trace/generated-helpers.o trace/control-target.o ../qom/object.o
../qom/container.o ../qom/qom-qobject.o ../qom/object_interfaces.o
../crypto/aes.o ../libqemuutil.a -L/home/alex.bennee/lsrc/qemu.git/capstone
-lcapstone -lm -lgthread-2.0 -pthread -lglib-2.0 -lz -lrt
So we end up linking these stubs in linux-user mode - which I think is
what breaks start_exclusive for the fork in linux-user:
(gdb) l 232
227 exclusive_work_ongoing = true;
228 qemu_mutex_unlock(&qemu_cpu_list_lock);
229
230 /* Make all other cpus stop executing. */
231 CPU_FOREACH(other_cpu) {
232 cpu_mutex_lock(other_cpu);
233 if (other_cpu->running) {
234 g_assert(!other_cpu->exclusive_waiter);
235 other_cpu->exclusive_waiter = true;
236 qemu_cpu_kick(other_cpu);
(gdb) info line 232
Line 232 of "cpus-common.c" starts at address 0x5555556f4527
<start_exclusive+151> and ends at 0x5555556f4540 <start_exclusive+176>.
(gdb) x/10i 0x5555556f4527
0x5555556f4527 <start_exclusive+151>: lea 0xf0bba(%rip),%rbp
# 0x5555557e50e8
0x5555556f452e <start_exclusive+158>: xchg %ax,%ax
0x5555556f4530 <start_exclusive+160>: mov $0xe8,%edx
0x5555556f4535 <start_exclusive+165>: mov %rbp,%rsi
0x5555556f4538 <start_exclusive+168>: mov %rbx,%rdi
0x5555556f453b <start_exclusive+171>: callq 0x555555757b20
<cpu_mutex_lock_impl>
0x5555556f4540 <start_exclusive+176>: cmpb $0x0,0x230(%rbx)
0x5555556f4547 <start_exclusive+183>: je 0x5555556f4587
<start_exclusive+247>
0x5555556f4549 <start_exclusive+185>: cmpb $0x0,0x231(%rbx)
0x5555556f4550 <start_exclusive+192>: je 0x5555556f4578
<start_exclusive+232>
(gdb) x/10i cpu_mutex_lock_impl
0x555555757b20 <cpu_mutex_lock_impl>: repz retq
0x555555757b22: nopl 0x0(%rax)
0x555555757b26: nopw %cs:0x0(%rax,%rax,1)
0x555555757b30 <cpu_mutex_unlock_impl>: repz retq
0x555555757b32: nopl 0x0(%rax)
0x555555757b36: nopw %cs:0x0(%rax,%rax,1)
0x555555757b40 <cpu_mutex_locked>: mov $0x1,%eax
0x555555757b45 <cpu_mutex_locked+5>: retq
0x555555757b46: nopw %cs:0x0(%rax,%rax,1)
0x555555757b50 <no_cpu_mutex_locked>: mov $0x1,%eax
--
Alex Bennée
- Re: [Qemu-devel] [RFC v4 05/71] cpu: move run_on_cpu to cpus-common, (continued)
[Qemu-devel] [RFC v4 02/71] cpu: rename cpu->work_mutex to cpu->lock, Emilio G. Cota, 2018/10/25
[Qemu-devel] [RFC v4 39/71] i386/hax-all: convert to cpu_interrupt_request, Emilio G. Cota, 2018/10/25
[Qemu-devel] [RFC v4 06/71] cpu: introduce process_queued_cpu_work_locked, Emilio G. Cota, 2018/10/25
[Qemu-devel] [RFC v4 03/71] cpu: introduce cpu_mutex_lock/unlock, Emilio G. Cota, 2018/10/25
[Qemu-devel] [RFC v4 10/71] hppa: convert to helper_cpu_halted_set, Emilio G. Cota, 2018/10/25
[Qemu-devel] [RFC v4 14/71] cpu: define cpu_halted helpers, Emilio G. Cota, 2018/10/25
[Qemu-devel] [RFC v4 12/71] alpha: convert to helper_cpu_halted_set, Emilio G. Cota, 2018/10/25
[Qemu-devel] [RFC v4 38/71] i386/kvm: convert to cpu_interrupt_request, Emilio G. Cota, 2018/10/25