Re: [Qemu-devel] [PATCH v6 03/73] cpu: introduce cpu_mutex_lock/unlock

[Alex Bennée]

Emilio G. Cota <address@hidden> writes:

> The few direct users of &cpu->lock will be converted soon.
>
> Reviewed-by: Richard Henderson <address@hidden>
> Signed-off-by: Emilio G. Cota <address@hidden>
> ---
>  include/qom/cpu.h   | 33 +++++++++++++++++++++++++++++++
>  cpus.c              | 48 +++++++++++++++++++++++++++++++++++++++++++--
>  stubs/cpu-lock.c    | 28 ++++++++++++++++++++++++++
>  stubs/Makefile.objs |  1 +
>  4 files changed, 108 insertions(+), 2 deletions(-)
>  create mode 100644 stubs/cpu-lock.c
<snip>
> diff --git a/cpus.c b/cpus.c
> index 9a3a1d8a6a..187aed2533 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);

I'm a little confused by this __thread bitmap. So by being a __thread
this is a per thread record (like __thread bool iothread_locked) of the
lock. However the test bellow:

> +
> +bool no_cpu_mutex_locked(void)
> +{
> +    return bitmap_empty(cpu_lock_bitmap, CPU_LOCK_BITMAP_SIZE);
> +}

which is used for asserts implies the only case we care about is
ensuring one thread doesn't take multiple cpu locks (which seems
reasonable). The only other test is used by cpu_mutex_locked to see if
the current thread has locked a given CPU index.

Given that a thread can only be in two conditions:

  - no lock held
  - holding lock for cpu->index

Why the bitmap? Surely it could simply be:

  static __thread int current_cpu_lock_held;

Where:

  bool no_cpu_mutex_locked(void)
  {
      return current_cpu_lock_held == 0;
  }

  bool cpu_mutex_locked(const CPUState *cpu)
  {
      return current_cpu_lock_held == cpu->cpu_index + 1;
  }

And then we scale to INT_MAX cpus ;-)

If I've missed something I think the doc comment needs to be a bit more
explicit about our locking rules.

<snip>
>
> +    /* prevent deadlock with CPU mutex */
> +    g_assert(no_cpu_mutex_locked());
> +

Technically asserts don't prevent this - they are just enforcing the
locking rules otherwise we would deadlock.

--
Alex Bennée