diff mbox series

[v8,02/74] cpu: rename cpu->work_mutex to cpu->lock

Message ID 20200326193156.4322-3-robert.foley@linaro.org
State Superseded
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>


This lock will soon protect more fields of the struct. Give
it a more appropriate name.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Signed-off-by: Emilio G. Cota <cota@braap.org>

Signed-off-by: Robert Foley <robert.foley@linaro.org>

---
 cpus-common.c         | 14 +++++++-------
 cpus.c                |  4 ++--
 hw/core/cpu.c         |  4 ++--
 include/hw/core/cpu.h |  6 ++++--
 4 files changed, 15 insertions(+), 13 deletions(-)

-- 
2.17.1

Comments

Alex Bennée May 11, 2020, 2:48 p.m. UTC | #1
Robert Foley <robert.foley@linaro.org> writes:

> From: "Emilio G. Cota" <cota@braap.org>

>

> This lock will soon protect more fields of the struct. Give

> it a more appropriate name.


Hmm while bisecting to find another problem I found this commit:

  /home/alex/lsrc/qemu.git/hw/core/cpu.c: In function ‘cpu_common_finalize’:
  /home/alex/lsrc/qemu.git/hw/core/cpu.c:383:27: error: incompatible type for argument 1 of ‘qemu_mutex_destroy’
       qemu_mutex_destroy(cpu->lock);
                          ~~~^~~~~~
  In file included from /home/alex/lsrc/qemu.git/include/hw/core/cpu.h:31,
                   from /home/alex/lsrc/qemu.git/hw/core/cpu.c:23:
  /home/alex/lsrc/qemu.git/include/qemu/thread.h:26:36: note: expected ‘QemuMutex *’ {aka ‘struct QemuMutex *’} but argument is of type ‘QemuMutex’ {aka ‘struct QemuMutex’}
   void qemu_mutex_destroy(QemuMutex *mutex);
                           ~~~~~~~~~~~^~~~~
  make: *** [/home/alex/lsrc/qemu.git/rules.mak:69: hw/core/cpu.o] Error 1

which works fine in the final product so I suspect something has slipped
between commits somewhere.

>

> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> Signed-off-by: Emilio G. Cota <cota@braap.org>

> Signed-off-by: Robert Foley <robert.foley@linaro.org>

> ---

>  cpus-common.c         | 14 +++++++-------

>  cpus.c                |  4 ++--

>  hw/core/cpu.c         |  4 ++--

>  include/hw/core/cpu.h |  6 ++++--

>  4 files changed, 15 insertions(+), 13 deletions(-)

>

> diff --git a/cpus-common.c b/cpus-common.c

> index 3d659df464..f75cae23d9 100644

> --- a/cpus-common.c

> +++ b/cpus-common.c

