On 05/07/16 16:42, Paolo Bonzini wrote: > > On 05/07/2016 15:11, Alex Bennée wrote: >> Paolo Bonzini <[email protected]> writes: >> >>> On 05/07/2016 13:14, Alex Bennée wrote: >>>> /* >>>> * Patch the last TB with a jump to the current TB. >>>> * >>>> * Modification of the TB has to be protected with tb_lock which can >>>> * either be already held or taken here. >>>> */ >>>> static inline void maybe_patch_last_tb(CPUState *cpu, >>>> TranslationBlock *tb, >>>> TranslationBlock **last_tb, >>>> int tb_exit, >>>> bool locked) >>>> { >>>> if (cpu->tb_flushed) { >>>> /* Ensure that no TB jump will be modified as the >>>> * translation buffer has been flushed. >>>> */ >>>> *last_tb = NULL; >>>> cpu->tb_flushed = false; >>>> } >>>> #ifndef CONFIG_USER_ONLY >>>> /* We don't take care of direct jumps when address mapping changes in >>>> * system emulation. So it's not safe to make a direct jump to a TB >>>> * spanning two pages because the mapping for the second page can >>>> change. >>>> */ >>>> if (tb->page_addr[1] != -1) { >>>> *last_tb = NULL; >>>> } >>>> #endif >>>> /* See if we can patch the calling TB. */ >>>> if (*last_tb && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) { >>>> if (!locked) { >>>> tb_lock(); >>>> } >>>> tb_add_jump(*last_tb, tb_exit, tb); >>>> if (!locked) { >>>> tb_unlock(); >>>> } >>>> } >>>> } >>> Why not add tb_lock_recursive() and tb_lock_reset()? >> I thought we didn't like having recursive locking? I agree it would make >> things a little neater though. > I didn't like having recursive mutexes (because recursive mutexes > encourage you to be sloppy). Explicitly tagging some tb_lock()s as > recursive is fine, though. The explicit tag tells you to be careful.
We could also introduce mmap_lock_reset() similar to tb_lock_reset(). Then we can remove tb_unlock() and mmap_unlock() from tb_find_slow(), call tb_lock() and mmap_lock() right before tb_add_jump(), and then call tb_lock_reset() and mmap_lock_reset() at the end of tb_find_fast(). This would render tb_lock() pretty useless though, since it would be always held under mmap_lock(). Kind regards, Sergey
