[Qemu-devel] [PATCH 0/4] some TCG fixes

[Qemu-devel] [PATCH 0/4] some TCG fixes

[Emilio G. Cota]

The first patch we've seen before -- I'm taking it from the
atomic interrupt_request series.

The other three patches are related to TCG profiling. One
of them is a build fix that I suspect has gone unnoticed
due to its dependence on CONFIG_PROFILER.

The series is checkpatch-clean. You can fetch it from:
  https://github.com/cota/qemu/tree/tcg-profile

Thanks,

		Emilio

[Qemu-devel] [PATCH 1/4] tcg: access cpu->icount_decr.u16.high with atomics

[Emilio G. Cota]

Consistently access u16.high with atomics to avoid
undefined behaviour in MTTCG.

Note that icount_decr.u16.low is only used in icount mode,
so regular accesses to it are OK.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 accel/tcg/tcg-all.c       | 2 +-
 accel/tcg/translate-all.c | 2 +-
 qom/cpu.c                 | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index 56dbb56a16..3d25bdcc17 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -51,7 +51,7 @@ static void tcg_handle_interrupt(CPUState *cpu, int mask)
     if (!qemu_cpu_is_self(cpu)) {
         qemu_cpu_kick(cpu);
     } else {
-        cpu->icount_decr.u16.high = -1;
+        atomic_set(&cpu->icount_decr.u16.high, -1);
         if (use_icount &&
             !cpu->can_do_io
             && (mask & ~old_mask) != 0) {
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index ad5c758246..356dcd0948 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2341,7 +2341,7 @@ void cpu_interrupt(CPUState *cpu, int mask)
 {
     g_assert(qemu_mutex_iothread_locked());
     cpu->interrupt_request |= mask;
-    cpu->icount_decr.u16.high = -1;
+    atomic_set(&cpu->icount_decr.u16.high, -1);
 }
 
 /*
diff --git a/qom/cpu.c b/qom/cpu.c
index 92599f3541..20ad54d43f 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -266,7 +266,7 @@ static void cpu_common_reset(CPUState *cpu)
     cpu->mem_io_pc = 0;
     cpu->mem_io_vaddr = 0;
     cpu->icount_extra = 0;
-    cpu->icount_decr.u32 = 0;
+    atomic_set(&cpu->icount_decr.u32, 0);
     cpu->can_do_io = 1;
     cpu->exception_index = -1;
     cpu->crash_occurred = false;
-- 
2.17.1

[Qemu-devel] [PATCH 2/4] tcg: fix use of uninitialized variable under CONFIG_PROFILER

[Emilio G. Cota]

We forgot to initialize n in commit 15fa08f845 ("tcg: Dynamically
allocate TCGOps", 2017-12-29).

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 tcg/tcg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index f27b22bd3c..8f26916b99 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -3430,7 +3430,7 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb)
 
 #ifdef CONFIG_PROFILER
     {
-        int n;
+        int n = 0;
 
         QTAILQ_FOREACH(op, &s->ops, link) {
             n++;
-- 
2.17.1

[Qemu-devel] [PATCH 3/4] tcg: plug holes in struct TCGProfile

[Emilio G. Cota]

This plugs two 4-byte holes in 64-bit.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 tcg/tcg.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.h b/tcg/tcg.h
index f9f12378e9..d80ef2a883 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -633,8 +633,8 @@ typedef struct TCGProfile {
     int64_t tb_count;
     int64_t op_count; /* total insn count */
     int op_count_max; /* max insn per TB */
-    int64_t temp_count;
     int temp_count_max;
+    int64_t temp_count;
     int64_t del_op_count;
     int64_t code_in_len;
     int64_t code_out_len;
-- 
2.17.1

[Qemu-devel] [PATCH 4/4] tcg: distribute tcg_time into TCG contexts

[Emilio G. Cota]

When we implemented per-vCPU TCG contexts, we forgot to also
distribute the tcg_time counter, which has remained as a global
accessed without any serialization, leading to potentially missed
counts.

Fix it by distributing the field over the TCG contexts, embedding
it into TCGProfile with a field called "cpu_exec_time", which is more
descriptive than "tcg_time". Add a function to query this value
directly, and for completeness, fill in the field in
tcg_profile_snapshot, even though its callers do not use it.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 include/qemu/timer.h |  1 -
 tcg/tcg.h            |  2 ++
 cpus.c               |  3 ++-
 monitor.c            | 13 ++++++++++---
 tcg/tcg.c            | 23 +++++++++++++++++++++++
 5 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index a005ed2692..dfecd03e28 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -1046,7 +1046,6 @@ static inline int64_t profile_getclock(void)
     return get_clock();
 }
 
-extern int64_t tcg_time;
 extern int64_t dev_time;
 #endif
 
diff --git a/tcg/tcg.h b/tcg/tcg.h
index d80ef2a883..c59f254e27 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -629,6 +629,7 @@ typedef struct TCGOp {
 QEMU_BUILD_BUG_ON(NB_OPS > (1 << 8));
 
 typedef struct TCGProfile {
+    int64_t cpu_exec_time;
     int64_t tb_count1;
     int64_t tb_count;
     int64_t op_count; /* total insn count */
@@ -1002,6 +1003,7 @@ int tcg_check_temp_count(void);
 #define tcg_check_temp_count() 0
 #endif
 
+int64_t tcg_cpu_exec_time(void);
 void tcg_dump_info(FILE *f, fprintf_function cpu_fprintf);
 void tcg_dump_op_count(FILE *f, fprintf_function cpu_fprintf);
 
diff --git a/cpus.c b/cpus.c
index 361678e459..cce64874e6 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1425,7 +1425,8 @@ static int tcg_cpu_exec(CPUState *cpu)
     ret = cpu_exec(cpu);
     cpu_exec_end(cpu);
 #ifdef CONFIG_PROFILER
-    tcg_time += profile_getclock() - ti;
+    atomic_set(&tcg_ctx->prof.cpu_exec_time,
+               tcg_ctx->prof.cpu_exec_time + profile_getclock() - ti);
 #endif
     return ret;
 }
diff --git a/monitor.c b/monitor.c
index b9258a7438..823b5a1099 100644
--- a/monitor.c
+++ b/monitor.c
@@ -83,6 +83,7 @@
 #include "sysemu/cpus.h"
 #include "sysemu/iothread.h"
 #include "qemu/cutils.h"
+#include "tcg/tcg.h"
 
 #if defined(TARGET_S390X)
 #include "hw/s390x/storage-keys.h"
@@ -1966,16 +1967,22 @@ static void hmp_info_numa(Monitor *mon, const QDict *qdict)
 
 #ifdef CONFIG_PROFILER
 
-int64_t tcg_time;
 int64_t dev_time;
 
 static void hmp_info_profile(Monitor *mon, const QDict *qdict)
 {
+    static int64_t last_cpu_exec_time;
+    int64_t cpu_exec_time;
+    int64_t delta;
+
+    cpu_exec_time = tcg_cpu_exec_time();
+    delta = cpu_exec_time - last_cpu_exec_time;
+
     monitor_printf(mon, "async time  %" PRId64 " (%0.3f)\n",
                    dev_time, dev_time / (double)NANOSECONDS_PER_SECOND);
     monitor_printf(mon, "qemu time   %" PRId64 " (%0.3f)\n",
-                   tcg_time, tcg_time / (double)NANOSECONDS_PER_SECOND);
-    tcg_time = 0;
+                   delta, delta / (double)NANOSECONDS_PER_SECOND);
+    last_cpu_exec_time = cpu_exec_time;
     dev_time = 0;
 }
 #else
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 8f26916b99..e85133ef05 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -30,6 +30,7 @@
 /* Define to jump the ELF file used to communicate with GDB.  */
 #undef DEBUG_JIT
 
+#include "qemu/error-report.h"
 #include "qemu/cutils.h"
 #include "qemu/host-utils.h"
 #include "qemu/timer.h"
@@ -3361,6 +3362,7 @@ void tcg_profile_snapshot(TCGProfile *prof, bool counters, bool table)
         const TCGProfile *orig = &s->prof;
 
         if (counters) {
+            PROF_ADD(prof, orig, cpu_exec_time);
             PROF_ADD(prof, orig, tb_count1);
             PROF_ADD(prof, orig, tb_count);
             PROF_ADD(prof, orig, op_count);
@@ -3412,11 +3414,32 @@ void tcg_dump_op_count(FILE *f, fprintf_function cpu_fprintf)
                     prof.table_op_count[i]);
     }
 }
