Re: [Qemu-devel] [PATCH v3 10/11] tcg: Avoid bouncing tb_lock between tb_gen_code() and tb_add_jump()

[Alex Bennée]

Sergey Fedorov <address@hidden> writes:

> From: Sergey Fedorov <address@hidden>
>
> Signed-off-by: Sergey Fedorov <address@hidden>
> Signed-off-by: Sergey Fedorov <address@hidden>

Reviewed-by: Alex Bennée <address@hidden>

Much better than my cack-hander attempt to clean this up ;-)

TBH I'd be up for merging this with patch 11 but I'm happy to defer to
the maintainers on this one.

> ---
>  cpu-exec.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 4eabd534aba0..22c672fe03fd 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -281,7 +281,8 @@ static TranslationBlock *tb_find_physical(CPUState *cpu,
>  static TranslationBlock *tb_find_slow(CPUState *cpu,
>                                        target_ulong pc,
>                                        target_ulong cs_base,
> -                                      uint32_t flags)
> +                                      uint32_t flags,
> +                                      bool *have_tb_lock)
>  {
>      TranslationBlock *tb;
>
> @@ -294,6 +295,7 @@ static TranslationBlock *tb_find_slow(CPUState *cpu,
>           */
>          mmap_lock();
>          tb_lock();
> +        *have_tb_lock = true;
>
>          /* There's a chance that our desired tb has been translated while
>           * taking the locks so we check again inside the lock.
> @@ -304,7 +306,6 @@ static TranslationBlock *tb_find_slow(CPUState *cpu,
>              tb = tb_gen_code(cpu, pc, cs_base, flags, 0);
>          }
>
> -        tb_unlock();
>          mmap_unlock();
>      }
>
> @@ -321,6 +322,7 @@ static inline TranslationBlock *tb_find_fast(CPUState 
> *cpu,
>      TranslationBlock *tb;
>      target_ulong cs_base, pc;
>      uint32_t flags;
> +    bool have_tb_lock = false;
>
>      /* we record a subset of the CPU state. It will
>         always be the same before a given translated block
> @@ -329,7 +331,7 @@ static inline TranslationBlock *tb_find_fast(CPUState 
> *cpu,
>      tb = atomic_read(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)]);
>      if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
>                   tb->flags != flags)) {
> -        tb = tb_find_slow(cpu, pc, cs_base, flags);
> +        tb = tb_find_slow(cpu, pc, cs_base, flags, &have_tb_lock);
>      }
>  #ifndef CONFIG_USER_ONLY
>      /* We don't take care of direct jumps when address mapping changes in
> @@ -342,13 +344,18 @@ static inline TranslationBlock *tb_find_fast(CPUState 
> *cpu,
>  #endif
>      /* See if we can patch the calling TB. */
>      if (last_tb && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
> -        tb_lock();
> +        if (!have_tb_lock) {
> +            tb_lock();
> +            have_tb_lock = true;
> +        }
>          /* Check if translation buffer has been flushed */
>          if (cpu->tb_flushed) {
>              cpu->tb_flushed = false;
>          } else if (!tb_is_invalid(tb)) {
>              tb_add_jump(last_tb, tb_exit, tb);
>          }
> +    }
> +    if (have_tb_lock) {
>          tb_unlock();
>      }
>      return tb;


--
Alex Bennée