From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asTP5-0002t7-BM for qemu-devel@nongnu.org; Tue, 19 Apr 2016 07:01:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1asTP2-0003tw-0l for qemu-devel@nongnu.org; Tue, 19 Apr 2016 07:01:11 -0400 Received: from mail-wm0-x234.google.com ([2a00:1450:400c:c09::234]:37075) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asTP1-0003te-Hw for qemu-devel@nongnu.org; Tue, 19 Apr 2016 07:01:07 -0400 Received: by mail-wm0-x234.google.com with SMTP id n3so23067862wmn.0 for ; Tue, 19 Apr 2016 04:01:07 -0700 (PDT) References: <1460324732-30330-1-git-send-email-sergey.fedorov@linaro.org> <1460324732-30330-6-git-send-email-sergey.fedorov@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <1460324732-30330-6-git-send-email-sergey.fedorov@linaro.org> Date: Tue, 19 Apr 2016 12:01:05 +0100 Message-ID: <87a8kpx3pq.fsf@linaro.org> MIME-Version: 1.0 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: Sergey Fedorov Cc: qemu-devel@nongnu.org, Sergey Fedorov , Paolo Bonzini , Peter Crosthwaite , Richard Henderson 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. 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, -- Alex Bennée