+
+int64_t tcg_cpu_exec_time(void)
+{
+    unsigned int n_ctxs = atomic_read(&n_tcg_ctxs);
+    unsigned int i;
+    int64_t ret = 0;
+
+    for (i = 0; i < n_ctxs; i++) {
+        const TCGContext *s = atomic_read(&tcg_ctxs[i]);
+        const TCGProfile *prof = &s->prof;
+
+        ret += atomic_read(&prof->cpu_exec_time);
+    }
+    return ret;
+}
 #else
 void tcg_dump_op_count(FILE *f, fprintf_function cpu_fprintf)
 {
     cpu_fprintf(f, "[TCG profiler not compiled]\n");
 }
+
+int64_t tcg_cpu_exec_time(void)
+{
+    error_report("%s: TCG profiler not compiled", __func__);
+    exit(EXIT_FAILURE);
+}
 #endif
 
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/4] tcg: fix use of uninitialized variable under CONFIG_PROFILER

[Philippe Mathieu-Daudé]

On 10/10/2018 16:48, Emilio G. Cota wrote:
> We forgot to initialize n in commit 15fa08f845 ("tcg: Dynamically
> allocate TCGOps", 2017-12-29).
> 
> Signed-off-by: Emilio G. Cota <cota@braap.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  tcg/tcg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index f27b22bd3c..8f26916b99 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -3430,7 +3430,7 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb)
>  
>  #ifdef CONFIG_PROFILER
>      {
> -        int n;
> +        int n = 0;
>  
>          QTAILQ_FOREACH(op, &s->ops, link) {
>              n++;
> 

Re: [Qemu-devel] [PATCH 0/4] some TCG fixes

[Richard Henderson]

On 10/10/18 7:48 AM, Emilio G. Cota wrote:
> The first patch we've seen before -- I'm taking it from the
> atomic interrupt_request series.
> 
> The other three patches are related to TCG profiling. One
> of them is a build fix that I suspect has gone unnoticed
> due to its dependence on CONFIG_PROFILER.
> 
> The series is checkpatch-clean. You can fetch it from:
>   https://github.com/cota/qemu/tree/tcg-profile

Queued, thanks.


r~