Re: [Qemu-devel] [PATCH v6 00/12] cpu-exec: Safe work in quiescent state

["Alex Bennée" <alex.bennee@linaro.org>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This is not the code I promised at the beginning of the week.
> It's better, and not only in that this one works. :)  Instead of
> reinventing the wheel and using the new wheel for linux-user's
> start_exclusive/end_exclusive, the linux-user/ code is moved to
> cpus-common.c and reused as the synchronization mechanism behind
> async_safe_run_on_cpu.
>
> The code works and actually satisfies our needs very well.  The only
> disadvantage is that safe work items will run with a mutex taken; the
> mutex fits decently in QEMU's hierarchy however, sitting between the
> BQL and the tb_lock.

The series is looking good. I only had a few comments on comments.

So for testing I applied:

1 file changed, 16 insertions(+), 5 deletions(-)
translate-all.c | 21 ++++++++++++++++-----

modified   translate-all.c
@@ -20,6 +20,9 @@
 #include <windows.h>
 #endif
 #include "qemu/osdep.h"
+#include <sys/types.h>
+#include <unistd.h>
+#include <sys/syscall.h>


 #include "qemu-common.h"
@@ -57,7 +60,7 @@
 #include "exec/log.h"

 //#define DEBUG_TB_INVALIDATE
-//#define DEBUG_FLUSH
+#define DEBUG_FLUSH
 /* make various TB consistency checks */
 //#define DEBUG_TB_CHECK

@@ -456,7 +459,8 @@ static inline PageDesc *page_find(tb_page_addr_t index)
    indicated, this is constrained by the range of direct branches on the
    host cpu, as used by the TCG implementation of goto_tb.  */
 #if defined(__x86_64__)
-# define MAX_CODE_GEN_BUFFER_SIZE  (2ul * 1024 * 1024 * 1024)
+/* # define MAX_CODE_GEN_BUFFER_SIZE  (2ul * 1024 * 1024 * 1024) */
+# define MAX_CODE_GEN_BUFFER_SIZE  (256 * 1024)
 #elif defined(__sparc__)
 # define MAX_CODE_GEN_BUFFER_SIZE  (2ul * 1024 * 1024 * 1024)
 #elif defined(__powerpc64__)
@@ -835,18 +839,25 @@ static void page_flush_tb(void)
 static void do_tb_flush(CPUState *cpu, void *data)
 {
     unsigned tb_flush_req = (unsigned) (uintptr_t) data;
-
+    unsigned tb_flush_count;
     tb_lock();

     /* If it's already been done on request of another CPU,
      * just retry.
      */
-    if (atomic_read(&tcg_ctx.tb_ctx.tb_flush_count) != tb_flush_req) {
+    tb_flush_count = atomic_read(&tcg_ctx.tb_ctx.tb_flush_count);
+    if (tb_flush_count != tb_flush_req) {
+#if defined(DEBUG_FLUSH)
+        fprintf(stderr,"%s: (%d/%ld) skipping racing flush %x/%x\n", __func__,
+                getpid(), syscall(SYS_gettid), tb_flush_req, tb_flush_count);
+#endif
         goto done;
     }

 #if defined(DEBUG_FLUSH)
-    printf("qemu: flush code_size=%ld nb_tbs=%d avg_tb_size=%ld\n",
+    fprintf(stderr, "%s: (%d/%ld, %x/%x) code_size=%ld nb_tbs=%d avg_tb_size=%ld\n", __func__,
+            getpid(), syscall(SYS_gettid),
+            tb_flush_req, tb_flush_count,
            (unsigned long)(tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer),
            tcg_ctx.tb_ctx.nb_tbs, tcg_ctx.tb_ctx.nb_tbs > 0 ?
            ((unsigned long)(tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer)) /

And then ran the test in my pigz testing image:

    ./tests/docker/docker.py update pigz-test:armhf ./arm-linux-user/qemu-arm
    retry.py -n 20 -c -- docker run -i -t --rm pigz-test:armhf pigz data.tar

An observed the results of flushing including racing flushes which would
ordinarily take out the system.

It would be nice to resurrect some of the tests/tcg stuff and see if we
can stress linux-user with creating and destroying threads. I guess the
work here also replaces Emilo's RCU cpu list.

>
> For performance, the last patch changes it to avoid condition variables
> in the fast path.  (There are still two memory barriers; if desired they
> could be merged with the ones in rcu_read_lock/rcu_read_unlock).  I am
> including a formal model of the algorithm; together with new documentation
> in include/qom/cpu.h, it accounts for most of the added lines of code.
> Still, it is completely optional.
>
> Paolo
>
> Alex Bennée (1):
>   cpus: pass CPUState to run_on_cpu helpers
>
> Paolo Bonzini (5):
>   cpus-common: move CPU list management to common code
>   cpus-common: move exclusive work infrastructure from linux-user
>   cpus-common: always defer async_run_on_cpu work items
>   cpus-common: Introduce async_safe_run_on_cpu()
>   cpus-common: lock-free fast path for cpu_exec_start/end
>
> Sergey Fedorov (6):
>   cpus: Move common code out of {async_, }run_on_cpu()
>   cpus: Rename flush_queued_work()
>   linux-user: Use QemuMutex and QemuCond
>   linux-user: Add qemu_cpu_is_self() and qemu_cpu_kick()
>   cpus-common: move CPU work item management to common code
>   tcg: Make tb_flush() thread safe
>
>  Makefile.target            |   2 +-
>  bsd-user/main.c            |  30 +----
>  cpu-exec.c                 |  12 +-
>  cpus-common.c              | 314 +++++++++++++++++++++++++++++++++++++++++++++
>  cpus.c                     |  99 +-------------
>  docs/tcg-exclusive.promela | 224 ++++++++++++++++++++++++++++++++
>  exec.c                     |  30 +----
>  hw/i386/kvm/apic.c         |   3 +-
>  hw/i386/kvmvapic.c         |   6 +-
>  hw/ppc/ppce500_spin.c      |  31 ++---
>  hw/ppc/spapr.c             |   6 +-
>  hw/ppc/spapr_hcall.c       |  17 +--
>  include/exec/cpu-all.h     |   4 +
>  include/exec/cpu-common.h  |   2 +
>  include/exec/exec-all.h    |  11 --
>  include/exec/tb-context.h  |   2 +-
>  include/qom/cpu.h          |  95 +++++++++++++-
>  kvm-all.c                  |  21 +--
>  linux-user/main.c          | 130 ++++++-------------
>  target-i386/helper.c       |  19 ++-
>  target-i386/kvm.c          |   6 +-
>  target-s390x/cpu.c         |   4 +-
>  target-s390x/cpu.h         |   7 +-
>  target-s390x/kvm.c         |  98 +++++++-------
>  target-s390x/misc_helper.c |   4 +-
>  translate-all.c            |  38 ++++--
>  vl.c                       |   1 +
>  27 files changed, 814 insertions(+), 402 deletions(-)
>  create mode 100644 cpus-common.c
>  create mode 100644 docs/tcg-exclusive.promela


--
Alex Bennée