[RFC,1/3] tcg: Release tb_lock in the order acquired

Commit Message

Pranith Kumar Dec. 7, 2016, 3:38 p.m. UTC

Hi Alex,

Alex Bennée writes:

>
> Do you have any numbers for this? The main reason being we are trying to
> avoid bouncing the lock too much and while this is cleaner it could
> cause more contention.

I did not really consider performance while cleaning this up. However, I
looked closer and I think we can remove the tb lock acquisition while adding
the jump by using atomics. I've attached the patch below. This should remove
any concern for a negative performance impact.

I will include this patch if you think it's okay.

Thanks,

Comments

Paolo Bonzini Dec. 8, 2016, 5:52 p.m. UTC | #1

On 07/12/2016 16:38, Pranith Kumar wrote:
> 
> Hi Alex,
> 
> Alex Bennée writes:
> 
>>
>> Do you have any numbers for this? The main reason being we are trying to
>> avoid bouncing the lock too much and while this is cleaner it could
>> cause more contention.
> 
> I did not really consider performance while cleaning this up. However, I
> looked closer and I think we can remove the tb lock acquisition while adding
> the jump by using atomics. I've attached the patch below. This should remove
> any concern for a negative performance impact.
> 
> I will include this patch if you think it's okay.

I don't like this, it's too tricky.  You'd have to prove that it's
correct and that it keeps the list consistent.  There's nothing wrong in
locking like

	lock(L1)
	lock(L2)
	unlock(L1)
	unlock(L2)

so I'm not sure I see the point of this patch.

Paolo

> Thanks,
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 13cb15de0e..93debe64b6 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -365,9 +365,7 @@ static inline TranslationBlock *tb_find(CPUState *cpu,
>      /* See if we can patch the calling TB. */
>      if (last_tb && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
>          if (!tb->invalid) {
> -            tb_lock();
>              tb_add_jump(last_tb, tb_exit, tb);
> -            tb_unlock();
>          }
>      }
>      return tb;
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 84a3240df6..60597cb07e 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -336,7 +336,7 @@ static inline void tb_set_jmp_target(TranslationBlock *tb,
>  static inline void tb_add_jump(TranslationBlock *tb, int n,
>                                 TranslationBlock *tb_next)
>  {
> -    if (tb->jmp_list_next[n]) {
> +    if (atomic_cmpxchg(&tb->jmp_list_next[n], 0, tb_next->jmp_list_first)) {
>          /* Another thread has already done this while we were
>           * outside of the lock; nothing to do in this case */
>          return;
> @@ -351,7 +351,6 @@ static inline void tb_add_jump(TranslationBlock *tb, int n,
>      tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
>  
>      /* add in TB jmp circular list */
> -    tb->jmp_list_next[n] = tb_next->jmp_list_first;
>      tb_next->jmp_list_first = (uintptr_t)tb | n;
>  }
>  
>

Patch

diff --git a/cpu-exec.c b/cpu-exec.c
index 13cb15de0e..93debe64b6 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -365,9 +365,7 @@  static inline TranslationBlock *tb_find(CPUState *cpu,
     /* See if we can patch the calling TB. */
     if (last_tb && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
         if (!tb->invalid) {
-            tb_lock();
             tb_add_jump(last_tb, tb_exit, tb);
-            tb_unlock();
         }
     }
     return tb;
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 84a3240df6..60597cb07e 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -336,7 +336,7 @@  static inline void tb_set_jmp_target(TranslationBlock *tb,
 static inline void tb_add_jump(TranslationBlock *tb, int n,
                                TranslationBlock *tb_next)
 {
-    if (tb->jmp_list_next[n]) {
+    if (atomic_cmpxchg(&tb->jmp_list_next[n], 0, tb_next->jmp_list_first)) {
         /* Another thread has already done this while we were
          * outside of the lock; nothing to do in this case */
         return;
@@ -351,7 +351,6 @@  static inline void tb_add_jump(TranslationBlock *tb, int n,
     tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
 
     /* add in TB jmp circular list */
-    tb->jmp_list_next[n] = tb_next->jmp_list_first;
     tb_next->jmp_list_first = (uintptr_t)tb | n;
 }