| Message ID | 1460324732-30330-6-git-send-email-sergey.fedorov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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, -- Alex Bennée
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, >
Sergey Fedorov <serge.fdrv@gmail.com> writes: > 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()? 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 <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, >> -- Alex Bennée
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); + tb_next->jmp_list_first = (uintptr_t)tb | n; } /* GETRA is the true target of the return instruction that we'll execute,