| Message ID | 20200326193156.4322-4-robert.foley@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | per-CPU locks | expand |
Robert Foley <robert.foley@linaro.org> writes: > From: "Emilio G. Cota" <cota@braap.org> > > The few direct users of &cpu->lock will be converted soon. > > The per-thread bitmap introduced here might seem unnecessary, > since a bool could just do. However, once we complete the > conversion to per-vCPU locks, we will need to cover the use > case where all vCPUs are locked by the same thread, which > explains why the bitmap is introduced here. > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Emilio G. Cota <cota@braap.org> > Signed-off-by: Robert Foley <robert.foley@linaro.org> > --- > cpus.c | 48 +++++++++++++++++++++++++++++++++++++++++-- > include/hw/core/cpu.h | 33 +++++++++++++++++++++++++++++ > stubs/Makefile.objs | 1 + > stubs/cpu-lock.c | 28 +++++++++++++++++++++++++ > 4 files changed, 108 insertions(+), 2 deletions(-) > create mode 100644 stubs/cpu-lock.c > > diff --git a/cpus.c b/cpus.c > index 71bd2f5d55..633734fb5c 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -91,6 +91,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 I wonder if we should be asserting this somewhere? Given it's an init time constant we can probably do it somewhere in the machine realise stage. I think the value is set in MachineState *ms->smp.max_cpus; <snip> > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs > index 45be5dc0ed..d2dd6c94cc 100644 > --- a/stubs/Makefile.objs > +++ b/stubs/Makefile.objs > @@ -5,6 +5,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 > stub-obj-y += dump.o > stub-obj-y += error-printf.o > stub-obj-y += fdset.o > diff --git a/stubs/cpu-lock.c b/stubs/cpu-lock.c > new file mode 100644 > index 0000000000..ca2ea8a9c2 > --- /dev/null > +++ b/stubs/cpu-lock.c > @@ -0,0 +1,28 @@ > +#include "qemu/osdep.h" > +#include "hw/core/cpu.h" > + > +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); > + f(&cpu->lock, file, line); > +#endif > +} > + > +void cpu_mutex_unlock_impl(CPUState *cpu, const char *file, int line) > +{ > + qemu_mutex_unlock_impl(&cpu->lock, file, line); > +} I find this a little confusing because we clearly aren't stubbing something out here - we are indeed doing a lock. What we seem to have is effectively the linux-user implementation of cpu locking which currently doesn't support qsp profiling. > +bool cpu_mutex_locked(const CPUState *cpu) > +{ > + return true; > +} > + > +bool no_cpu_mutex_locked(void) > +{ > + return true; > +} What functions care about these checks. I assume it's only system emulation checks that are in common code. Maybe that indicates we could achieve better separation of emulation and linux-user code. My worry is by adding an assert in linux-user code we wouldn't actually be asserting anything.
On Mon, 11 May 2020 at 06:24, Alex Bennée <alex.bennee@linaro.org> wrote: > Robert Foley <robert.foley@linaro.org> writes: snip > > +/* XXX: is this really the max number of CPUs? */ > > +#define CPU_LOCK_BITMAP_SIZE 2048 > > I wonder if we should be asserting this somewhere? Given it's an init > time constant we can probably do it somewhere in the machine realise > stage. I think the value is set in MachineState *ms->smp.max_cpus; Sure, I suppose we can relocate the define to something like hw/core/cpu.h, and then assert on it in smp_parse() from hw/core/machine.c? > <snip> > > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs > > index 45be5dc0ed..d2dd6c94cc 100644 > > --- a/stubs/Makefile.objs > > +++ b/stubs/Makefile.objs > > @@ -5,6 +5,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 > > stub-obj-y += dump.o > > stub-obj-y += error-printf.o > > stub-obj-y += fdset.o > > diff --git a/stubs/cpu-lock.c b/stubs/cpu-lock.c > > new file mode 100644 > > index 0000000000..ca2ea8a9c2 > > --- /dev/null > > +++ b/stubs/cpu-lock.c > > @@ -0,0 +1,28 @@ > > +#include "qemu/osdep.h" > > +#include "hw/core/cpu.h" > > + > > +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); > > + f(&cpu->lock, file, line); > > +#endif > > +} > > + > > +void cpu_mutex_unlock_impl(CPUState *cpu, const char *file, int line) > > +{ > > + qemu_mutex_unlock_impl(&cpu->lock, file, line); > > +} > > I find this a little confusing because we clearly aren't stubbing > something out here - we are indeed doing a lock. What we seem to have is > effectively the linux-user implementation of cpu locking which currently > doesn't support qsp profiling. I agree, it seems like cpu_mutex_lock/unlock can follow the model of stubs/iothread-lock.c, which does not use a lock. Will change this. > > > +bool cpu_mutex_locked(const CPUState *cpu) > > +{ > > + return true; > > +} > > + > > +bool no_cpu_mutex_locked(void) > > +{ > > + return true; > > +} > > What functions care about these checks. I assume it's only system > emulation checks that are in common code. Maybe that indicates we could > achieve better separation of emulation and linux-user code. My worry is > by adding an assert in linux-user code we wouldn't actually be asserting > anything. There is code that runs during linux-user, which calls cpu_mutex_locked(). I found a few cases at least where cpu_interrupt_request_set, cpu_halted, cpu_halted_set from include/hw/core/cpu.h are called in linux-user. Also cpu_handle_halt_locked from accel/tcg/cpu-exec.c no_cpu_mutex_locked() is also linked into linux user for run_on_cpu()in cpus-common.c. Thanks, -Rob
diff --git a/cpus.c b/cpus.c index 71bd2f5d55..633734fb5c 100644 --- a/cpus.c +++ b/cpus.c @@ -91,6 +91,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(); @@ -100,9 +141,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; } @@ -1837,6 +1878,9 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line) { QemuMutexLockFunc bql_lock = atomic_read(&qemu_bql_mutex_lock_func); + /* enforce locking order */ + 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/include/hw/core/cpu.h b/include/hw/core/cpu.h index 0b75fdb093..4521bba7a5 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -457,6 +457,39 @@ extern 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/stubs/Makefile.objs b/stubs/Makefile.objs index 45be5dc0ed..d2dd6c94cc 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -5,6 +5,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 stub-obj-y += dump.o stub-obj-y += error-printf.o stub-obj-y += fdset.o diff --git a/stubs/cpu-lock.c b/stubs/cpu-lock.c new file mode 100644 index 0000000000..ca2ea8a9c2 --- /dev/null +++ b/stubs/cpu-lock.c @@ -0,0 +1,28 @@ +#include "qemu/osdep.h" +#include "hw/core/cpu.h" + +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); + f(&cpu->lock, file, line); +#endif +} + +void cpu_mutex_unlock_impl(CPUState *cpu, const char *file, int line) +{ + qemu_mutex_unlock_impl(&cpu->lock, file, line); +} + +bool cpu_mutex_locked(const CPUState *cpu) +{ + return true; +} + +bool no_cpu_mutex_locked(void) +{ + return true; +}