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

[Alex Bennée]

Sergey Fedorov <address@hidden> writes:

> 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.

Sure, lets see what it looks like ;-)

>
> 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


--
Alex Bennée