diff mbox series

[v8,03/74] cpu: introduce cpu_mutex_lock/unlock

Message ID 20200326193156.4322-4-robert.foley@linaro.org (mailing list archive)
State New, archived
Headers show
Series per-CPU locks | expand

Commit Message

Robert Foley March 26, 2020, 7:30 p.m. UTC 
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

Comments

Alex Bennée May 11, 2020, 10:24 a.m. UTC | #1 
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.
Robert Foley May 11, 2020, 4:09 p.m. UTC | #2 
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 mbox series

Patch

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;
+}