Re: [Qemu-devel] tcg: reworking tb_invalidated_flag

[Paolo Bonzini <pbonzini@redhat.com>]

On 30/03/2016 19:08, Sergey Fedorov wrote:
> cpu_exec() case is a bit more subtle. Regarding tb_phys_invalidate(), it
> shouldn't be harmful if an invalidated TB get patched because it is not
> going to be executed anymore. It may only be a concern of efficiency.
> Though it doesn't seem to happen frequently.
> 
> As of catching tb_flush() in cpu_exec() there have been three approaches
> proposed.
> 
> The first approach is to get rid of 'tb_invalidated_flag' and use
> 'tb_flush_count'. Capture 'tb_flush_count' inside 'tb_lock' critical
> section of cpu_exec() and compare it on each execution loop iteration
> before trying to do tb_add_jump(). This would be simple and clear but it
> would cost an extra load from a shared variable 'tb_flush_count' each
> time we go over the execution loop.
> 
> The second approach is to make 'tb_invalidated_flag' per-CPU. This
> would be conceptually similar to what we have, but would give us thread
> safety. With this approach, we need to be careful to correctly clear and
> set the flag.

You can just ensure that setting and clearing it is done under tb_lock.

> The third approach is to mark each individual TB as valid/invalid. This
> is what Emilio has in his MTTCG series [2]. Following this approach, we
> could have very clean code with no extra overhead on the hot path.
> However, it would require to mark all TBs as invalid on tb_flush().
> Given that tb_flush() is rare, it shouldn't be a significant overhead.

I agree; the problem with this solution is that it adds complexity to
tb_flush.  Because TranslationBlocks live in tcg_ctx.tb_ctx.tbs you need
special code to exit all CPUs at tb_flush time, otherwise you risk that
a tb_alloc reuses a TranslationBlock while it is in use by a VCPU.  This
would be complicated, hard to test, and only needed for a very rare
occurrence(*).  Because tb_flush() is rare I believe we should keep it
as simple as possible.

	(*) Both Emilio and Fred have a similar "exit all CPUs"
	    primitive.  Fred used it for tb_flush() and IIRC also
	    for some TLB flush primitives.  However, Alvise has been
	    reworking the TLB flush code to use a "CPU halted" state
	    that's less prone to deadlocks.

> Also, there could be several options how to mark TB valid/invalid:
> a dedicated flag could be introduced or some invalid value of
> pc/cs_base/flags could be used.

This is probably necessary nevertheless in order to make
tb_find_physical run outside tb_lock.

Paolo