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

["Alex Bennée" <alex.bennee@linaro.org>]

Emilio G. Cota <cota@braap.org> writes:

> On Fri, Mar 18, 2016 at 16:18:42 +0000, Alex Bennée wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> [AJB: minor checkpatch fixes]
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> 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 ]

I have to admit that clarity is one thing the code in this area could do
with. I find it hard to follow on the best of days.

>            /* 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.

Cool, I look forward to it.

>
> Thanks for putting these changes together!

This was exactly my aim, getting the common base stuff reviewed so the
competing plurality of approaches can build on it as we shake out the
design ;-)

>
> 		Emilio


--
Alex Bennée