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

[Alex Bennée]

Sergey Fedorov <address@hidden> writes:

> On 19/04/16 14:01, Alex Bennée wrote:
>> Sergey Fedorov <address@hidden> writes:
>>
>>> From: Sergey Fedorov <address@hidden>
>>>
>>> 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 <address@hidden>
>>> Signed-off-by: Sergey Fedorov <address@hidden>
>>> ---
>>>
>>> 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()?

I think in tb_link_page it is reasonable although fairly unlikely that
tb_alloc is going to start spitting out unaligned TranslationBlocks.

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


--
Alex Bennée