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
