diff mbox

[3/5] tcg: always keep jump target and tb->jmp_next consistent

Message ID 1458222382-6498-4-git-send-email-sergey.fedorov@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

sergey.fedorov@linaro.org March 17, 2016, 1:46 p.m. UTC 
From: Paolo Bonzini <pbonzini@redhat.com>

Simple code simplification.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 translate-all.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Comments

Richard Henderson March 17, 2016, 5:57 p.m. UTC | #1 
On 03/17/2016 06:46 AM, sergey.fedorov@linaro.org wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Simple code simplification.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
>  translate-all.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/translate-all.c b/translate-all.c
> index f17ace1ae899..a1ac9841de48 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -927,6 +927,14 @@ static inline void tb_page_remove(TranslationBlock **ptb, TranslationBlock *tb)
>      }
>  }
>  
> +/* reset the jump entry 'n' of a TB so that it is not chained to
> +   another TB */
> +static inline void tb_reset_jump(TranslationBlock *tb, int n)
> +{
> +    tb_set_jmp_target(tb, n, (uintptr_t)(tb->tc_ptr + tb->tb_next_offset[n]));
> +    tb->jmp_next[n] = NULL;
> +}
> +
>  static inline void tb_jmp_remove(TranslationBlock *tb, int n)
>  {
>      TranslationBlock *tb1, **ptb;
> @@ -951,18 +959,10 @@ static inline void tb_jmp_remove(TranslationBlock *tb, int n)
>          }
>          /* now we can suppress tb(n) from the list */
>          *ptb = tb->jmp_next[n];
> -
> -        tb->jmp_next[n] = NULL;
> +        tb_reset_jump(tb, n);

What's the motivation here?  This implies an extra cache flush.
Where were we resetting the jump previously?  Or is this a bug
in that we *weren't* resetting the jump previously?


r~
Paolo Bonzini March 17, 2016, 7:31 p.m. UTC | #2 
On 17/03/2016 18:57, Richard Henderson wrote:
> > @@ -951,18 +959,10 @@ static inline void tb_jmp_remove(TranslationBlock *tb, int n)
> >          }
> >          /* now we can suppress tb(n) from the list */
> >          *ptb = tb->jmp_next[n];
> > -
> > -        tb->jmp_next[n] = NULL;
> > +        tb_reset_jump(tb, n);
>
> What's the motivation here?  This implies an extra cache flush.
> Where were we resetting the jump previously?  Or is this a bug
> in that we *weren't* resetting the jump previously?

Indeed I think this patch can be removed if it has a performance effect
on machines that require icache invalidation.  If it doesn't, it would
be just a small code simplification.

Paolo
Sergey Fedorov March 17, 2016, 8:45 p.m. UTC | #3 
On 17/03/16 22:31, Paolo Bonzini wrote:
>
> On 17/03/2016 18:57, Richard Henderson wrote:
>>> @@ -951,18 +959,10 @@ static inline void tb_jmp_remove(TranslationBlock *tb, int n)
>>>           }
>>>           /* now we can suppress tb(n) from the list */
>>>           *ptb = tb->jmp_next[n];
>>> -
>>> -        tb->jmp_next[n] = NULL;
>>> +        tb_reset_jump(tb, n);
>> What's the motivation here?  This implies an extra cache flush.
>> Where were we resetting the jump previously?  Or is this a bug
>> in that we *weren't* resetting the jump previously?
> Indeed I think this patch can be removed if it has a performance effect
> on machines that require icache invalidation.  If it doesn't, it would
> be just a small code simplification.

In fact, tb_jmp_remove() is only supposed to remove the TB from a list 
of all TB's jumping to the same TB which is n-th jump destination of the 
given TB. This function is only called in tb_phys_invalidate() for the 
TB being invalidated. Thus we don't have to patch that TB anymore. We 
don't even have to do "tb->jmp_next[n] = NULL" here.

Probably it's time to audit the code that handles direct jumping and 
clean-up/document/rename things to make it more easy to understand? :)

