On Fri, Mar 18, 2016 at 16:18:42 +0000, Alex Bennée wrote:
> From: KONRAD Frederic <[email protected]>
>
> Signed-off-by: KONRAD Frederic <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
> [AJB: minor checkpatch fixes]
> Signed-off-by: Alex Bennée <[email protected]>
>
> ---
> 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
