[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8 03/74] cpu: introduce cpu_mutex_lock/unlock
From: |
Alex Bennée |
Subject: |
Re: [PATCH v8 03/74] cpu: introduce cpu_mutex_lock/unlock |
Date: |
Mon, 11 May 2020 11:24:26 +0100 |
User-agent: |
mu4e 1.4.5; emacs 28.0.50 |
Robert Foley <address@hidden> writes:
> From: "Emilio G. Cota" <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Emilio G. Cota <address@hidden>
> Signed-off-by: Robert Foley <address@hidden>
> ---
> 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.
--
Alex Bennée
- Re: [PATCH v8 03/74] cpu: introduce cpu_mutex_lock/unlock,
Alex Bennée <=