From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37282) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asXYq-0006E6-5C for qemu-devel@nongnu.org; Tue, 19 Apr 2016 11:27:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1asXYl-0006rD-TG for qemu-devel@nongnu.org; Tue, 19 Apr 2016 11:27:32 -0400 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]:35310) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asXYl-0006qt-JA for qemu-devel@nongnu.org; Tue, 19 Apr 2016 11:27:27 -0400 Received: by mail-wm0-x231.google.com with SMTP id e201so18326511wme.0 for ; Tue, 19 Apr 2016 08:27:27 -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> <57162973.1050308@gmail.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <57162973.1050308@gmail.com> Date: Tue, 19 Apr 2016 16:27:24 +0100 Message-ID: <8737qhwrdv.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: Sergey Fedorov , qemu-devel@nongnu.org, Paolo Bonzini , Peter Crosthwaite , Richard Henderson Sergey Fedorov writes: > 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()? 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 >> >>> + 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