Kind regards,
Sergey
Richard Henderson March 17, 2016, 8:46 p.m. UTC | #4 
On 03/17/2016 01:45 PM, Sergey Fedorov wrote:
> Probably it's time to audit the code that handles direct jumping and
> clean-up/document/rename things to make it more easy to understand? :)

Seconded!


r~
Sergey Fedorov March 18, 2016, 10:29 a.m. UTC | #5 
On 17/03/16 23:46, Richard Henderson wrote:
> On 03/17/2016 01:45 PM, Sergey Fedorov wrote:
>> Probably it's time to audit the code that handles direct jumping and
>> clean-up/document/rename things to make it more easy to understand? :)
> Seconded!

I'll go for it, then :)

Kind regards,
Sergey
Sergey Fedorov March 18, 2016, 10:32 a.m. UTC | #6 
On 17/03/16 23:45, Sergey Fedorov wrote:
> On 17/03/16 22:31, Paolo Bonzini wrote:
>>
>> On 17/03/2016 18:57, Richard Henderson wrote:
>>>> @@ -951,18 +959,10 @@ static inline void 
>>>> tb_jmp_remove(TranslationBlock *tb, int n)
>>>>           }
>>>>           /* now we can suppress tb(n) from the list */
>>>>           *ptb = tb->jmp_next[n];
>>>> -
>>>> -        tb->jmp_next[n] = NULL;
>>>> +        tb_reset_jump(tb, n);
>>> What's the motivation here?  This implies an extra cache flush.
>>> Where were we resetting the jump previously?  Or is this a bug
>>> in that we *weren't* resetting the jump previously?
>> Indeed I think this patch can be removed if it has a performance effect
>> on machines that require icache invalidation.  If it doesn't, it would
>> be just a small code simplification.
>
> In fact, tb_jmp_remove() is only supposed to remove the TB from a list 
> of all TB's jumping to the same TB which is n-th jump destination of 
> the given TB. This function is only called in tb_phys_invalidate() for 
> the TB being invalidated. Thus we don't have to patch that TB anymore. 
> We don't even have to do "tb->jmp_next[n] = NULL" here. 

I'll drop the patch from the series in v2 then.

Kind regards,
Sergey
diff mbox

Patch

diff --git a/translate-all.c b/translate-all.c
index f17ace1ae899..a1ac9841de48 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -927,6 +927,14 @@  static inline void tb_page_remove(TranslationBlock **ptb, TranslationBlock *tb)
     }
 }
 
+/* reset the jump entry 'n' of a TB so that it is not chained to
+   another TB */
+static inline void tb_reset_jump(TranslationBlock *tb, int n)
+{
+    tb_set_jmp_target(tb, n, (uintptr_t)(tb->tc_ptr + tb->tb_next_offset[n]));
+    tb->jmp_next[n] = NULL;
+}
+
 static inline void tb_jmp_remove(TranslationBlock *tb, int n)
 {
     TranslationBlock *tb1, **ptb;
@@ -951,18 +959,10 @@  static inline void tb_jmp_remove(TranslationBlock *tb, int n)
         }
         /* now we can suppress tb(n) from the list */
         *ptb = tb->jmp_next[n];
-
-        tb->jmp_next[n] = NULL;
+        tb_reset_jump(tb, n);
     }
 }
 
-/* reset the jump entry 'n' of a TB so that it is not chained to
-   another TB */
-static inline void tb_reset_jump(TranslationBlock *tb, int n)
-{
-    tb_set_jmp_target(tb, n, (uintptr_t)(tb->tc_ptr + tb->tb_next_offset[n]));
-}
-
 /* invalidate one TB */
 void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
 {
@@ -1013,7 +1013,6 @@  void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
         tb1 = (TranslationBlock *)((uintptr_t)tb1 & ~3);
         tb2 = tb1->jmp_next[n1];
         tb_reset_jump(tb1, n1);
-        tb1->jmp_next[n1] = NULL;
         tb1 = tb2;
     }
     tb->jmp_first = (TranslationBlock *)((uintptr_t)tb | 2); /* fail safe */