| Message ID | 1458815961-31979-6-git-send-email-sergey.fedorov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 24/03/2016 11:39, sergey.fedorov@linaro.org wrote: > + /* FIXME: This test provides only some probablistic "thread safety" for > + * user-mode emulation; appropriate synchronization/locking scheme should > + * be implemented. > + */ There is appropriate locking. This code: if (next_tb != 0 && tb->page_addr[1] == -1 && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) { tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK), next_tb & TB_EXIT_MASK, tb); } in cpu-exec.c runs under tb_lock. However, two threads can decide to call tb_add_jump at the same time outside the lock, so we have to check inside the lock whether someone has already done the work. What the comment means is that, in single-threaded scenarios (current TCG and single-threaded user emulation), tb->jmp_list_next[n] is only set once. Paolo
s/safaty/safety/ ? On Thu, Mar 24, 2016 at 11:39 AM, <sergey.fedorov@linaro.org> wrote: > From: Sergey Fedorov <serge.fdrv@gmail.com> > > The check does not give an absolute guarantee of thread safety because > there still may be a race condition between two threads which both have > just read zero from jmp_list_next[n] and proceed with list modification. > Clarify this in the comment to attract here some attention. > > Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com> > Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org> > --- > include/exec/exec-all.h | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index cd96219a89e7..4f36d109ac7f 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -390,7 +390,10 @@ 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 */ > + /* FIXME: This test provides only some probablistic "thread safety" for > + * user-mode emulation; appropriate synchronization/locking scheme should > + * be implemented. > + */ > if (!tb->jmp_list_next[n]) { > /* patch the native jump address */ > tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr); > -- > 2.7.3 > >
On 24/03/16 15:23, Artyom Tarasenko wrote:
> s/safaty/safety/ ?
Oops, silly typo :)
Thanks,
Sergey
On 24/03/16 14:31, Paolo Bonzini wrote: > On 24/03/2016 11:39, sergey.fedorov@linaro.org wrote: >> + /* FIXME: This test provides only some probablistic "thread safety" for >> + * user-mode emulation; appropriate synchronization/locking scheme should >> + * be implemented. >> + */ > There is appropriate locking. This code: > > if (next_tb != 0 && tb->page_addr[1] == -1 > && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) { > tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK), > next_tb & TB_EXIT_MASK, tb); > } > > in cpu-exec.c runs under tb_lock. However, two threads can decide to > call tb_add_jump at the same time outside the lock, so we have to check > inside the lock whether someone has already done the work. > > What the comment means is that, in single-threaded scenarios (current > TCG and single-threaded user emulation), tb->jmp_list_next[n] is only > set once. Right, thanks for clarifying this. So I'm going to put mention this in the comment, then. Kind regards, Sergey
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index cd96219a89e7..4f36d109ac7f 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -390,7 +390,10 @@ 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 */ + /* FIXME: This test provides only some probablistic "thread safety" for + * user-mode emulation; appropriate synchronization/locking scheme should + * be implemented. + */ if (!tb->jmp_list_next[n]) { /* patch the native jump address */ tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);