Re: [Qemu-devel] [PATCH 4/5] tcg: reorder removal from lists in tb_phys_invalidate

[Sergey Fedorov <serge.fdrv@gmail.com>]

On 14/04/16 18:13, Paolo Bonzini wrote:
> This is very similar to the current code.  From 10,000 feet, because
> tb_find_fast calls tb_find_slow, this could indeed work, but I'm a bit
> concerned about how to order the removal of the jump lists.  The usage
> of "tcg_ctx.tb_ctx.tb_invalidated_flag = 1" in the existing code was
> what worries me.  Indeed the motivation of this patch was removing that
> single line of code to prepare for the move of tb_invalidated_flag to
> CPUState.

I'm just preparing a patch with a long commit message which describes
and removes this setting of tb_invalidated_flag :) I think we can do
this safely and even benefit in performance.

>
> Also, this loop will not be thread-safe anymore as soon as Fred's
> "tb_jmp_cache lookup outside tb_lock" goes in:
>
>     CPU_FOREACH(cpu) {
>         if (cpu->tb_jmp_cache[h] == tb) {
>             cpu->tb_jmp_cache[h] = NULL;
>         }
>     }
>
> It should use atomic_cmpxchg (slow!) or to unconditionally NULL out
> cpu->tb_jmp_cache (a bit hacky).  Preparing for that change is an added
> bonus of the tb-hacking approach.

IIUC we always modify tb_jmp_cache/tb_phys_hash under tb_lock. We're
just gonna do tb_jmp_cache lookup outside of tb_lock. I think write
memory barrier in tb_phys_invalidate() paired with read memory memory
barrier in tb_find_fast()/tb_find_slow() would be enough to make it
safe. Also we need to use atomic read/set to tb_jmp_cache but cmpxchg is
not necessary. So I'm going to give it a thought then.

Kind regards,
Sergey