| Message ID | 20161116153705.30206-1-bobby.prani@gmail.com |
|---|---|
| State | New |
| Headers | show |
Pranith Kumar <bobby.prani@gmail.com> writes: > Unconditionally enable locking checks in debug builds so that we get > wider testing. Using tcg_debug_assert() allows us to remove > DEBUG_LOCKING define. Interesting. The other option would be to add a debug build to .travis.yml that define this (and others) with -DFOO_DEBUG. > > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > --- > translate-all.c | 50 +++++++++++++++++--------------------------------- > 1 file changed, 17 insertions(+), 33 deletions(-) > > diff --git a/translate-all.c b/translate-all.c > index cf828aa..a03f323 100644 > --- a/translate-all.c > +++ b/translate-all.c > @@ -60,7 +60,6 @@ > > /* #define DEBUG_TB_INVALIDATE */ > /* #define DEBUG_TB_FLUSH */ > -/* #define DEBUG_LOCKING */ > /* make various TB consistency checks */ > /* #define DEBUG_TB_CHECK */ So if we are enabling this for tcg_debug builds why not the other cases? > > @@ -75,23 +74,13 @@ > * access to the memory related structures are protected with the > * mmap_lock. > */ > -#ifdef DEBUG_LOCKING > -#define DEBUG_MEM_LOCKS 1 > -#else > -#define DEBUG_MEM_LOCKS 0 > -#endif > - In retrospect I should probably of had a comment in here about the roll of tb_lock in CONFIG_SOFTMMU versus the mmap_lock. > #ifdef CONFIG_SOFTMMU > #define assert_memory_lock() do { \ > - if (DEBUG_MEM_LOCKS) { \ > - g_assert(have_tb_lock); \ > - } \ > + tcg_debug_assert(have_tb_lock); \ > } while (0) > #else > #define assert_memory_lock() do { \ > - if (DEBUG_MEM_LOCKS) { \ > - g_assert(have_mmap_lock()); \ > - } \ > + tcg_debug_assert(have_mmap_lock()); \ > } while (0) > #endif > > @@ -172,16 +161,24 @@ static void page_table_config_init(void) > assert(v_l2_levels >= 0); > } > > +#define assert_tb_locked() do { \ > + tcg_debug_assert(have_tb_lock); \ > + } while (0) > + > +#define assert_tb_unlocked() do { \ > + tcg_debug_assert(!have_tb_lock); \ > + } while (0) > + I'm not sure we need all this multi-line stuff for a simple substitution? Richard? > void tb_lock(void) > { > - assert(!have_tb_lock); > + assert_tb_unlocked(); Hmm why introduce a helper for exactly one use? > qemu_mutex_lock(&tcg_ctx.tb_ctx.tb_lock); > have_tb_lock++; > } > > void tb_unlock(void) > { > - assert(have_tb_lock); > + assert_tb_locked(); > have_tb_lock--; > qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock); > } > @@ -194,19 +191,6 @@ void tb_lock_reset(void) > } > } > > -#ifdef DEBUG_LOCKING > -#define DEBUG_TB_LOCKS 1 > -#else > -#define DEBUG_TB_LOCKS 0 > -#endif > - > -#define assert_tb_lock() do { \ > - if (DEBUG_TB_LOCKS) { \ > - g_assert(have_tb_lock); \ > - } \ > - } while (0) > - > - > static TranslationBlock *tb_find_pc(uintptr_t tc_ptr); > > void cpu_gen_init(void) > @@ -840,7 +824,7 @@ static TranslationBlock *tb_alloc(target_ulong pc) > { > TranslationBlock *tb; > > - assert_tb_lock(); > + assert_tb_locked(); > > if (tcg_ctx.tb_ctx.nb_tbs >= tcg_ctx.code_gen_max_blocks) { > return NULL; > @@ -855,7 +839,7 @@ static TranslationBlock *tb_alloc(target_ulong pc) > /* Called with tb_lock held. */ > void tb_free(TranslationBlock *tb) > { > - assert_tb_lock(); > + assert_tb_locked(); > > /* In practice this is mostly used for single use temporary TB > Ignore the hard cases and just back up if this TB happens to > @@ -1097,7 +1081,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr) > uint32_t h; > tb_page_addr_t phys_pc; > > - assert_tb_lock(); > + assert_tb_locked(); > > atomic_set(&tb->invalid, true); > > @@ -1412,7 +1396,7 @@ static void tb_invalidate_phys_range_1(tb_page_addr_t start, tb_page_addr_t end) > #ifdef CONFIG_SOFTMMU > void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end) > { > - assert_tb_lock(); > + assert_tb_locked(); > tb_invalidate_phys_range_1(start, end); > } > #else > @@ -1455,7 +1439,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end, > #endif /* TARGET_HAS_PRECISE_SMC */ > > assert_memory_lock(); > - assert_tb_lock(); > + assert_tb_locked(); > > p = page_find(start >> TARGET_PAGE_BITS); > if (!p) { -- Alex Bennée
On 11/16/2016 04:57 PM, Alex Bennée wrote: >> > +#define assert_tb_locked() do { \ >> > + tcg_debug_assert(have_tb_lock); \ >> > + } while (0) >> > + >> > +#define assert_tb_unlocked() do { \ >> > + tcg_debug_assert(!have_tb_lock); \ >> > + } while (0) >> > + > I'm not sure we need all this multi-line stuff for a simple > substitution? Richard? > I wouldn't think so, no. r~
On Wed, Nov 16, 2016 at 10:57 AM, Alex Bennée <alex.bennee@linaro.org> wrote: > > Pranith Kumar <bobby.prani@gmail.com> writes: > >> Unconditionally enable locking checks in debug builds so that we get >> wider testing. Using tcg_debug_assert() allows us to remove >> DEBUG_LOCKING define. > > Interesting. The other option would be to add a debug build to > .travis.yml that define this (and others) with -DFOO_DEBUG. > >> >> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> >> --- >> translate-all.c | 50 +++++++++++++++++--------------------------------- >> 1 file changed, 17 insertions(+), 33 deletions(-) >> >> diff --git a/translate-all.c b/translate-all.c >> index cf828aa..a03f323 100644 >> --- a/translate-all.c >> +++ b/translate-all.c >> @@ -60,7 +60,6 @@ >> >> /* #define DEBUG_TB_INVALIDATE */ >> /* #define DEBUG_TB_FLUSH */ >> -/* #define DEBUG_LOCKING */ >> /* make various TB consistency checks */ >> /* #define DEBUG_TB_CHECK */ > > So if we are enabling this for tcg_debug builds why not the other cases? Ideally, we should enable all the debug checks in the debug build. I didn't want to touch unrelated stuff in this patch. I can clean up all these cases if you prefer. > >> >> @@ -75,23 +74,13 @@ >> * access to the memory related structures are protected with the >> * mmap_lock. >> */ >> -#ifdef DEBUG_LOCKING >> -#define DEBUG_MEM_LOCKS 1 >> -#else >> -#define DEBUG_MEM_LOCKS 0 >> -#endif >> - > > In retrospect I should probably of had a comment in here about the roll > of tb_lock in CONFIG_SOFTMMU versus the mmap_lock. > >> #ifdef CONFIG_SOFTMMU >> #define assert_memory_lock() do { \ >> - if (DEBUG_MEM_LOCKS) { \ >> - g_assert(have_tb_lock); \ >> - } \ >> + tcg_debug_assert(have_tb_lock); \ >> } while (0) >> #else >> #define assert_memory_lock() do { \ >> - if (DEBUG_MEM_LOCKS) { \ >> - g_assert(have_mmap_lock()); \ >> - } \ >> + tcg_debug_assert(have_mmap_lock()); \ >> } while (0) >> #endif >> >> @@ -172,16 +161,24 @@ static void page_table_config_init(void) >> assert(v_l2_levels >= 0); >> } >> >> +#define assert_tb_locked() do { \ >> + tcg_debug_assert(have_tb_lock); \ >> + } while (0) >> + >> +#define assert_tb_unlocked() do { \ >> + tcg_debug_assert(!have_tb_lock); \ >> + } while (0) >> + > > I'm not sure we need all this multi-line stuff for a simple > substitution? Richard? OK, I will update this to a single line macro... > >> void tb_lock(void) >> { >> - assert(!have_tb_lock); >> + assert_tb_unlocked(); > > Hmm why introduce a helper for exactly one use? ...or can entirely remove the macro. I don't favour one over the other. :)
diff --git a/translate-all.c b/translate-all.c index cf828aa..a03f323 100644 --- a/translate-all.c +++ b/translate-all.c @@ -60,7 +60,6 @@ /* #define DEBUG_TB_INVALIDATE */ /* #define DEBUG_TB_FLUSH */ -/* #define DEBUG_LOCKING */ /* make various TB consistency checks */ /* #define DEBUG_TB_CHECK */ @@ -75,23 +74,13 @@ * access to the memory related structures are protected with the * mmap_lock. */ -#ifdef DEBUG_LOCKING -#define DEBUG_MEM_LOCKS 1 -#else -#define DEBUG_MEM_LOCKS 0 -#endif - #ifdef CONFIG_SOFTMMU #define assert_memory_lock() do { \ - if (DEBUG_MEM_LOCKS) { \ - g_assert(have_tb_lock); \ - } \ + tcg_debug_assert(have_tb_lock); \ } while (0) #else #define assert_memory_lock() do { \ - if (DEBUG_MEM_LOCKS) { \ - g_assert(have_mmap_lock()); \ - } \ + tcg_debug_assert(have_mmap_lock()); \ } while (0) #endif @@ -172,16 +161,24 @@ static void page_table_config_init(void) assert(v_l2_levels >= 0); } +#define assert_tb_locked() do { \ + tcg_debug_assert(have_tb_lock); \ + } while (0) + +#define assert_tb_unlocked() do { \ + tcg_debug_assert(!have_tb_lock); \ + } while (0) + void tb_lock(void) { - assert(!have_tb_lock); + assert_tb_unlocked(); qemu_mutex_lock(&tcg_ctx.tb_ctx.tb_lock); have_tb_lock++; } void tb_unlock(void) { - assert(have_tb_lock); + assert_tb_locked(); have_tb_lock--; qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock); } @@ -194,19 +191,6 @@ void tb_lock_reset(void) } } -#ifdef DEBUG_LOCKING -#define DEBUG_TB_LOCKS 1 -#else -#define DEBUG_TB_LOCKS 0 -#endif - -#define assert_tb_lock() do { \ - if (DEBUG_TB_LOCKS) { \ - g_assert(have_tb_lock); \ - } \ - } while (0) - - static TranslationBlock *tb_find_pc(uintptr_t tc_ptr); void cpu_gen_init(void) @@ -840,7 +824,7 @@ static TranslationBlock *tb_alloc(target_ulong pc) { TranslationBlock *tb; - assert_tb_lock(); + assert_tb_locked(); if (tcg_ctx.tb_ctx.nb_tbs >= tcg_ctx.code_gen_max_blocks) { return NULL; @@ -855,7 +839,7 @@ static TranslationBlock *tb_alloc(target_ulong pc) /* Called with tb_lock held. */ void tb_free(TranslationBlock *tb) { - assert_tb_lock(); + assert_tb_locked(); /* In practice this is mostly used for single use temporary TB Ignore the hard cases and just back up if this TB happens to @@ -1097,7 +1081,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr) uint32_t h; tb_page_addr_t phys_pc; - assert_tb_lock(); + assert_tb_locked(); atomic_set(&tb->invalid, true); @@ -1412,7 +1396,7 @@ static void tb_invalidate_phys_range_1(tb_page_addr_t start, tb_page_addr_t end) #ifdef CONFIG_SOFTMMU void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end) { - assert_tb_lock(); + assert_tb_locked(); tb_invalidate_phys_range_1(start, end); } #else @@ -1455,7 +1439,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end, #endif /* TARGET_HAS_PRECISE_SMC */ assert_memory_lock(); - assert_tb_lock(); + assert_tb_locked(); p = page_find(start >> TARGET_PAGE_BITS); if (!p) {
Unconditionally enable locking checks in debug builds so that we get wider testing. Using tcg_debug_assert() allows us to remove DEBUG_LOCKING define. Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> --- translate-all.c | 50 +++++++++++++++++--------------------------------- 1 file changed, 17 insertions(+), 33 deletions(-)