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: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 */



Kind 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