Re: [Qemu-devel] [PATCH 11/22] translate-all: use a binary search tree to track TBs in TBContext

[Richard Henderson]

On 07/08/2017 09:50 PM, Emilio G. Cota wrote:
> In order to use glib's binary search tree we embed a helper struct
> in TranslationBlock to allow us to compare tb's based on their
> tc_ptr as well as their tc_size fields.

Using an anon struct really doesn't help.  You're effectively using two 
different structs when you pass them in.  It "works" simply because in both 
cases we happen to know that a given pointer is followed by a uint32_t.

But with that in mind, and knowing that qemu builds with -fno-strict-aliasing, 
you could just as well *not* use the anon struct and address the pointer and 
the uint32_t within the un-partitioned TranslationBlock.  With appropriate 
comments in both places, of course.

> +    /*
> +     * When both sizes are set, we know this isn't a lookup and therefore
> +     * the two buffers are non-overlapping: a pointer comparison will do.
> +     * This is the most likely case: every TB must be inserted; lookups
> +     * are a lot less frequent.
> +     */
> +    if (likely(a->size && b->size)) {
> +        return a->ptr - b->ptr;
> +    }

sizeof(gint) < sizeof(ptrdiff_t) on 64-bit hosts.  You wouldn't have seen a 
problem on x86_64 because of MAX_CODE_GEN_BUFFER_SIZE.  I think it would be 
better to reduce this to -1/0/1.

>  static void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count)
>  {
> +    int nb_tbs __attribute__((unused));
> +
>      tb_lock();
>  
>      /* If it is already been done on request of another CPU,
> @@ -894,11 +913,12 @@ static void do_tb_flush(CPUState *cpu, run_on_cpu_data 
> tb_flush_count)
>      }
>  
>  #if defined(DEBUG_TB_FLUSH)
> +    nb_tbs = g_tree_nnodes(tcg_ctx.tb_ctx.tb_tree);
>      printf("qemu: flush code_size=%ld nb_tbs=%d avg_tb_size=%ld\n",
>             (unsigned long)(tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer),
> -           tcg_ctx.tb_ctx.nb_tbs, tcg_ctx.tb_ctx.nb_tbs > 0 ?
> +           nb_tbs, nb_tbs > 0 ?
>             ((unsigned long)(tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer)) /
> -           tcg_ctx.tb_ctx.nb_tbs : 0);
> +           nb_tbs : 0);
>  #endif

Variable declaration within braces within the ifdef.  Better as size_t or 
unsigned long.  Using int to count thing on 64-bit hosts always seems like a bug.


r~