Re: [Qemu-devel] [PATCH 02/10] tcg: Pass last_tb by value to tb_find_fast()

[Paolo Bonzini <pbonzini@redhat.com>]

On 08/09/2016 14:44, Alex Bennée wrote:
>> >              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_fast(cpu, last_tb, tb_exit);
> Maybe a comment here for those that missed the subtly in the commit
> message?
> 
>                    /* cpu_loop_exec_tb updates a to a new last_tb */
> 
>> >                  cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
> You could even make it explicit and change cpu_loop_exec_tb to return
> last_tb instead of passing by reference. Then it would be even clearer
> when reading the code.
> 

I gave it a quick shot and it's not that simple... One simpler possibility
is to take this patch one step further and merge "tb" and "last_tb", but
I've not tested it yet:

diff --git a/cpu-exec.c b/cpu-exec.c
index cf511f1..80e6ff5 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -515,11 +515,11 @@ static inline void cpu_handle_interrupt(CPUState *cpu,
     }
 }
 
-static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
-                                    TranslationBlock **last_tb, int *tb_exit,
-                                    SyncClocks *sc)
+static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock **last_tb,
+                                    int *tb_exit, SyncClocks *sc)
 {
     uintptr_t ret;
+    TranslationBlock *tb = *last_tb;
 
     if (unlikely(cpu->exit_request)) {
         return;
@@ -609,7 +609,7 @@ int cpu_exec(CPUState *cpu)
     for(;;) {
         /* prepare setjmp context for exception handling */
         if (sigsetjmp(cpu->jmp_env, 0) == 0) {
-            TranslationBlock *tb, *last_tb = NULL;
+            TranslationBlock *tb = NULL;
             int tb_exit = 0;
 
             /* if an exception is pending, we execute it here */
@@ -619,9 +619,9 @@ 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);
-                cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
+                cpu_handle_interrupt(cpu, &tb);
+                tb = tb_find_fast(cpu, tb, tb_exit);
+                cpu_loop_exec_tb(cpu, &tb, &tb_exit, &sc);
                 /* Try to align the host and virtual clocks
                    if the guest is in advance */
                 align_clocks(&sc, cpu);

It seems better to me to do it as a follow-up step.

Paolo