> @@ -107,10 +107,10 @@ struct qemu_work_item {

>  

>  static void queue_work_on_cpu(CPUState *cpu, struct qemu_work_item *wi)

>  {

> -    qemu_mutex_lock(&cpu->work_mutex);

> +    qemu_mutex_lock(&cpu->lock);

>      QSIMPLEQ_INSERT_TAIL(&cpu->work_list, wi, node);

>      wi->done = false;

> -    qemu_mutex_unlock(&cpu->work_mutex);

> +    qemu_mutex_unlock(&cpu->lock);

>  

>      qemu_cpu_kick(cpu);

>  }

> @@ -304,15 +304,15 @@ void process_queued_cpu_work(CPUState *cpu)

>  {

>      struct qemu_work_item *wi;

>  

> -    qemu_mutex_lock(&cpu->work_mutex);

> +    qemu_mutex_lock(&cpu->lock);

>      if (QSIMPLEQ_EMPTY(&cpu->work_list)) {

> -        qemu_mutex_unlock(&cpu->work_mutex);

> +        qemu_mutex_unlock(&cpu->lock);

>          return;

>      }

>      while (!QSIMPLEQ_EMPTY(&cpu->work_list)) {

>          wi = QSIMPLEQ_FIRST(&cpu->work_list);

>          QSIMPLEQ_REMOVE_HEAD(&cpu->work_list, node);

> -        qemu_mutex_unlock(&cpu->work_mutex);

> +        qemu_mutex_unlock(&cpu->lock);

>          if (wi->exclusive) {

>              /* Running work items outside the BQL avoids the following deadlock:

>               * 1) start_exclusive() is called with the BQL taken while another

> @@ -328,13 +328,13 @@ void process_queued_cpu_work(CPUState *cpu)

>          } else {

>              wi->func(cpu, wi->data);

>          }

> -        qemu_mutex_lock(&cpu->work_mutex);

> +        qemu_mutex_lock(&cpu->lock);

>          if (wi->free) {

>              g_free(wi);

>          } else {

>              atomic_mb_set(&wi->done, true);

>          }

>      }

> -    qemu_mutex_unlock(&cpu->work_mutex);

> +    qemu_mutex_unlock(&cpu->lock);

>      qemu_cond_broadcast(&qemu_work_cond);

>  }

> diff --git a/cpus.c b/cpus.c

> index 151abbc04c..71bd2f5d55 100644

> --- a/cpus.c

> +++ b/cpus.c

> @@ -100,9 +100,9 @@ static inline bool cpu_work_list_empty(CPUState *cpu)

>  {

>      bool ret;

>  

> -    qemu_mutex_lock(&cpu->work_mutex);

> +    qemu_mutex_lock(&cpu->lock);

>      ret = QSIMPLEQ_EMPTY(&cpu->work_list);

> -    qemu_mutex_unlock(&cpu->work_mutex);

> +    qemu_mutex_unlock(&cpu->lock);

>      return ret;

>  }

>  

> diff --git a/hw/core/cpu.c b/hw/core/cpu.c

> index 2fc6aa2159..bc0416829a 100644

> --- a/hw/core/cpu.c

> +++ b/hw/core/cpu.c

> @@ -367,7 +367,7 @@ static void cpu_common_initfn(Object *obj)

>      cpu->nr_cores = 1;

>      cpu->nr_threads = 1;

>  

> -    qemu_mutex_init(&cpu->work_mutex);

> +    qemu_mutex_init(&cpu->lock);

>      QSIMPLEQ_INIT(&cpu->work_list);

>      QTAILQ_INIT(&cpu->breakpoints);

>      QTAILQ_INIT(&cpu->watchpoints);

> @@ -379,7 +379,7 @@ static void cpu_common_finalize(Object *obj)

>  {

>      CPUState *cpu = CPU(obj);

>  

> -    qemu_mutex_destroy(&cpu->work_mutex);

> +    qemu_mutex_destroy(cpu->lock);

>  }

>  

>  static int64_t cpu_common_get_arch_id(CPUState *cpu)

> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h

> index 398b65159e..0b75fdb093 100644

> --- a/include/hw/core/cpu.h

> +++ b/include/hw/core/cpu.h

> @@ -331,7 +331,8 @@ struct qemu_work_item;

>   * @opaque: User data.

>   * @mem_io_pc: Host Program Counter at which the memory was accessed.

>   * @kvm_fd: vCPU file descriptor for KVM.

> - * @work_mutex: Lock to prevent multiple access to @work_list.

> + * @lock: Lock to prevent multiple access to per-CPU fields. Must be acquired

> + *        after the BQL.

>   * @work_list: List of pending asynchronous work.

>   * @trace_dstate_delayed: Delayed changes to trace_dstate (includes all changes

>   *                        to @trace_dstate).

> @@ -375,7 +376,8 @@ struct CPUState {

>      uint64_t random_seed;

>      sigjmp_buf jmp_env;

>  

> -    QemuMutex work_mutex;

> +    QemuMutex lock;

> +    /* fields below protected by @lock */

>      QSIMPLEQ_HEAD(, qemu_work_item) work_list;

>  

>      CPUAddressSpace *cpu_ases;



-- 
Alex Bennée
Robert Foley May 11, 2020, 4:33 p.m. UTC | #2
On Mon, 11 May 2020 at 10:48, Alex Bennée <alex.bennee@linaro.org> wrote:
> Hmm while bisecting to find another problem I found this commit:

>

>   /home/alex/lsrc/qemu.git/hw/core/cpu.c: In function ‘cpu_common_finalize’:

>   /home/alex/lsrc/qemu.git/hw/core/cpu.c:383:27: error: incompatible type for argument 1 of ‘qemu_mutex_destroy’

>        qemu_mutex_destroy(cpu->lock);

>                           ~~~^~~~~~

>   In file included from /home/alex/lsrc/qemu.git/include/hw/core/cpu.h:31,

>                    from /home/alex/lsrc/qemu.git/hw/core/cpu.c:23:

>   /home/alex/lsrc/qemu.git/include/qemu/thread.h:26:36: note: expected ‘QemuMutex *’ {aka ‘struct QemuMutex *’} but argument is of type ‘QemuMutex’ {aka ‘struct QemuMutex’}

>    void qemu_mutex_destroy(QemuMutex *mutex);

>                            ~~~~~~~~~~~^~~~~

>   make: *** [/home/alex/lsrc/qemu.git/rules.mak:69: hw/core/cpu.o] Error 1

>

> which works fine in the final product so I suspect something has slipped

> between commits somewhere.


I agree, looks like something is off with the intermediate commits.
Thanks for noticing it !  Will fix it.

Thanks,
-Rob
diff mbox series

Patch

diff --git a/cpus-common.c b/cpus-common.c
index 3d659df464..f75cae23d9 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -107,10 +107,10 @@  struct qemu_work_item {
 
 static void queue_work_on_cpu(CPUState *cpu, struct qemu_work_item *wi)
 {
-    qemu_mutex_lock(&cpu->work_mutex);
+    qemu_mutex_lock(&cpu->lock);
     QSIMPLEQ_INSERT_TAIL(&cpu->work_list, wi, node);
     wi->done = false;
-    qemu_mutex_unlock(&cpu->work_mutex);
+    qemu_mutex_unlock(&cpu->lock);
 
     qemu_cpu_kick(cpu);
 }
@@ -304,15 +304,15 @@  void process_queued_cpu_work(CPUState *cpu)
 {
     struct qemu_work_item *wi;
 
-    qemu_mutex_lock(&cpu->work_mutex);
+    qemu_mutex_lock(&cpu->lock);
     if (QSIMPLEQ_EMPTY(&cpu->work_list)) {
-        qemu_mutex_unlock(&cpu->work_mutex);
+        qemu_mutex_unlock(&cpu->lock);
         return;
     }
     while (!QSIMPLEQ_EMPTY(&cpu->work_list)) {
         wi = QSIMPLEQ_FIRST(&cpu->work_list);
         QSIMPLEQ_REMOVE_HEAD(&cpu->work_list, node);
-        qemu_mutex_unlock(&cpu->work_mutex);
+        qemu_mutex_unlock(&cpu->lock);
         if (wi->exclusive) {
             /* Running work items outside the BQL avoids the following deadlock:
              * 1) start_exclusive() is called with the BQL taken while another
@@ -328,13 +328,13 @@  void process_queued_cpu_work(CPUState *cpu)
         } else {
             wi->func(cpu, wi->data);
         }
-        qemu_mutex_lock(&cpu->work_mutex);
+        qemu_mutex_lock(&cpu->lock);
         if (wi->free) {
             g_free(wi);
         } else {
             atomic_mb_set(&wi->done, true);
         }
     }
-    qemu_mutex_unlock(&cpu->work_mutex);
+    qemu_mutex_unlock(&cpu->lock);
     qemu_cond_broadcast(&qemu_work_cond);
 }
diff --git a/cpus.c b/cpus.c
index 151abbc04c..71bd2f5d55 100644
--- a/cpus.c
+++ b/cpus.c
@@ -100,9 +100,9 @@  static inline bool cpu_work_list_empty(CPUState *cpu)
 {
     bool ret;
 
-    qemu_mutex_lock(&cpu->work_mutex);
+    qemu_mutex_lock(&cpu->lock);
     ret = QSIMPLEQ_EMPTY(&cpu->work_list);
-    qemu_mutex_unlock(&cpu->work_mutex);
+    qemu_mutex_unlock(&cpu->lock);
     return ret;
 }
 
diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index 2fc6aa2159..bc0416829a 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -367,7 +367,7 @@  static void cpu_common_initfn(Object *obj)
     cpu->nr_cores = 1;
     cpu->nr_threads = 1;
 
-    qemu_mutex_init(&cpu->work_mutex);
+    qemu_mutex_init(&cpu->lock);
     QSIMPLEQ_INIT(&cpu->work_list);
     QTAILQ_INIT(&cpu->breakpoints);
     QTAILQ_INIT(&cpu->watchpoints);
@@ -379,7 +379,7 @@  static void cpu_common_finalize(Object *obj)
 {
     CPUState *cpu = CPU(obj);
 
-    qemu_mutex_destroy(&cpu->work_mutex);
+    qemu_mutex_destroy(cpu->lock);
 }
 
 static int64_t cpu_common_get_arch_id(CPUState *cpu)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 398b65159e..0b75fdb093 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -331,7 +331,8 @@  struct qemu_work_item;
  * @opaque: User data.
  * @mem_io_pc: Host Program Counter at which the memory was accessed.
  * @kvm_fd: vCPU file descriptor for KVM.
- * @work_mutex: Lock to prevent multiple access to @work_list.
+ * @lock: Lock to prevent multiple access to per-CPU fields. Must be acquired
+ *        after the BQL.
  * @work_list: List of pending asynchronous work.
  * @trace_dstate_delayed: Delayed changes to trace_dstate (includes all changes
  *                        to @trace_dstate).
@@ -375,7 +376,8 @@  struct CPUState {
     uint64_t random_seed;
     sigjmp_buf jmp_env;
 
-    QemuMutex work_mutex;
+    QemuMutex lock;
+    /* fields below protected by @lock */
     QSIMPLEQ_HEAD(, qemu_work_item) work_list;
 
     CPUAddressSpace *cpu_ases;