From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46269) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asV6S-0005hF-1v for qemu-devel@nongnu.org; Tue, 19 Apr 2016 08:50:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1asV6M-0000Ak-BP for qemu-devel@nongnu.org; Tue, 19 Apr 2016 08:50:04 -0400 Received: from mail-lf0-x232.google.com ([2a00:1450:4010:c07::232]:34490) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asV6L-0000AZ-Uz for qemu-devel@nongnu.org; Tue, 19 Apr 2016 08:49:58 -0400 Received: by mail-lf0-x232.google.com with SMTP id j11so17490950lfb.1 for ; Tue, 19 Apr 2016 05:49:57 -0700 (PDT) References: <1460324732-30330-1-git-send-email-sergey.fedorov@linaro.org> <1460324732-30330-6-git-send-email-sergey.fedorov@linaro.org> <87a8kpx3pq.fsf@linaro.org> From: Sergey Fedorov Message-ID: <57162973.1050308@gmail.com> Date: Tue, 19 Apr 2016 15:49:55 +0300 MIME-Version: 1.0 In-Reply-To: <87a8kpx3pq.fsf@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 05/10] tcg: Clarify thread safety check in tb_add_jump() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Alex_Benn=c3=a9e?= , Sergey Fedorov Cc: qemu-devel@nongnu.org, Paolo Bonzini , Peter Crosthwaite , Richard Henderson On 19/04/16 14:01, Alex Bennée wrote: > Sergey Fedorov writes: > >> From: Sergey Fedorov >> >> 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 >> Signed-off-by: Sergey Fedorov >> --- >> >> 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 > >> + tb_next->jmp_list_first = (uintptr_t)tb | n; >> } >> >> /* GETRA is the true target of the return instruction that we'll execute, >