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

[Sergey Fedorov]

On 14/07/16 16:01, 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>
> 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.

I just split them because I was not sure if both are acceptable.

Thanks,
Sergey

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