| Message ID | 1438593291-27109-12-git-send-email-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
Aurelien Jarno <aurelien@aurel32.net> writes: > On 2015-08-03 10:14, Alex Bennée wrote: >> To avoid cluttering the code with #ifdef legs we wrap up the print >> statements into a tlb_debug() macro. As access to the virtual TLB can >> get quite heavy defining DEBUG_TLB_LOG will ensure all the logs go to >> the qemu_log target of CPU_LOG_MMU instead of stderr. >> >> I've also removed DEBUG_TLB_CHECK which wasn't used. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> --- >> v2 >> - ensure the compiler checks format strings even if debug is optimised out >> --- >> cputlb.c | 54 +++++++++++++++++++++++++++++++++++------------------- >> 1 file changed, 35 insertions(+), 19 deletions(-) >> >> diff --git a/cputlb.c b/cputlb.c >> index a506086..7095e6f 100644 >> --- a/cputlb.c >> +++ b/cputlb.c >> @@ -30,8 +30,30 @@ >> #include "exec/ram_addr.h" >> #include "tcg/tcg.h" >> >> -//#define DEBUG_TLB >> -//#define DEBUG_TLB_CHECK >> +/* DEBUG defines, enable DEBUG_TLB_LOG to log to the CPU_LOG_MMU target */ >> +/* #define DEBUG_TLB */ >> +/* #define DEBUG_TLB_LOG */ >> + >> +#ifdef DEBUG_TLB >> +# define DEBUG_TLB_GATE 1 >> +# ifdef DEBUG_TLB_LOG >> +# define DEBUG_TLB_LOG_GATE 1 >> +# else >> +# define DEBUG_TLB_LOG_GATE 0 >> +# endif >> +#else >> +# define DEBUG_TLB_GATE 0 >> +# define DEBUG_TLB_LOG_GATE 0 >> +#endif >> + >> +#define tlb_debug(fmt, ...) do { \ >> + if (DEBUG_TLB_LOG_GATE) { \ >> + qemu_log_mask(CPU_LOG_MMU, "%s: " fmt, __func__, \ >> + ## __VA_ARGS__); \ >> + } else if (DEBUG_TLB_GATE) { \ >> + fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \ >> + } \ >> +} while (0) > > Do we really want to support sending debug logs to either the logfile or > stderr? It's already possible to send the debug logs through stderr when > not specifying -D file. It preserves the old behaviour (and the general behaviour of DEBUG_FOO going to stderr). However I'm happy to make it default to using the log output. It does raise the question of if we should just enable the debugging by default? -- Alex Bennée
On 3 February 2016 at 18:54, Alex Bennée <alex.bennee@linaro.org> wrote: > It preserves the old behaviour (and the general behaviour of DEBUG_FOO > going to stderr). However I'm happy to make it default to using the log > output. > > It does raise the question of if we should just enable the debugging by > default? Not without thinking carefully about it. This is programmer debug code for figuring out what's happening in a performance sensitive bit of code. "Just print to stderr" is the classic way to do this, and I don't think we should just convert that into userfacing trace. There may be useful user facing trace we can do of TLB operations but I wouldn't assume that our current debug printfs are it. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 3 February 2016 at 18:54, Alex Bennée <alex.bennee@linaro.org> wrote: >> It preserves the old behaviour (and the general behaviour of DEBUG_FOO >> going to stderr). However I'm happy to make it default to using the log >> output. >> >> It does raise the question of if we should just enable the debugging by >> default? > > Not without thinking carefully about it. This is programmer > debug code for figuring out what's happening in a performance > sensitive bit of code. "Just print to stderr" is the classic > way to do this, and I don't think we should just convert that > into userfacing trace. Shall I just go back to the original fprintf output then? I made the output optional a few review comments back. > > There may be useful user facing trace we can do of TLB > operations but I wouldn't assume that our current debug > printfs are it. > > thanks > -- PMM -- Alex Bennée
diff --git a/cputlb.c b/cputlb.c index a506086..7095e6f 100644 --- a/cputlb.c +++ b/cputlb.c @@ -30,8 +30,30 @@ #include "exec/ram_addr.h" #include "tcg/tcg.h" -//#define DEBUG_TLB -//#define DEBUG_TLB_CHECK +/* DEBUG defines, enable DEBUG_TLB_LOG to log to the CPU_LOG_MMU target */ +/* #define DEBUG_TLB */ +/* #define DEBUG_TLB_LOG */ + +#ifdef DEBUG_TLB +# define DEBUG_TLB_GATE 1 +# ifdef DEBUG_TLB_LOG +# define DEBUG_TLB_LOG_GATE 1 +# else +# define DEBUG_TLB_LOG_GATE 0 +# endif +#else +# define DEBUG_TLB_GATE 0 +# define DEBUG_TLB_LOG_GATE 0 +#endif + +#define tlb_debug(fmt, ...) do { \ + if (DEBUG_TLB_LOG_GATE) { \ + qemu_log_mask(CPU_LOG_MMU, "%s: " fmt, __func__, \ + ## __VA_ARGS__); \ + } else if (DEBUG_TLB_GATE) { \ + fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \ + } \ +} while (0) /* statistics */ int tlb_flush_count; @@ -52,9 +74,8 @@ void tlb_flush(CPUState *cpu, int flush_global) { CPUArchState *env = cpu->env_ptr; -#if defined(DEBUG_TLB) - printf("tlb_flush:\n"); -#endif + tlb_debug("(%d)\n", flush_global); + /* must reset current TB so that interrupts cannot modify the links while we are modifying them */ cpu->current_tb = NULL; @@ -87,16 +108,14 @@ void tlb_flush_page(CPUState *cpu, target_ulong addr) int i; int mmu_idx; -#if defined(DEBUG_TLB) - printf("tlb_flush_page: " TARGET_FMT_lx "\n", addr); -#endif + tlb_debug("page :" TARGET_FMT_lx "\n", addr); + /* Check if we need to flush due to large pages. */ if ((addr & env->tlb_flush_mask) == env->tlb_flush_addr) { -#if defined(DEBUG_TLB) - printf("tlb_flush_page: forced full flush (" - TARGET_FMT_lx "/" TARGET_FMT_lx ")\n", - env->tlb_flush_addr, env->tlb_flush_mask); -#endif + tlb_debug("forcing full flush (" + TARGET_FMT_lx "/" TARGET_FMT_lx ")\n", + env->tlb_flush_addr, env->tlb_flush_mask); + tlb_flush(cpu, 1); return; } @@ -271,12 +290,9 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, section = address_space_translate_for_iotlb(cpu, paddr, &xlat, &sz); assert(sz >= TARGET_PAGE_SIZE); -#if defined(DEBUG_TLB) - qemu_log_mask(CPU_LOG_MMU, - "tlb_set_page: vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx - " prot=%x idx=%d\n", - vaddr, paddr, prot, mmu_idx); -#endif + tlb_debug("tlb_set_page: vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx + " prot=%x idx=%d\n", + vaddr, paddr, prot, mmu_idx); address = vaddr; if (!memory_region_is_ram(section->mr) && !memory_region_is_romd(section->mr)) {
To avoid cluttering the code with #ifdef legs we wrap up the print statements into a tlb_debug() macro. As access to the virtual TLB can get quite heavy defining DEBUG_TLB_LOG will ensure all the logs go to the qemu_log target of CPU_LOG_MMU instead of stderr. I've also removed DEBUG_TLB_CHECK which wasn't used. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- v2 - ensure the compiler checks format strings even if debug is optimised out --- cputlb.c | 54 +++++++++++++++++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 19 deletions(-)