On 10/7/19 11:28 AM, Alex Bennée wrote: > From: "Vanderson M. do Rosario" <vanderson...@gmail.com> > > These commands allow the exploration of TBs generated by the TCG. > Understand which one hotter, with more guest/host instructions... and > examine their guest, host and IR code. > > The goal of this command is to allow the dynamic exploration of TCG > behavior and code quality. Therefore, for now, a corresponding QMP > command is not worthwhile. > > [AJB: WIP - we still can't be safely sure a translation will succeed] > > Example of output: > > TB id:1 | phys:0x34d54 virt:0x0000000000034d54 flags:0x0000f0 > | exec:4828932/0 guest inst cov:16.38% > | trans:1 ints: g:3 op:82 op_opt:34 spills:3 > | h/g (host bytes / guest insts): 90.666664 > | time to gen at 2.4GHz => code:3150.83(ns) IR:712.08(ns) > | targets: 0x0000000000034d5e (id:11), 0x0000000000034d0d (id:2) > > TB id:2 | phys:0x34d0d virt:0x0000000000034d0d flags:0x0000f0 > | exec:4825842/0 guest inst cov:21.82% > | trans:1 ints: g:4 op:80 op_opt:38 spills:2 > | h/g (host bytes / guest insts): 84.000000 > | time to gen at 2.4GHz => code:3362.92(ns) IR:793.75(ns) > | targets: 0x0000000000034d19 (id:12), 0x0000000000034d54 (id:1) > > TB id:2 | phys:0x34d0d virt:0x0000000000034d0d flags:0x0000f0 > | exec:6956495/0 guest inst cov:21.82% > | trans:2 ints: g:2 op:40 op_opt:19 spills:1 > | h/g (host bytes / guest insts): 84.000000 > | time to gen at 2.4GHz => code:3130.83(ns) IR:722.50(ns) > | targets: 0x0000000000034d19 (id:12), 0x0000000000034d54 (id:1) > > ---------------- > IN: > 0x00034d0d: 89 de movl %ebx, %esi > 0x00034d0f: 26 8b 0e movl %es:(%esi), %ecx > 0x00034d12: 26 f6 46 08 80 testb $0x80, %es:8(%esi) > 0x00034d17: 75 3b jne 0x34d54 > > ------------------------------ > > TB id:1 | phys:0x34d54 virt:0x0000000000034d54 flags:0x0000f0 > | exec:5202686/0 guest inst cov:11.28% > | trans:1 ints: g:3 op:82 op_opt:34 spills:3 > | h/g (host bytes / guest insts): 90.666664 > | time to gen at 2.4GHz => code:2793.75(ns) IR:614.58(ns) > | targets: 0x0000000000034d5e (id:3), 0x0000000000034d0d (id:2) > > TB id:2 | phys:0x34d0d virt:0x0000000000034d0d flags:0x0000f0 > | exec:5199468/0 guest inst cov:15.03% > | trans:1 ints: g:4 op:80 op_opt:38 spills:2 > | h/g (host bytes / guest insts): 84.000000 > | time to gen at 2.4GHz => code:2958.75(ns) IR:719.58(ns) > | targets: 0x0000000000034d19 (id:4), 0x0000000000034d54 (id:1) > > ------------------------------ > 2 TBs to reach 25% of guest inst exec coverage > Total of guest insts exec: 138346727 >
Is there too much cut-and-paste in this commit message? I certainly hope that identical information about TB id:2 is not output 3 times within the same report... Or, alternately, that we are not generating multiple TBs for the { phys:0x34d0d virt:0x34d0d flags:0xf0 } tuple. Also, I think you probably need to output cs_base. Depending on the target, that might have been different, and so it might make sense that you have 3 copies of the above. > +static gint > +inverse_sort_tbs(gconstpointer p1, gconstpointer p2, gpointer psort_by) What about this makes the sort "inverse"? > + int sort_by = *((int *) psort_by); Why is this not enum SortBy? > + if (likely(sort_by == SORT_BY_SPILLS)) { ... > + } else if (likely(sort_by == SORT_BY_HOTNESS)) { ... > + } else if (likely(sort_by == SORT_BY_HG)) { Surely these options are not all "likely". > + float a = > + (float) stat_per_translation(tbs1, code.out_len) / > tbs1->code.num_guest_inst; > + float b = > + (float) stat_per_translation(tbs2, code.out_len) / > tbs2->code.num_guest_inst; I don't understand why we're suddenly introducing floats, when the division hidden within stat_per_translation is integer. Think about what units being compared here, because I don't think that "average host code length / sum of guest insn count" makes sense. Certainly as time progresses, average / sum -> ((sum / sum) / sum) -> (sum / sum**2) -> (1 / sum) is going to approach 0. I think you actually want "sum of host code length / sum of guest insn length", which could be stated as "host/guest code ratio" or "jit code expansion factor". > + c1 = a <= b ? 0 : 1; > + c2 = a <= b ? 1 : 0; Please do recall that (x < y ? 1 : 0) => (x < y). In addition, there's no point in having these comparisons feed... > + return c1 < c2 ? 1 : c1 == c2 ? 0 : -1; ... these comparisions. > + for (i = last_search; i; i = i->next) { > + TBStatistics *tbs = (TBStatistics *) i->data; > + uint64_t tb_total_execs = > + (tbs->executions.atomic + tbs->executions.normal) * > tbs->code.num_guest_inst; > + tbs->executions.coverage = (10000 * tb_total_execs) / > (total_exec_count + 1); If ever there was a time to want to use float, this is it. I assume the total_exec_count + 1 is to avoid divide by zero? Surely we can do better than this... Indeed, given that we've already checked... > + if (!last_search) { > + qemu_printf("No data collected yet!\n"); > + return; > + } ... surely we can either assert total_exec_count != 0, or don't and just let the divide-by-zero signal do the same thing. (I don't see the value of replacing one signal with another in most cases.) > +/* > + * We cannot always re-generate the code even if we know there are > + * valid translations still in the cache. The reason being the guest > + * may have un-mapped the page code. Um... unless I mistake what's being described here, that wouldn't be a valid translation. Or do you just mean that the page mapping isn't present within the TLB? Which is not quite the same thing as "unmapping". > + * TODO: can we do this safely? We need to > + * a) somehow recover the mmu_idx for this translation We could add an interface for this, yes. The value *must* be able to be derived from tb->flags, but of course in a target-dependent way. > + * b) probe MMU_INST_FETCH to know it will succeed We *do* have this now, sort of: tlb_vaddr_to_host. So far all use of this function originates from target/foo/, and so some targets have not been updated to work with this. I've marked these with asserts within foo_cpu_tlb_fill. Notable targets for which it won't work: i386, sparc. > +static GString *get_code_string(TBStatistics *tbs, int log_flags) > +{ > + int old_log_flags = qemu_loglevel; > + > + CPUState *cpu = first_cpu; > + uint32_t cflags = curr_cflags() | CF_NOCACHE; > + TranslationBlock *tb = NULL; > + > + GString *code_s = g_string_new(NULL); > + qemu_log_to_string(true, code_s); > + > + qemu_set_log(log_flags); > + > + if (sigsetjmp(cpu->jmp_env, 0) == 0) { > + mmap_lock(); > + tb = tb_gen_code(cpu, tbs->pc, tbs->cs_base, tbs->flags, cflags); > + tb_phys_invalidate(tb, -1); > + mmap_unlock(); Ew. No. Let us not go through tb_gen_code, just to get logging output from the translator. What are we really after here? Input assembly? > @@ -86,7 +91,6 @@ struct TBStatistics { > > struct { > unsigned long total; > - unsigned long uncached; > unsigned long spanning; > } translations; > Vanishing unused variable that maybe shouldn't have existed? r~