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

[Sergey Fedorov]

On 07/07/16 22:46, Sergey Fedorov wrote:
> On 07/07/16 22:36, Alex Bennée wrote:
>> Sergey Fedorov <address@hidden> writes:
>>
>>> From: Sergey Fedorov <address@hidden>
>>>
>>> Signed-off-by: Sergey Fedorov <address@hidden>
>>> Signed-off-by: Sergey Fedorov <address@hidden>
>>> ---
>>>  cpu-exec.c | 15 +++++++++------
>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/cpu-exec.c b/cpu-exec.c
>>> index dd0bd5007701..54c935039592 100644
>>> --- a/cpu-exec.c
>>> +++ b/cpu-exec.c
>>> @@ -295,7 +295,8 @@ static TranslationBlock *tb_find_slow(CPUState *cpu,
>>>
>>>          /* mmap_lock is needed by tb_gen_code, and mmap_lock must be
>>>           * taken outside tb_lock. As system emulation is currently
>>> -         * single threaded the locks are NOPs.
>>> +         * single threaded the locks are NOPs. Both locks are to be
>>> +         * released at the end of tb_find_fast().
>>>           */
>>>          mmap_lock();
>>>          tb_lock();
>>> @@ -308,9 +309,6 @@ static TranslationBlock *tb_find_slow(CPUState *cpu,
>>>              /* if no translated code available, then translate it now */
>>>              tb = tb_gen_code(cpu, pc, cs_base, flags, 0);
>>>          }
>>> -
>>> -        tb_unlock();
>>> -        mmap_unlock();
>> Hmm pushing these outside of tb_find_slow() makes me uncomfortable. I
>> guess tb_find_fast/slow are intimately tied together but the idea of
>> taking locks which are the responsibility of the calling function to
>> clear seems ugly to me.
> Okay, what if we also:
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 54c935039592..ff8f92bc1dc1 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -278,45 +278,7 @@ static TranslationBlock *tb_find_physical(CPUState
> *cpu,
>      return qht_lookup(&tcg_ctx.tb_ctx.htable, tb_cmp, &desc, h);
>  }
>  
> -static TranslationBlock *tb_find_slow(CPUState *cpu,
> -                                      target_ulong pc,
> -                                      target_ulong cs_base,
> -                                      uint32_t flags)
> -{
> -    TranslationBlock *tb;
> -
> -    /* Ensure that we won't find a TB in the shared hash table
> -     * if it is being invalidated by some other thread.
> -     * Otherwise we'd put it back to CPU's local cache.
> -     * Pairs with smp_wmb() in tb_phys_invalidate(). */
> -    smp_rmb();
> -    tb = tb_find_physical(cpu, pc, cs_base, flags);
> -    if (!tb) {
> -
> -        /* mmap_lock is needed by tb_gen_code, and mmap_lock must be
> -         * taken outside tb_lock. As system emulation is currently
> -         * single threaded the locks are NOPs. Both locks are to be
> -         * released at the end of tb_find_fast().
> -         */
> -        mmap_lock();
> -        tb_lock();
> -
> -        /* There's a chance that our desired tb has been translated while
> -         * taking the locks so we check again inside the lock.
> -         */
> -        tb = tb_find_physical(cpu, pc, cs_base, flags);
> -        if (!tb) {
> -            /* if no translated code available, then translate it now */
> -            tb = tb_gen_code(cpu, pc, cs_base, flags, 0);
> -        }
> -    }
> -
> -    /* We add the TB in the virtual pc hash table for the fast lookup */
> -    atomic_set(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)], tb);
> -    return tb;
> -}
> -
> -static inline TranslationBlock *tb_find_fast(CPUState *cpu,
> +static inline TranslationBlock *tb_find(CPUState *cpu,
>                                               TranslationBlock **last_tb,
>                                               int tb_exit)
>  {
> @@ -332,7 +294,34 @@ 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);
> +        /* Ensure that we won't find a TB in the shared hash table
> +         * if it is being invalidated by some other thread.
> +         * Otherwise we'd put it back to CPU's local cache.
> +         * Pairs with smp_wmb() in tb_phys_invalidate(). */
> +        smp_rmb();
> +        tb = tb_find_physical(cpu, pc, cs_base, flags);
> +        if (!tb) {
> +
> +            /* mmap_lock is needed by tb_gen_code, and mmap_lock must be
> +             * taken outside tb_lock. As system emulation is currently
> +             * single threaded the locks are NOPs. Both locks are to be
> +             * released at the end of the function.
> +             */
> +            mmap_lock();
> +            tb_lock();
> +
> +            /* There's a chance that our desired tb has been translated
> while
> +             * taking the locks so we check again inside the lock.
> +             */
> +            tb = tb_find_physical(cpu, pc, cs_base, flags);
> +            if (!tb) {
> +                /* if no translated code available, then translate it
> now */
> +                tb = tb_gen_code(cpu, pc, cs_base, flags, 0);
> +            }
> +        }
> +
> +        /* We add the TB in the virtual pc hash table for the fast
> lookup */
> +        atomic_set(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)], tb);
>      }
>      if (cpu->tb_flushed) {
>          /* Ensure that no TB jump will be modified as the
> @@ -440,7 +429,7 @@ static inline bool cpu_handle_exception(CPUState
> *cpu, int *ret)
>                 && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
>          /* try to cause an exception pending in the log */
>          TranslationBlock *last_tb = NULL; /* Avoid chaining TBs */
> -        cpu_exec_nocache(cpu, 1, tb_find_fast(cpu, &last_tb, 0), true);
> +        cpu_exec_nocache(cpu, 1, tb_find(cpu, &last_tb, 0), true);
>          *ret = -1;
>          return true;
>  #endif
> @@ -624,7 +613,7 @@ int cpu_exec(CPUState *cpu)
>              cpu->tb_flushed = false; /* reset before first TB lookup */
>              for(;;) {
>                  cpu_handle_interrupt(cpu, &last_tb);
> -                tb = tb_find_fast(cpu, &last_tb, tb_exit);
> +                tb = tb_find(cpu, &last_tb, tb_exit);
>                  cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
>                  /* Try to align the host and virtual clocks
>                     if the guest is in advance */
>
>

Using goto statements, the lines from tb_find_slow() which take the
locks can even be on the same indentation level as the lines resetting
them. I could prepare a patch series based on your first 3 patches and
this idea.

Regards,
Sergey

>
>
>>>      }
>>>
>>>      /* We add the TB in the virtual pc hash table for the fast lookup */
>>> @@ -354,10 +352,15 @@ 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 (!tb_lock_locked()) {
>>> +            tb_lock();
>>> +        }
>>>          tb_add_jump(*last_tb, tb_exit, tb);
>>> -        tb_unlock();
>>>      }
>>> +
>>> +    tb_lock_reset();
>>> +    mmap_lock_reset();
>>> +
>>>      return tb;
>>>  }
>> --
>> Alex Bennée