qemu-devel
[Top][All Lists]
Advanced

[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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]