tcg: parallel code generation (Work in Progress)
cover tcg: parallel code generation (Work in Progress)
This is a request for comments as well as a request for help :-)
I've been experimenting with making TCGContext per-thread, so that we can run most of tcg_gen_code in parallel. I've made some progress, but haven't yet got it to work.
My guess is that the TCG stack is still global instead of per-vCPU (it's been global since tmp_buf was removed from CPUState, right?), but I'm having trouble following that code so most likely I'm wrong.
Any help would be appreciated--please disregard minor nits, I want to see whether I can make this work to then take measurements to decide whether this is worth the trouble.
- Patch 1 is a trivial doc fixup, feel free to pick it up
- Patches 2-3 remove *tbs[] to use a binary search tree instead. This removes the assumption in tb_find_pc that *tbs[] are ordered by tc_ptr, thereby allowing us to generate code regardless of its location on the host (as we do after patch 6).
- Patch 4 addresses a reporting issue: ever since we embedded the struct TB's in code_gen_buffer (6e3b2bfd6), we have been misreporting the size of the generated code. Not a huge deal, but I noticed while I was working on this.
- Patches 5-7 make TCGContext per-thread in softmmu. I have put there some XXX's to note that I'm aware of those issues, so don't worry too much about those--except of course if you have any input on what the cause of the race(s) might be.
Thanks,
Emilio
RFC 1/7 exec-all: fix typos in TranslationBlock's documentation
--- include/exec/exec-all.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
include/exec/exec-all.h +2 -2
@@ -338,7 +338,7 @@ struct TranslationBlock {/* The following data are used to directly call another TB from * the code of this one. This can be done either by emitting direct or * indirect native jump instructions. These jumps are reset so that the TB -* just continue its execution. The TB can be linked to another one by +* just continues its execution. The TB can be linked to another one by * setting one of the jump targets (or patching the jump instruction). Only * two of such jumps are supported. */
@@ -349,7 +349,7 @@ struct TranslationBlock {#else uintptr_t jmp_target_addr[2 ];/* target address for indirect jump */ #endif -/* Each TB has an assosiated circular list of TBs jumping to this one. +/* Each TB has an associated circular list of TBs jumping to this one. * jmp_list_first points to the first TB jumping to this one. * jmp_list_next is used to point to the next TB in a list. * Since each TB can have two jumps, it can participate in two lists.
Signature · 1 line
2.7.4
Re: [Qemu-devel] [RFC 1/7] exec-all: fix typos in TranslationBlock's documentation
Quoted 4 lines · Emilio G. Cota
Signed-off-by Emilio G. Cota<[email protected]>--- include/exec/exec-all.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
r~
RFC 2/7 translate-all: add out_size field to TranslationBlock
In reply to Emilio G. CotaThis paves the way for upcoming work: we need tb->out_size for tb_find_pc to work with a binary search tree.
Note that due to the cacheline padding we are using, for hosts with 64-byte cache lines this will not waste any additional memory. Using a s16 would be ideal, since that would plug an existing hole in the struct, but I see no guarantee that a TB won't overflow it.
--- include/exec/exec-all.h | 1 + accel/tcg/translate-all.c | 16 ++++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-)
include/exec/exec-all.h +1 -0
@@ -363,6 +363,7 @@ struct TranslationBlock {*/ uintptr_t jmp_list_next[2 ];uintptr_t jmp_list_first; +int32_t out_size;/* size of host code for this block */ };void tb_free (TranslationBlock *tb);
accel/tcg/translate-all.c +8 -8
@@ -1260,7 +1260,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,tb_page_addr_t phys_pc, phys_page2;target_ulong virt_page2;tcg_insn_unit *gen_code_buf; -int gen_code_size, search_size; +int search_size;#ifdef CONFIG_PROFILER int64_t ti;#endif
@@ -1327,11 +1327,11 @@ TranslationBlock *tb_gen_code(CPUState *cpu,the tcg optimization currently hidden inside tcg_gen_code. All that should be required is to flush the TBs, allocate a new TB, re-initialize it per above, and re-do the actual code generation. */ - gen_code_size =tcg_gen_code (&tcg_ctx, tb); -if (unlikely (gen_code_size <0 )) { + tb->out_size =tcg_gen_code (&tcg_ctx, tb); +if (unlikely (tb->out_size <0 )) { goto buffer_overflow; } - search_size =encode_search (tb, (void *)gen_code_buf + gen_code_size); + search_size =encode_search (tb, (void *)gen_code_buf + tb->out_size);if (unlikely (search_size <0 )) { goto buffer_overflow; }
@@ -1339,7 +1339,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,#ifdef CONFIG_PROFILER tcg_ctx.code_time +=profile_getclock (); tcg_ctx.code_in_len += tb->size; - tcg_ctx.code_out_len += gen_code_size; + tcg_ctx.code_out_len += tb->out_size; tcg_ctx.search_out_len += search_size;#endif
@@ -1347,8 +1347,8 @@ TranslationBlock *tb_gen_code(CPUState *cpu,if (qemu_loglevel_mask (CPU_LOG_TB_OUT_ASM ) &&qemu_log_in_addr_range (tb->pc)) {qemu_log_lock (); -qemu_log ("OUT: [size=%d]\n" , gen_code_size); -log_disas (tb->tc_ptr, gen_code_size); +qemu_log ("OUT: [size=%d]\n" , tb->out_size); +log_disas (tb->tc_ptr, tb->out_size);qemu_log ("\n" );qemu_log_flush ();qemu_log_unlock ();
@@ -1356,7 +1356,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,#endif tcg_ctx.code_gen_ptr = (void *) -ROUND_UP ((uintptr_t )gen_code_buf + gen_code_size + search_size, +ROUND_UP ((uintptr_t )gen_code_buf + tb->out_size + search_size,CODE_GEN_ALIGN );/* init jump list */
Signature · 1 line
2.7.4
Re: [Qemu-devel] [RFC 2/7] translate-all: add out_size field to TranslationBlock
Quoted 17 lines · Emilio G. Cota
Note that due to the cacheline padding we are using, for hosts with 64-byte cache lines this will not waste any additional memory. Using a s16 would be ideal, since that would plug an existing hole in the struct, but I see no guarantee that a TB won't overflow it.
Signed-off-by Emilio G. Cota<[email protected]>--- include/exec/exec-all.h | 1 + accel/tcg/translate-all.c | 16 ++++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-)include/exec/exec-all.h +0
-0
include/exec/exec-all.h struct TranslationBlock { · line 366
Quoted 4 lines
include/exec/exec-all.h +1
-0@@ -363,6 +363,7 @@ struct TranslationBlock {*/ uintptr_t jmp_list_next[2 ];uintptr_t jmp_list_first; +int32_t out_size;/* size of host code for this block */
unsigned probably better.
I do wonder about putting it in the hole after invalid. Which itself could be shrunk to bool.
I don't believe there's much chance of an overflow of uint16_t. The limit of OPC_BUF_SIZE = 640 fairly well limits the practical size. And, honestly, it doesn't matter if we saturate to 0xffff, so long as you retain the full-size gen_code_size local variable.
r~
RFC 3/7 translate-all: use a binary search tree to track TBs in TBContext
In reply to Emilio G. CotaThis is a prerequisite for having threads generate code on separate buffers, which will help scalability when booting multiple cores under MTTCG.
Note that tb_free does not free space in the code_gen_buffer anymore, since we cannot easily know whether the tb is the last one inserted in code_gen_buffer.
Performance-wise, lookups in tb_find_pc are the same as before:
O(log n). However, insertions are O(log n) instead of O(1), which results in a small slowdown when booting debian-arm:
Performance counter stats for 'build/arm-softmmu/qemu-system-arm \ -machine type=virt -nographic -smp 1 -m 4096 \ -netdev user,id=unet,hostfwd=tcp::2222-:22 \ -device virtio-net-device,netdev=unet \ -drive file=img/arm/jessie-arm32.qcow2,id=myblock,index=0,if=none \ -device virtio-blk-device,drive=myblock \ -kernel img/arm/aarch32-current-linux-kernel-only.img \ -append console=ttyAMA0 root=/dev/vda1 \ -name arm,debug-threads=on -smp 1' (10 runs):
- Before:
10289.389753 task-clock (msec) # 0.952 CPUs utilized ( +- 0.13% )
18,238 context-switches # 0.002 M/sec ( +- 0.73% )
0 cpu-migrations # 0.000 K/sec
86,555 page-faults # 0.008 M/sec ( +- 0.49% )
45,079,926,395 cycles # 4.381 GHz ( +- 0.14% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
84,582,463,603 instructions # 1.88 insns per cycle ( +- 0.26% )
14,964,335,400 branches # 1454.346 M/sec ( +- 0.29% )
288,324,215 branch-misses # 1.93% of all branches ( +- 0.34% )10.813687279 seconds time elapsed ( +- 0.42% )
- After:
10333.181473 task-clock (msec) # 0.944 CPUs utilized ( +- 0.27% )
18,167 context-switches # 0.002 M/sec ( +- 0.20% )
0 cpu-migrations # 0.000 K/sec
83,354 page-faults # 0.008 M/sec ( +- 0.92% )
45,247,697,926 cycles # 4.379 GHz ( +- 0.23% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
84,537,657,945 instructions # 1.87 insns per cycle ( +- 0.18% )
14,988,568,500 branches # 1450.528 M/sec ( +- 0.21% )
294,765,097 branch-misses # 1.97% of all branches ( +- 0.43% )10.946641611 seconds time elapsed ( +- 0.79% )
--- include/exec/tb-context.h | 4 +- accel/tcg/translate-all.c | 181 ++++++++++++++++++++-------------------------- 2 files changed, 81 insertions(+), 104 deletions(-)
include/exec/tb-context.h +1 -3
@@ -31,10 +31,8 @@ typedef struct TBContext TBContext;struct TBContext { -TranslationBlock **tbs; +GTree *tb_tree;struct qht htable; -size_t tbs_size; -int nb_tbs;/* any access to the tbs or the page table must use this lock */ QemuMutex tb_lock;
accel/tcg/translate-all.c +80 -101
266 lines not on this page · raw message
Signature · 1 line
2.7.4
Re: [Qemu-devel] [RFC 3/7] translate-all: use a binary search tree to track TBs in TBContext
accel/tcg/translate-all.c static inline void *alloc_code_gen_buffer(void) · line 777
Quoted 5 lines · Emilio G. Cota
accel/tcg/translate-all.c +5
-0+/* @key is already in the tree so it's safe to use container_of on it */ +static gint tc_ptr_cmp (gconstpointer candidate,gconstpointer key) +{ +uintptr_t a = *(uintptr_t *)candidate; +const TranslationBlock *tb =container_of (key, TranslationBlock, tc_ptr);
This I'm not keen on. It'd be one thing if it was our own datastructure, but I see nothing in the GTree documentation that says that the comparison must always be done this way.
r~
Re: [Qemu-devel] [RFC 3/7] translate-all: use a binary search tree to track TBs in TBContext
Quoted 8 lines · Richard Henderson
accel/tcg/translate-all.c static inline void *alloc_code_gen_buffer(void) · line 777
Quoted 5 lines · Emilio G. Cota
accel/tcg/translate-all.c +5
-0+/* @key is already in the tree so it's safe to use container_of on it */ +static gint tc_ptr_cmp (gconstpointer candidate,gconstpointer key) +{ +uintptr_t a = *(uintptr_t *)candidate; +const TranslationBlock *tb =container_of (key, TranslationBlock, tc_ptr);This I'm not keen on. It'd be one thing if it was our own datastructure, but I see nothing in the GTree documentation that says that the comparison must always be done this way.
What if we bundle tc_ptr + tc_size into a struct and only reference that? We'd embed that struct into the TB. In tb_find_pc, create that struct on the stack, setting tc_size = 0.
r~
Re: [Qemu-devel] [RFC 3/7] translate-all: use a binary search tree to track TBs in TBContext
Quoted 11 lines · Richard Henderson
Quoted 10 lines · Richard Henderson
Review
Quoted 5 lines · Emilio G. Cota
(quoted diff) +5
-0+/* @key is already in the tree so it's safe to use container_of on it */ +static gint tc_ptr_cmp(gconstpointer candidate, gconstpointer key) +{ + uintptr_t a = *(uintptr_t *)candidate; + const TranslationBlock *tb = container_of(key, TranslationBlock, tc_ptr);This I'm not keen on. It'd be one thing if it was our own datastructure, but I see nothing in the GTree documentation that says that the comparison must always be done this way.
I also checked for this. Couldn't find anything in the docs -- the only guarantee I could find is the implicit one that since the g_tree module was created, the 2nd pointer ("b" above) has consistenly been the in-node one. I don't think they'd ever change this, but yes relying on this assumption is a bit risky.
If we prefer using our own we could bring the AVL tree from CCAN:
http://git.ozlabs.org/?p=ccan;a=tree;f=ccan/avl
Quoted 3 lines
What if we bundle
tc_ptr+tc_sizeinto a struct and only reference that? We'd embed that struct into the TB. Intb_find_pc, create that struct on the stack, settingtc_size= 0.
This is clean and safe, but we'd add a 4-byte hole for 64-on-64bit. However, we could bring other fields into the embedded struct to plug the hole. Also, using an anonymous struct would hide this from the calling code:
include/exec/exec-all.h +7 -5
@@ -319,16 +319,18 @@ struct TranslationBlock {uint16_t size;/* size of target code for this block (1 <= size <= TARGET_PAGE_SIZE) */ uint16_t icount; -uint32_t cflags;/* compile flags */ + +struct { +void *tc_ptr;/* pointer to the translated code */ +int32_t out_size;/* size of host code for this block */ +uint32_t cflags;/* compile flags */ #define CF_COUNT_MASK 0x7fff#define CF_LAST_IO 0x8000/* Last insn may be an IO access. */ #define CF_NOCACHE 0x10000/* To be freed after execution */ #define CF_USE_ICOUNT 0x20000#define CF_IGNORE_ICOUNT 0x40000/* Do not generate icount code */ + }; -uint16_t invalid; - -void *tc_ptr;/* pointer to the translated code */ uint8_t *tc_search;/* pointer to search data */ /* original tb when cflags has CF_NOCACHE */ struct TranslationBlock *orig_tb;
@@ -365,7 +367,7 @@ struct TranslationBlock {*/ uintptr_t jmp_list_next[2 ];uintptr_t jmp_list_first; -int32_t out_size;/* size of host code for this block */ +uint16_t invalid; };
That is 122 bytes, with all 6 bytes of padding at the end. We also move invalid to the 2nd cache line, which I'm not sure it would matter much (I liked having out_size there because it's fairly slow path).
Also I'd rename out_size to tc_size.
E.
RFC 4/7 translate-all: report correct avg host TB size
In reply to Emilio G. CotaSince commit 6e3b2bfd6 ("tcg: allocate TB structs before the corresponding translated code") we are not fully utilizing code_gen_buffer for translated code, and therefore are incorrectly reporting the amount of translated code as well as the average host TB size. Address this by:
- Making the conscious choice of misreporting the total translated code; doing otherwise would mislead users into thinking "tb-size" is not honoured.
- Expanding tb_tree_stats to accurately count the bytes of translated code on the host, and using this for reporting the average tb host size, as well as the expansion ratio.
In the future we might want to consider reporting the accurate numbers for the total translated code, together with a "bookkeeping/overhead" field to account for the TB structs.
--- accel/tcg/translate-all.c | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-)
accel/tcg/translate-all.c +24 -10
@@ -894,9 +894,20 @@ static void page_flush_tb(void) } } +static __attribute__((unused)) +gboolean tb_host_size_iter (gpointer key,gpointer value,gpointer data) +{ +const TranslationBlock *tb = value; +int *size = data; + + *size += tb->out_size; +return false; +} +/* flush all the translation blocks */ static void do_tb_flush (CPUState *cpu,run_on_cpu_data tb_flush_count) { + int host_size __attribute__((unused)); int nb_tbs __attribute__((unused));tb_lock ();
@@ -909,12 +920,11 @@ static void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) }#if defined(DEBUG_TB_FLUSH ) +g_tree_foreach (tcg_ctx.tb_ctx.tb_tree, tb_host_size_iter, &host_size); nb_tbs =g_tree_nnodes (tcg_ctx.tb_ctx.tb_tree); -printf ("qemu: flush code_size=%ld nb_tbs=%d avg_tb_size=%ld\n" , +printf ("qemu: flush code_size=%ld nb_tbs=%d avg_tb_size=%d\n" , (unsigned long )(tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer), - nb_tbs, nb_tbs >0 ? - ((unsigned long )(tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer)) / - nb_tbs :0 ); + nb_tbs, nb_tbs >0 ? host_size / nb_tbs :0 );#endif if ((unsigned long )(tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer) > tcg_ctx.code_gen_buffer_size) {
@@ -1842,6 +1852,7 @@ static void print_qht_statistics(FILE *f, fprintf_function cpu_fprintf, }struct tb_tree_stats { +int host_size;int target_size;int max_target_size;int direct_jmp_count;
@@ -1854,6 +1865,7 @@ static gboolean tb_tree_stats_iter(gpointer key, gpointer value, gpointer data)const TranslationBlock *tb = value;struct tb_tree_stats *tst = data; + tst->host_size += tb->out_size; tst->target_size += tb->size;if (tb->size > tst->max_target_size) { tst->max_target_size = tb->size;
@@ -1882,6 +1894,11 @@ void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)g_tree_foreach (tcg_ctx.tb_ctx.tb_tree, tb_tree_stats_iter, &tst);/* XXX: avoid using doubles ? */ cpu_fprintf (f,"Translation buffer state:\n" ); +/* +* Report total code size including the padding and TB structs; +* otherwise users might think "-tb-size" is not honoured. +* For avg host size we use the precise numbers from tb_tree_stats though. +*/ cpu_fprintf (f,"gen code size %td/%zd\n" , tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer, tcg_ctx.code_gen_highwater - tcg_ctx.code_gen_buffer);
@@ -1889,12 +1906,9 @@ void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)cpu_fprintf (f,"TB avg target size %d max=%d bytes\n" , nb_tbs ? tst.target_size / nb_tbs :0 , tst.max_target_size); -cpu_fprintf (f,"TB avg host size %td bytes (expansion ratio: %0.1f)\n" , - nb_tbs ? (tcg_ctx.code_gen_ptr - - tcg_ctx.code_gen_buffer) / nb_tbs :0 , - tst.target_size ? (double ) (tcg_ctx.code_gen_ptr - - tcg_ctx.code_gen_buffer) / - tst.target_size :0 ); +cpu_fprintf (f,"TB avg host size %d bytes (expansion ratio: %0.1f)\n" , + nb_tbs ? tst.host_size / nb_tbs :0 , + tst.target_size ? (double )tst.host_size / tst.target_size :0 );cpu_fprintf (f,"cross page TB count %d (%d%%)\n" , tst.cross_page, nb_tbs ? (tst.cross_page *100 ) / nb_tbs :0 ); cpu_fprintf(f, "direct jump count %d (%d%%) (2 jumps=%d %d%%)\n",
Signature · 1 line
2.7.4
Re: [Qemu-devel] [RFC 4/7] translate-all: report correct avg host TB size
Quoted 1 line · Emilio G. Cota
+ int *size = data;
I'd really prefer you use size_t for all of these.
r~
RFC 5/7 tcg: take tb_ctx out of TCGContext
In reply to Emilio G. CotaBefore TCGContext is made thread-local.
--- include/exec/tb-context.h | 2 ++ tcg/tcg.h | 2 -- accel/tcg/cpu-exec.c | 2 +- accel/tcg/translate-all.c | 57 ++++++++++++++++++++++++----------------------- linux-user/main.c | 6 ++--- 5 files changed, 35 insertions(+), 34 deletions(-)
include/exec/tb-context.h +2 -0
@@ -41,4 +41,6 @@ struct TBContext { int tb_phys_invalidate_count; }; +extern TBContext tb_ctx; +#endif
tcg/tcg.h +0 -2
@@ -706,8 +706,6 @@ struct TCGContext {/* Threshold to flush the translated code buffer. */ void *code_gen_highwater; -TBContext tb_ctx; -/* Track which vCPU triggers events */ CPUState *cpu;/* *_trans */ TCGv_env tcg_env;/* *_exec */
accel/tcg/cpu-exec.c +1 -1
@@ -323,7 +323,7 @@ TranslationBlock *tb_htable_lookup(CPUState *cpu, target_ulong pc, phys_pc =get_page_addr_code (desc.env, pc); desc.phys_page1 = phys_pc &TARGET_PAGE_MASK ; h =tb_hash_func (phys_pc, pc, flags); -return qht_lookup (&tcg_ctx.tb_ctx.htable, tb_cmp, &desc, h); +return qht_lookup (&tb_ctx.htable, tb_cmp, &desc, h); } static inline TranslationBlock *tb_find(CPUState *cpu,
accel/tcg/translate-all.c +29 -28
186 lines not on this page · raw message
linux-user/main.c +3 -3
@@ -114,7 +114,7 @@ int cpu_get_pic_interrupt(CPUX86State *env)void fork_start (void ) {cpu_list_lock (); -qemu_mutex_lock (&tcg_ctx.tb_ctx.tb_lock); +qemu_mutex_lock (&tb_ctx.tb_lock);mmap_fork_start (); }
@@ -130,11 +130,11 @@ void fork_end(int child)QTAILQ_REMOVE (&cpus, cpu, node); } } -qemu_mutex_init (&tcg_ctx.tb_ctx.tb_lock); +qemu_mutex_init (&tb_ctx.tb_lock);qemu_init_cpu_list ();gdbserver_fork (thread_cpu); }else { -qemu_mutex_unlock (&tcg_ctx.tb_ctx.tb_lock); +qemu_mutex_unlock (&tb_ctx.tb_lock);cpu_list_unlock (); } }
Signature · 1 line
2.7.4
Re: [Qemu-devel] [RFC 5/7] tcg: take tb_ctx out of TCGContext
Quoted 10 lines · Emilio G. Cota
Before TCGContext is made thread-local.
Signed-off-by Emilio G. Cota<[email protected]>--- include/exec/tb-context.h | 2 ++ tcg/tcg.h | 2 -- accel/tcg/cpu-exec.c | 2 +- accel/tcg/translate-all.c | 57 ++++++++++++++++++++++++----------------------- linux-user/main.c | 6 ++--- 5 files changed, 35 insertions(+), 34 deletions(-)
There's a bit more of TCGContext that shouldn't be thread-local. But I guess this is the one that absolutely must be shared.
r~
RFC 6/7 tcg: make TCGContext thread-local for softmmu
In reply to Emilio G. CotaThis will allow us to generate TCG code in parallel.
User-mode is kept out of this: contention due to concurrent translation is more commonly found in full-system mode (e.g. booting a many-core guest).
XXX: For now, only convert arm/a64, since these are the only guests that have proper MTTCG support.
XXX: arm_translate_init needs to be called from a proper place.
XXX: TCG profiling info and statistics are broken by this
XXX: This is calling prologue_init once per vCPU, i.e. each TCGContext
gets a different prologue/epilogue (all of them with the same contents though). Far from ideal, but for an experiment it "should" work, right?
XXX: Giving the same amount of code_gen_buffer to each vCPU is certainly
a bad idea. A "page-like" allocation policy would be better, e.g. give chunks of 1MB to each vCPU as they need it. But for now I'm just trying to see whether this can ever work.
XXX: After allowing tb_gen_code to run in parallel (see next patch),
crashes due to races in TCG code are found very quickly with -smp > 1 (e.g. "tcg/tcg.c:233: tcg_out_label: Assertion `!l->has_value' failed.") Note that with -smp 1 it works fine; with smp > 1 I can make it fail later with "taskset -c 0", so clearly there is a race going on.
--- include/exec/exec-all.h | 4 +++- target/arm/translate.h | 8 +++---- tcg/tcg.h | 10 +++++++-- accel/tcg/translate-all.c | 56 ++++++++++++++++++++++++++++++++++++++++------ cpus.c | 2 ++ target/arm/cpu.c | 4 +++- target/arm/translate-a64.c | 6 ++--- target/arm/translate.c | 16 ++++++------- tcg/i386/tcg-target.inc.c | 2 +- tcg/tcg-common.c | 2 +- tcg/tcg.c | 6 ++--- 11 files changed, 85 insertions(+), 31 deletions(-)
include/exec/exec-all.h +3 -1
@@ -47,7 +47,9 @@ void gen_intermediate_code(CPUArchState *env, struct TranslationBlock *tb);void restore_state_to_opc (CPUArchState *env,struct TranslationBlock *tb,target_ulong *data); -void cpu_gen_init (void ); +#ifdef CONFIG_SOFTMMU +void cpu_gen_init (int cpu_index); +#endif bool cpu_restore_state (CPUState *cpu,uintptr_t searched_pc); void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu);
target/arm/translate.h +4 -4
@@ -76,10 +76,10 @@ typedef struct DisasCompare { } DisasCompare;/* Share the TCG temporaries common between 32 and 64 bit modes. */ -extern TCGv_env cpu_env; -extern TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF; -extern TCGv_i64 cpu_exclusive_addr; -extern TCGv_i64 cpu_exclusive_val; +extern TCG_THREAD TCGv_env cpu_env; +extern TCG_THREAD TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF; +extern TCG_THREAD TCGv_i64 cpu_exclusive_addr; +extern TCG_THREAD TCGv_i64 cpu_exclusive_val;static inline int arm_dc_feature (DisasContext *dc,int feature) {
tcg/tcg.h +8 -2
@@ -727,7 +727,13 @@ struct TCGContext {target_ulong gen_insn_data[TCG_MAX_INSNS ][TARGET_INSN_START_WORDS ]; }; -extern TCGContext tcg_ctx; +#ifdef CONFIG_SOFTMMU +#define TCG_THREAD __thread +#else +#define TCG_THREAD +#endif + +extern TCG_THREAD TCGContext tcg_ctx;extern bool parallel_cpus;static inline void tcg_set_insn_param (int op_idx,int arg,TCGArg v)
@@ -887,7 +893,7 @@ typedef struct TCGOpDef {#endif } TCGOpDef; -extern TCGOpDef tcg_op_defs[]; +extern TCG_THREAD TCGOpDef tcg_op_defs[];extern const size_t tcg_op_defs_max; typedef struct TCGTargetOpDef {
accel/tcg/translate-all.c +49 -7
100 lines not on this page · raw message
cpus.c +2 -0
@@ -1307,6 +1307,7 @@ static void *qemu_tcg_rr_cpu_thread_fn(void *arg)CPUState *cpu = arg;rcu_register_thread (); +cpu_gen_init (cpu->cpu_index);qemu_mutex_lock_iothread ();qemu_thread_get_self (cpu->thread);
@@ -1454,6 +1455,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)g_assert (!use_icount);rcu_register_thread (); +cpu_gen_init (cpu->cpu_index);qemu_mutex_lock_iothread ();qemu_thread_get_self (cpu->thread);
target/arm/cpu.c +3 -1
@@ -469,7 +469,7 @@ static void arm_cpu_initfn(Object *obj) {CPUState *cs =CPU (obj);ARMCPU *cpu =ARM_CPU (obj); -static bool inited; + static bool inited __attribute__((unused)); cs->env_ptr = &cpu->env; cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
@@ -511,10 +511,12 @@ static void arm_cpu_initfn(Object *obj)if (tcg_enabled ()) { cpu->psci_version =2 ;/* TCG implements PSCI 0.2 */ +#ifndef CONFIG_SOFTMMU if (!inited) { inited = true;arm_translate_init (); } +#endif } }
target/arm/translate-a64.c +3 -3
@@ -36,11 +36,11 @@#include "trace-tcg.h" -static TCGv_i64 cpu_X[32 ]; -static TCGv_i64 cpu_pc; +static TCG_THREAD TCGv_i64 cpu_X[32]; +static TCG_THREAD TCGv_i64 cpu_pc;/* Load/store exclusive handling */ -static TCGv_i64 cpu_exclusive_high; +static TCG_THREAD TCGv_i64 cpu_exclusive_high;static TCGv_i64 cpu_reg (DisasContext *s,int reg);static const char *regnames[] = {
target/arm/translate.c +8 -8
@@ -58,17 +58,17 @@#define IS_USER (s) (s->user)#endif -TCGv_env cpu_env; +TCG_THREAD TCGv_env cpu_env;/* We reuse the same 64-bit temporaries for efficiency. */ -static TCGv_i64 cpu_V0, cpu_V1, cpu_M0; -static TCGv_i32 cpu_R[16 ]; -TCGv_i32 cpu_CF, cpu_NF, cpu_VF, cpu_ZF; -TCGv_i64 cpu_exclusive_addr; -TCGv_i64 cpu_exclusive_val; +static TCG_THREAD TCGv_i64 cpu_V0, cpu_V1, cpu_M0; +static TCG_THREAD TCGv_i32 cpu_R[16]; +TCG_THREAD TCGv_i32 cpu_CF, cpu_NF, cpu_VF, cpu_ZF; +TCG_THREAD TCGv_i64 cpu_exclusive_addr; +TCG_THREAD TCGv_i64 cpu_exclusive_val;/* FIXME: These should be removed. */ -static TCGv_i32 cpu_F0s, cpu_F1s; -static TCGv_i64 cpu_F0d, cpu_F1d; +static TCG_THREAD TCGv_i32 cpu_F0s, cpu_F1s; +static TCG_THREAD TCGv_i64 cpu_F0d, cpu_F1d;#include "exec/gen-icount.h"
tcg/i386/tcg-target.inc.c +1 -1
@@ -146,7 +146,7 @@ static bool have_lzcnt;# define have_lzcnt 0#endif -static tcg_insn_unit *tb_ret_addr; +static TCG_THREAD tcg_insn_unit *tb_ret_addr;static void patch_reloc (tcg_insn_unit *code_ptr,int type,intptr_t value,intptr_t addend)
tcg/tcg-common.c +1 -1
@@ -31,7 +31,7 @@uintptr_t tci_tb_ptr;#endif -TCGOpDef tcg_op_defs[] = { +TCG_THREAD TCGOpDef tcg_op_defs[] = { #define DEF(s, oargs, iargs, cargs, flags) \ { #s, oargs, iargs, cargs, iargs + oargs + cargs, flags },#include "tcg-opc.h"
tcg/tcg.c +3 -3
@@ -117,8 +117,8 @@ static bool tcg_out_tb_finalize(TCGContext *s); -static TCGRegSet tcg_target_available_regs[2 ]; -static TCGRegSet tcg_target_call_clobber_regs; +static TCG_THREAD TCGRegSet tcg_target_available_regs[2]; +static TCG_THREAD TCGRegSet tcg_target_call_clobber_regs;#if TCG_TARGET_INSN_UNIT_SIZE ==1 static __attribute__((unused))inline void tcg_out8 (TCGContext *s,uint8_t v)
@@ -320,7 +320,7 @@ static const TCGHelperInfo all_helpers[] = {#include "exec/helper-tcg.h" }; -static int indirect_reg_alloc_order[ARRAY_SIZE (tcg_target_reg_alloc_order)]; +static TCG_THREAD int indirect_reg_alloc_order[ARRAY_SIZE(tcg_target_reg_alloc_order)];static void process_op_defs (TCGContext *s);void tcg_context_init (TCGContext *s)
Signature · 1 line
2.7.4
Re: [Qemu-devel] [RFC 6/7] [XXX] tcg: make TCGContext thread-local for softmmu
Quoted 7 lines · Emilio G. Cota
This will allow us to generate TCG code in parallel.
User-mode is kept out of this: contention due to concurrent translation is more commonly found in full-system mode (e.g. booting a many-core guest).
XXX: For now, only convert arm/a64, since these are the only guests that have proper MTTCG support.
Not true. TCG_GUEST_DEFAULT_MO suggests we have 4.
Quoted 4 lines
XXX: This is calling
prologue_initonce per vCPU, i.e. each TCGContext
gets a different prologue/epilogue (all of them with the same contents though). Far from ideal, but for an experiment it "should" work, right?
Um... sure. But surely there's a better way. Perhaps copying the main thread's tcg_ctx? That'd be after prologue, and target globals are created. That would avoid the need to make all the target cpu_* globals be thread-local, for instance.
Of course, now that I think of it, this implies that my tcg patches to use pointers instead of indicies is not the Way Forward...
target/arm/translate.h typedef struct DisasCompare { · line 82
Quoted 9 lines
/* Share the TCG temporaries common between 32 and 64 bit modes. */
target/arm/translate.h +4
-4-extern TCGv_env cpu_env; -extern TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF; -extern TCGv_i64 cpu_exclusive_addr; -extern TCGv_i64 cpu_exclusive_val; +extern TCG_THREAD TCGv_env cpu_env; +extern TCG_THREAD TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF; +extern TCG_THREAD TCGv_i64 cpu_exclusive_addr; +extern TCG_THREAD TCGv_i64 cpu_exclusive_val;
It would be Really Good if we could avoid this in the end. I see that it's the quickest way to test for now though.
tcg/tcg.h typedef struct TCGOpDef { · line 896
Quoted 6 lines
tcg/tcg.h +1
-1@@ -887,7 +893,7 @@ typedef struct TCGOpDef {#endif } TCGOpDef; -extern TCGOpDef tcg_op_defs[]; +extern TCG_THREAD TCGOpDef tcg_op_defs[];
Why? It's constant after startup.
r~
Re: [Qemu-devel] [RFC 6/7] [XXX] tcg: make TCGContext thread-local for softmmu
Quoted 10 lines · Richard Henderson
Quoted 7 lines · Emilio G. Cota
This will allow us to generate TCG code in parallel.
User-mode is kept out of this: contention due to concurrent translation is more commonly found in full-system mode (e.g. booting a many-core guest).
XXX: For now, only convert arm/a64, since these are the only guests that have proper MTTCG support.
Not true.
TCG_GUEST_DEFAULT_MOsuggests we have 4.
Yes, sorry that was outdated. I'm testing arm/a64 because I can boot many cores on a64.
Quoted 5 lines
Quoted 4 lines
XXX: This is calling
prologue_initonce per vCPU, i.e. each TCGContext
gets a different prologue/epilogue (all of them with the same contents though). Far from ideal, but for an experiment it "should" work, right?
Quoted 7 lines
Um... sure. But surely there's a better way. Perhaps copying the main thread's
tcg_ctx? That'd be after prologue, and target globals are created. That would avoid the need to make all the target cpu_* globals be thread-local, for instance.Of course, now that I think of it, this implies that my tcg patches to use pointers instead of indicies is not the Way Forward...
Didn't consider this, but after looking at the code (sans your patchset) I don't see why this wouldn't work -- would be great to avoid touching target code, as you point out below.
Quoted 13 lines
target/arm/translate.h typedef struct DisasCompare { · line 82
Quoted 9 lines
/* Share the TCG temporaries common between 32 and 64 bit modes. */
target/arm/translate.h +4
-4-extern TCGv_env cpu_env; -extern TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF; -extern TCGv_i64 cpu_exclusive_addr; -extern TCGv_i64 cpu_exclusive_val; +extern TCG_THREAD TCGv_env cpu_env; +extern TCG_THREAD TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF; +extern TCG_THREAD TCGv_i64 cpu_exclusive_addr; +extern TCG_THREAD TCGv_i64 cpu_exclusive_val;It would be Really Good if we could avoid this in the end. I see that it's the quickest way to test for now though.
Quoted 7 lines
tcg/tcg.h typedef struct TCGOpDef { · line 895
Quoted 5 lines
tcg/tcg.h +1
-1@@ -887,7 +893,7 @@ typedef struct TCGOpDef {#endif } TCGOpDef; -extern TCGOpDef tcg_op_defs[]; +extern TCG_THREAD TCGOpDef tcg_op_defs[];Why? It's constant after startup.
I was just going for a mass conversion first to see whether this could ever work. Will refine it.
Thanks,
E.
PATCH 1/1 fixup: missed some TLS variables
In reply to Emilio G. CotaQuoted 5 lines · Emilio G. Cota
XXX: After allowing
tb_gen_codeto run in parallel (see next patch),
crashes due to races in TCG code are found very quickly with -smp > 1 (e.g. "tcg/tcg.c:233:tcg_out_label: Assertion `!l->has_value' failed.") Note that with -smp 1 it works fine; with smp > 1 I can make it fail later with "taskset -c 0", so clearly there is a race going on.
Fixed! I missed a few __thread's -- see below.
I found the missing ones by moving tb_lock around and using 'valgrind --tool=drd' (it's not ThreadSanitizer, but it doesn't choke on our coroutines and it's reasonably fast).
Preliminary tests show even with patch 6 we don't gain almost anything when booting 64 cores (on a 64-core host), which is what I expected (we still have to acquire tb_lock on each translation, even if only for a few instructions, which is a scalability killer). But I'd worry about optimisations later; for now I'll focus on cleaning up the patchset, so my next steps are:
- Apply Richard's feedback so far
- Consolidate TLS variables into TCGContext, so that we'll have as little TLS variables as possible.
Cheers,
E.
--- 8< ----
--- include/exec/gen-icount.h | 2 +- tcg/optimize.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
include/exec/gen-icount.h +1 -1
@@ -6,7 +6,7 @@/* Helpers for instruction counting code generation. */ static int icount_start_insn_idx; -static TCGLabel *exitreq_label; +static TCG_THREAD TCGLabel *exitreq_label;static inline void gen_tb_start (TranslationBlock *tb) {
tcg/optimize.c +2 -2
@@ -40,8 +40,8 @@ struct tcg_temp_info {tcg_target_ulong mask; }; -static struct tcg_temp_info temps[TCG_MAX_TEMPS ]; -static TCGTempSet temps_used; +static TCG_THREAD struct tcg_temp_info temps[TCG_MAX_TEMPS]; +static TCG_THREAD TCGTempSet temps_used;static inline bool temp_is_const (TCGArg arg) {
Signature · 1 line
2.7.4
RFC 7/7 translate-all: generate TCG code without holding tb_lock
In reply to Emilio G. CotaGenerate new TBs without holding tb_lock, which should increase scalability when running MTTCG for workloads that generate a lot of code (e.g. booting linux).
XXX: Doesn't work :-) See the commit log from the previous patch.
--- accel/tcg/cpu-exec.c | 8 +++++++- accel/tcg/translate-all.c | 18 ++++++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-)
accel/tcg/cpu-exec.c +7 -1
@@ -351,6 +351,7 @@ static inline TranslationBlock *tb_find(CPUState *cpu,* single threaded the locks are NOPs. */ mmap_lock (); +#ifdef CONFIG_USER_ONLY tb_lock (); have_tb_lock = true;
@@ -362,7 +363,12 @@ static inline TranslationBlock *tb_find(CPUState *cpu,/* if no translated code available, then translate it now */ tb = tb_gen_code(cpu, pc, cs_base, flags, 0); } - +#else +/* tb_gen_code will acquire tb_lock. +* Just for the diff: note that have_tb_lock is local to tb_find! */ + have_tb_lock = true; + tb =tb_gen_code (cpu, pc, cs_base, flags,0 ); +#endif mmap_unlock (); }
accel/tcg/translate-all.c +16 -2
@@ -878,8 +878,6 @@ static TranslationBlock *tb_alloc(target_ulong pc) {TranslationBlock *tb; -assert_tb_locked (); - tb =tcg_tb_alloc (&tcg_ctx);if (unlikely (tb ==NULL )) {return NULL ;
@@ -1314,7 +1312,9 @@ TranslationBlock *tb_gen_code(CPUState *cpu,#ifdef CONFIG_PROFILER int64_t ti;#endif +#ifdef CONFIG_USER_ONLY assert_memory_lock (); +#endif phys_pc =get_page_addr_code (env, pc);if (use_icount && !(cflags &CF_IGNORE_ICOUNT )) {
@@ -1429,6 +1429,20 @@ TranslationBlock *tb_gen_code(CPUState *cpu,if ((pc &TARGET_PAGE_MASK ) != virt_page2) { phys_page2 =get_page_addr_code (env, virt_page2); } +if (!have_tb_lock) { +TranslationBlock *t; + +tb_lock (); +/* +* There's a chance that our desired tb has been translated while +* we were translating it. +*/ + t =tb_htable_lookup (cpu, pc, cs_base, flags); +if (unlikely (t)) { +/* this is very unlikely so just waste the TB space we just used */ +return t; + } + }/* As long as consistency of the TB stuff is provided by tb_lock in user * mode and is implicit in single-threaded softmmu emulation, no explicit * memory barrier is required before tb_link_page() makes the TB visible
Signature · 1 line
2.7.4
Quoted 4 lines · Emilio G. Cota
- Patches 2-3 remove *tbs[] to use a binary search tree instead. This removes the assumption in
tb_find_pcthat *tbs[] are ordered bytc_ptr, thereby allowing us to generate code regardless of its location on the host (as we do after patch 6).
Have you considered a scheme by which the front end translation and tcg optimization are done outside the lock, but final code generation is done inside the lock?
It would put at least half of the translation time in the parallel space without requiring changes to code_buffer allocation.
r~
Quoted 6 lines · Richard Henderson
Quoted 4 lines · Emilio G. Cota
- Patches 2-3 remove *tbs[] to use a binary search tree instead. This removes the assumption in
tb_find_pcthat *tbs[] are ordered bytc_ptr, thereby allowing us to generate code regardless of its location on the host (as we do after patch 6).
Quoted 6 lines
Have you considered a scheme by which the front end translation and tcg optimization are done outside the lock, but final code generation is done inside the lock?
It would put at least half of the translation time in the parallel space without requiring changes to
code_bufferallocation.
I don't think that would save much, because the performance issue comes from the fact that we have to grab the lock, regardless of how long we hold it. So even if we did nothing inside the lock, scalability when translating a lot of code (e.g. booting) would still be quite bad.
So we either get rid of the lock altogether, or use a more scalable lock.
E.