Re: [Qemu-devel] [RFC v1 01/11] tcg: move tb_find_fast outside the tb_lock critical section

[Emilio G. Cota]

On Fri, Mar 18, 2016 at 16:18:42 +0000, Alex Bennée wrote:
> From: KONRAD Frederic <address@hidden>
> 
> Signed-off-by: KONRAD Frederic <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> [AJB: minor checkpatch fixes]
> Signed-off-by: Alex Bennée <address@hidden>
> 
> ---
> v1(ajb)
>   - checkpatch fixes
> ---
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 07545aa..52f25de 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -225,8 +225,9 @@ static TranslationBlock *tb_find_physical(CPUState *cpu,
>      phys_page1 = phys_pc & TARGET_PAGE_MASK;
>      h = tb_phys_hash_func(phys_pc);
>      for (ptb1 = &tcg_ctx.tb_ctx.tb_phys_hash[h];
> -         (tb = *ptb1) != NULL;
> +         (tb = atomic_read(ptb1)) != NULL;
>           ptb1 = &tb->phys_hash_next) {
> +        smp_read_barrier_depends();
>          if (tb->pc != pc ||
>              tb->page_addr[0] != phys_page1 ||
>              tb->cs_base != cs_base ||
> @@ -254,7 +255,18 @@ static TranslationBlock *tb_find_physical(CPUState *cpu,
[ Adding this missing line to the diff for clarity ]
           /* Move the TB to the head of the list */
>          *ptb1 = tb->phys_hash_next;
>          tb->phys_hash_next = tcg_ctx.tb_ctx.tb_phys_hash[h];
>          tcg_ctx.tb_ctx.tb_phys_hash[h] = tb;

This function, as is, doesn't really just "find"; two concurrent "finders"
could race here by *writing* to the head of the list at the same time.

The fix is to get rid of this write entirely; moving the just-found TB to
the head of the list is not really that necessary thanks to the CPU's
tb_jmp_cache table. This fix would make the function read-only, which
is what the function's name implies.

Further, I'd like to see tb_phys_hash to use the RCU queue primitives; it
makes everything easier to understand (and we avoid sprinkling the code
base with smp_barrier_depends).

I have these two changes queued up as part of my upcoming series, which I'm
basing on your patchset.

Thanks for putting these changes together!

                Emilio