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

[Paolo Bonzini]

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

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

[Alex Bennée]

Paolo Bonzini  writes:

> From: Sergey Fedorov 
>
> This is a small clean up. tb_find_fast() is a final consumer of this
> variable so no need to pass it by reference. 'last_tb' is always updated
> by subsequent cpu_loop_exec_tb() in cpu_exec().
>

> @@ -621,7 +620,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_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.

>  /* Try to align the host and virtual clocks
> if the guest is in advance */



--
Alex Bennée