From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42831) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X72bo-00045f-Jm for qemu-devel@nongnu.org; Tue, 15 Jul 2014 09:17:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X72bh-0003Re-3s for qemu-devel@nongnu.org; Tue, 15 Jul 2014 09:17:28 -0400 Received: from static.88-198-71-155.clients.your-server.de ([88.198.71.155]:59673 helo=socrates.bennee.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X72bg-0003R0-Tz for qemu-devel@nongnu.org; Tue, 15 Jul 2014 09:17:21 -0400 References: <1405424541-21803-1-git-send-email-alex.bennee@linaro.org> <1405424541-21803-4-git-send-email-alex.bennee@linaro.org> <53C51B56.6010509@suse.de> From: Alex =?utf-8?Q?Benn=C3=A9e?= Date: Tue, 15 Jul 2014 14:12:13 +0100 In-reply-to: <53C51B56.6010509@suse.de> Message-ID: <8738e2yd40.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 3/3] trace: instrument and trace tcg tb flush activity List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?utf-8?Q?F=C3=A4rber?= Cc: qemu-devel@nongnu.org, stefanha@redhat.com, mohamad.gebai@gmail.com Andreas Färber writes: > Hi, > > Am 15.07.2014 13:42, schrieb Alex Bennée: >> index df977c8..8376678 100644 >> --- a/include/qom/cpu.h >> +++ b/include/qom/cpu.h >> @@ -243,6 +243,10 @@ struct CPUState { >> void *env_ptr; /* CPUArchState */ >> struct TranslationBlock *current_tb; >> struct TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE]; >> + struct { >> + int hits; >> + int misses; > > Is anything else going to be added here? If not, the indentation can be > dropped. At the moment probably not. > >> + } tb_jmp_cache_stats; > > This is lacking documentation. Should be trivial to add for this field > (not here, above the struct). To document the subfields we may need to > name the struct. Would not a simple comment be enough? > >> struct GDBRegisterState *gdb_regs; >> int gdb_num_regs; >> int gdb_num_g_regs; >> @@ -584,6 +588,15 @@ void cpu_exit(CPUState *cpu); >> */ >> void cpu_resume(CPUState *cpu); >> >> + >> +/** >> + * tb_flush_all_jmp_cache: >> + * @cpu: The CPU jmp cache to flush >> + * >> + * Flush all the entries from the cpu fast jump cache > > "CPU" for consistency OK > >> +#ifndef CONFIG_TRACE_NOP >> +static inline void trace_inc_counter(int *counter) { >> + int cnt = *counter; >> + cnt++; >> + *counter = cnt; >> +} >> +#else >> +static inline void trace_inc_counter(int *counter) { /* do nothing */ } >> +#endif >> + >> #endif /* TRACE_H */ > > Coding Style issues with the first function. For simplicity just keep > the first implementation but with the proper brace placement, and then > just put the #ifdef into the function body. That avoids the signatures > getting out of sync. mea-culpa, I forgot to run checkpatch.pl.... >> >> memset(tcg_ctx.tb_ctx.tb_phys_hash, 0, sizeof(tcg_ctx.tb_ctx.tb_phys_hash)); >> @@ -1520,6 +1530,8 @@ void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr) >> i = tb_jmp_cache_hash_page(addr); >> memset(&cpu->tb_jmp_cache[i], 0, >> TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *)); > > Can this one be dropped, too? No, this is only a partial invalidation. I did toy with instrumenting how many entries are flushed but it didn't seem the be worth it given with the current architecture there is not much we can do. > >> + >> + trace_tb_flush_jump_cache(addr); >> } >> >> void dump_exec_info(FILE *f, fprintf_function cpu_fprintf) > > Cheers, > Andreas -- Alex Bennée