Re: [Qemu-devel] [PATCH v3 05/10] tcg: Clarify thread safety check in tb_add_jump()

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

On 19/04/16 14:01, Alex Bennée wrote:
> Sergey Fedorov <sergey.fedorov@linaro.org> writes:
>
>> From: Sergey Fedorov <serge.fdrv@gmail.com>
>>
>> The check is to make sure that another thread hasn't already done the
>> same while we were outside of tb_lock. Mention this in a comment.
>>
>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
>> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
>> ---
>>
>> Changes in v2:
>>  * Typo fixed in the commit title
>>  * Complete rewrite of the commit body and the patch based on Paolo's comments
>>
>>  include/exec/exec-all.h | 31 +++++++++++++++++--------------
>>  1 file changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> index b055716ed690..8e81ef5fb2c2 100644
>> --- a/include/exec/exec-all.h
>> +++ b/include/exec/exec-all.h
>> @@ -391,21 +391,24 @@ static inline void tb_set_jmp_target(TranslationBlock *tb,
>>  static inline void tb_add_jump(TranslationBlock *tb, int n,
>>                                 TranslationBlock *tb_next)
>>  {
>> -    /* NOTE: this test is only needed for thread safety */
>> -    if (!tb->jmp_list_next[n]) {
>> -        qemu_log_mask_and_addr(CPU_LOG_EXEC, tb->pc,
>> -                               "Linking TBs %p [" TARGET_FMT_lx
>> -                               "] index %d -> %p [" TARGET_FMT_lx "]\n",
>> -                               tb->tc_ptr, tb->pc, n,
>> -                               tb_next->tc_ptr, tb_next->pc);
>> -        /* patch the native jump address */
>> -        tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
>> -
>> -        /* add in TB jmp circular list */
>> -        tb->jmp_list_next[n] = tb_next->jmp_list_first;
>> -        assert(((uintptr_t)tb & 3) == 0);
>> -        tb_next->jmp_list_first = (uintptr_t)tb | n;
>> +    if (tb->jmp_list_next[n]) {
>> +        /* Another thread has already done this while we were
>> +         * outside of the lock; nothing to do in this case */
>> +        return;
>>      }
>> +    qemu_log_mask_and_addr(CPU_LOG_EXEC, tb->pc,
>> +                           "Linking TBs %p [" TARGET_FMT_lx
>> +                           "] index %d -> %p [" TARGET_FMT_lx "]\n",
>> +                           tb->tc_ptr, tb->pc, n,
>> +                           tb_next->tc_ptr, tb_next->pc);
>> +
>> +    /* patch the native jump address */
>> +    tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
>> +
>> +    /* add in TB jmp circular list */
>> +    tb->jmp_list_next[n] = tb_next->jmp_list_first;
>> +    assert(((uintptr_t)tb & 3) == 0);
> I think this assert can be dropped. The only call explicitly masks with
> TB_EXIT_MASK (which would be a better choice than the number 3 anyway)
> so something really strange would have had to happen in the intervening
> few lines.

What about the same assert in tb_gen_code()?

Kind regards,
Sergey

>
> Otherwise:
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
>> +    tb_next->jmp_list_first = (uintptr_t)tb | n;
>>  }
>>
>>  /* GETRA is the true target of the return instruction that we'll execute,
>