| Message ID | 1459870344-16773-5-git-send-email-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 05/04/16 18:32, Alex Bennée wrote: > diff --git a/exec.c b/exec.c > index f46e596..17f390e 100644 > --- a/exec.c > +++ b/exec.c > @@ -826,6 +826,7 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags, > { > CPUBreakpoint *bp; > > + /* TODO: locking (RCU?) */ > bp = g_malloc(sizeof(*bp)); > > bp->pc = pc; This comment is a little inconsistent. We should make access to breakpoint and watchpoint lists to be thread-safe in all the functions using them. So if we note this, it should be noted in all such places. Also, it's probably not a good idea to put such comment just above g_malloc() invocation, it could be a bit confusing. A bit more details would also be nice. Kind regards, Sergey
Sergey Fedorov <serge.fdrv@gmail.com> writes: > On 05/04/16 18:32, Alex Bennée wrote: >> diff --git a/exec.c b/exec.c >> index f46e596..17f390e 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -826,6 +826,7 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags, >> { >> CPUBreakpoint *bp; >> >> + /* TODO: locking (RCU?) */ >> bp = g_malloc(sizeof(*bp)); >> >> bp->pc = pc; > > This comment is a little inconsistent. We should make access to > breakpoint and watchpoint lists to be thread-safe in all the functions > using them. So if we note this, it should be noted in all such places. > Also, it's probably not a good idea to put such comment just above > g_malloc() invocation, it could be a bit confusing. A bit more details > would also be nice. Good point. I could really do with some tests to exercise the debugging interface. I did some when I wrote the arm kvm GDB stuff (see 261f4d6d3e5445f887e070f047968e756c30cf06) but it is a) not plumbed in and b) not really a stress test which is what you want to be sure your handling is thread safe. > > Kind regards, > Sergey -- Alex Bennée
On 05/05/16 18:03, Alex Bennée wrote: > Sergey Fedorov <serge.fdrv@gmail.com> writes: > >> On 05/04/16 18:32, Alex Bennée wrote: >>> diff --git a/exec.c b/exec.c >>> index f46e596..17f390e 100644 >>> --- a/exec.c >>> +++ b/exec.c >>> @@ -826,6 +826,7 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags, >>> { >>> CPUBreakpoint *bp; >>> >>> + /* TODO: locking (RCU?) */ >>> bp = g_malloc(sizeof(*bp)); >>> >>> bp->pc = pc; >> This comment is a little inconsistent. We should make access to >> breakpoint and watchpoint lists to be thread-safe in all the functions >> using them. So if we note this, it should be noted in all such places. >> Also, it's probably not a good idea to put such comment just above >> g_malloc() invocation, it could be a bit confusing. A bit more details >> would also be nice. > Good point. > > I could really do with some tests to exercise the debugging interface. I > did some when I wrote the arm kvm GDB stuff (see > 261f4d6d3e5445f887e070f047968e756c30cf06) but it is a) not plumbed in > and b) not really a stress test which is what you want to be sure your > handling is thread safe. It seems we can only have a race between TCG helper inserting/removing break-/watchpoint and gdbstub. So maybe we could just use separate lists for CPU and GDB breakpoints? Kind regards, Sergey
On 05/05/16 18:25, Sergey Fedorov wrote: > On 05/05/16 18:03, Alex Bennée wrote: >> Sergey Fedorov <serge.fdrv@gmail.com> writes: >> >>> On 05/04/16 18:32, Alex Bennée wrote: >>>> diff --git a/exec.c b/exec.c >>>> index f46e596..17f390e 100644 >>>> --- a/exec.c >>>> +++ b/exec.c >>>> @@ -826,6 +826,7 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags, >>>> { >>>> CPUBreakpoint *bp; >>>> >>>> + /* TODO: locking (RCU?) */ >>>> bp = g_malloc(sizeof(*bp)); >>>> >>>> bp->pc = pc; >>> This comment is a little inconsistent. We should make access to >>> breakpoint and watchpoint lists to be thread-safe in all the functions >>> using them. So if we note this, it should be noted in all such places. >>> Also, it's probably not a good idea to put such comment just above >>> g_malloc() invocation, it could be a bit confusing. A bit more details >>> would also be nice. >> Good point. >> >> I could really do with some tests to exercise the debugging interface. I >> did some when I wrote the arm kvm GDB stuff (see >> 261f4d6d3e5445f887e070f047968e756c30cf06) but it is a) not plumbed in >> and b) not really a stress test which is what you want to be sure your >> handling is thread safe. > It seems we can only have a race between TCG helper inserting/removing > break-/watchpoint and gdbstub. So maybe we could just use separate lists > for CPU and GDB breakpoints? However, we would still need to synchronize reads in VCPU threads with insertions/removals in gdbstub... No much point in splitting lists, we could just use a spinlock to serialize insertions/removals and RCU to make reads safe. Kind regards, Sergey
On 05/04/16 18:32, Alex Bennée wrote: > From: Paolo Bonzini <pbonzini@redhat.com> > > softmmu requires more functions to be thread-safe, because translation > blocks can be invalidated from e.g. notdirty callbacks. Probably the > same holds for user-mode emulation, it's just that no one has ever > tried to produce a coherent locking there. > > This patch will guide the introduction of more tb_lock and tb_unlock > calls for system emulation. > > Note that after this patch some (most) of the mentioned functions are > still called outside tb_lock/tb_unlock. The next one will rectify this. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> (snip) > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 6931db9..13eeaae 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -306,7 +306,10 @@ struct CPUState { > > void *env_ptr; /* CPUArchState */ > struct TranslationBlock *current_tb; > + > + /* Writes protected by tb_lock, reads not thread-safe */ > struct TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE]; What does "reads not thread-safe" mean? Where does it get read outside of either actually held tb_lock or promised in a comment added by this patch? > + > struct GDBRegisterState *gdb_regs; > int gdb_num_regs; > int gdb_num_g_regs; > diff --git a/tcg/tcg.h b/tcg/tcg.h > index ea4ff02..a46d17c 100644 > --- a/tcg/tcg.h > +++ b/tcg/tcg.h > @@ -609,6 +609,7 @@ static inline bool tcg_op_buf_full(void) > > /* pool based memory allocation */ > > +/* tb_lock must be held for tcg_malloc_internal. */ How far are we ready to go in commenting such functions? The functions can be divided into three categories: * extern * static, called only from another one function (for better code structuring) * static, called from multiple other functions (for encapsulation of common code) I think we should decide how to comment locking requirements and follow this consistently. Concerning this particular case, I think there's no much point in making tcg_malloc_internal() and tcg_malloc() externally visible and commenting locking requirement for them. We'd better have a separate header file under include/ for external TCG interface declarations and use this one as internal only in tcg/. > void *tcg_malloc_internal(TCGContext *s, int size); > void tcg_pool_reset(TCGContext *s); > void tcg_pool_delete(TCGContext *s); > @@ -617,6 +618,7 @@ void tb_lock(void); > void tb_unlock(void); > void tb_lock_reset(void); > > +/* Called with tb_lock held. */ > static inline void *tcg_malloc(int size) > { > TCGContext *s = &tcg_ctx; > diff --git a/translate-all.c b/translate-all.c > index d923008..a7ff5e7 100644 > --- a/translate-all.c > +++ b/translate-all.c > @@ -248,7 +248,9 @@ static int encode_search(TranslationBlock *tb, uint8_t *block) > return p - block; > } > > -/* The cpu state corresponding to 'searched_pc' is restored. */ > +/* The cpu state corresponding to 'searched_pc' is restored. > + * Called with tb_lock held. > + */ > static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb, > uintptr_t searched_pc) > { > @@ -306,8 +308,10 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t retaddr) cpu_restore_state_from_tb() called right here also requires tb_lock(). > if (tb->cflags & CF_NOCACHE) { > /* one-shot translation, invalidate it immediately */ > cpu->current_tb = NULL; > + tb_lock(); > tb_phys_invalidate(tb, -1); > tb_free(tb); > + tb_unlock(); > } > return true; > } > @@ -399,6 +403,7 @@ static void page_init(void) > } > > /* If alloc=1: > + * Called with tb_lock held for system emulation. > * Called with mmap_lock held for user-mode emulation. There's a number of functions where their comment states that tb_lock should be held for system emulation but mmap_lock for user-mode emulation. BTW, what is the purpose of mmap_lock? And how it is related to tb_lock? > */ > static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc) (snip) > @@ -1429,7 +1446,9 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len) > } > if (!p->code_bitmap && > ++p->code_write_count >= SMC_BITMAP_USE_THRESHOLD) { > - /* build code bitmap */ > + /* build code bitmap. FIXME: writes should be protected by > + * tb_lock, reads by tb_lock or RCU. > + */ Probably, page_find() called earlier in this function requires tb_lock held as well as tb_invalidate_phys_page_range() which can be called later. Maybe tb_invalidate_phys_page_fast() is a good candidate to be always called with tb_lock held. > build_page_bitmap(p); > } > if (p->code_bitmap) { (snip) A list of candidates (as of rth/tcg-next) to also have such a comment includes: tb_find_physical() tb_find_slow() tb_hash_remove() tb_page_remove() tb_remove_from_jmp_list() tb_jmp_unlink() build_page_bitmap() tb_link_page() tb_invalidate_phys_range() tb_invalidate_phys_page_range() page_find() invalidate_page_bitmap() tb_invalidate_check() tb_invalidate_phys_page_fast() tb_invalidate_phys_page() tb_invalidate_phys_addr() cpu_io_recompile() However, there's no sensible description of what is protected by tb_lock and mmap_lock. I think we need to have a clear documented description of the TCG locking scheme in order to be sure we do right things in MTTCG. Kind regards, Sergey
On 06/05/2016 20:22, Sergey Fedorov wrote: > However, there's no sensible description of what is protected by tb_lock > and mmap_lock. I think we need to have a clear documented description of > the TCG locking scheme in order to be sure we do right things in MTTCG. I think there was such a patch somewhere, but: tb_lock basically protects tcg_ctx, while mmap_lock protects the user-mode emulation page table (the equivalent for system emulation is the memory map which is protected by the BQL). Furthermore, mmap_lock must be taken outside tb_lock. Paolo
On 11/05/16 15:58, Paolo Bonzini wrote: > > On 06/05/2016 20:22, Sergey Fedorov wrote: >> However, there's no sensible description of what is protected by tb_lock >> and mmap_lock. I think we need to have a clear documented description of >> the TCG locking scheme in order to be sure we do right things in MTTCG. > I think there was such a patch somewhere, but: tb_lock basically > protects tcg_ctx, while mmap_lock protects the user-mode emulation page > table (the equivalent for system emulation is the memory map which is > protected by the BQL). Furthermore, mmap_lock must be taken outside > tb_lock. What's a user-mode emulation page table? 'l1_map'? It is used by system emulation to keep track of TBs per page and 'code_bitmap'. Shouldn't it be protected with 'mmap_lock' in system emulation? Thanks, Sergey
On 11/05/2016 15:36, Sergey Fedorov wrote: > On 11/05/16 15:58, Paolo Bonzini wrote: >> >> On 06/05/2016 20:22, Sergey Fedorov wrote: >>> However, there's no sensible description of what is protected by tb_lock >>> and mmap_lock. I think we need to have a clear documented description of >>> the TCG locking scheme in order to be sure we do right things in MTTCG. >> I think there was such a patch somewhere, but: tb_lock basically >> protects tcg_ctx, while mmap_lock protects the user-mode emulation page >> table (the equivalent for system emulation is the memory map which is >> protected by the BQL). Furthermore, mmap_lock must be taken outside >> tb_lock. > > What's a user-mode emulation page table? 'l1_map'? Yes. It's used beyond TCG in user-mode emulation. > It is used by system > emulation to keep track of TBs per page and 'code_bitmap'. Shouldn't it > be protected with 'mmap_lock' in system emulation? tb_lock is used instead because it's taken everywhere system emulation uses l1_map; so tb_lock is protecting l1_map too in system emulation. As mentioned above, user-mode emulation uses l1_map in linux-user/mmap.c via page_{get,set}_flags, which I guess is why the lock is separate. None of us was involved in the original multi-threaded linux-user work, we're reverse engineering it just like you. :) Thanks, Paolo
On 11/05/16 16:46, Paolo Bonzini wrote: > On 11/05/2016 15:36, Sergey Fedorov wrote: >> On 11/05/16 15:58, Paolo Bonzini wrote: >>> On 06/05/2016 20:22, Sergey Fedorov wrote: >>>> However, there's no sensible description of what is protected by tb_lock >>>> and mmap_lock. I think we need to have a clear documented description of >>>> the TCG locking scheme in order to be sure we do right things in MTTCG. >>> I think there was such a patch somewhere, but: tb_lock basically >>> protects tcg_ctx, while mmap_lock protects the user-mode emulation page >>> table (the equivalent for system emulation is the memory map which is >>> protected by the BQL). Furthermore, mmap_lock must be taken outside >>> tb_lock. >> What's a user-mode emulation page table? 'l1_map'? > Yes. It's used beyond TCG in user-mode emulation. > >> It is used by system >> emulation to keep track of TBs per page and 'code_bitmap'. Shouldn't it >> be protected with 'mmap_lock' in system emulation? > tb_lock is used instead because it's taken everywhere system emulation > uses l1_map; so tb_lock is protecting l1_map too in system emulation. > > As mentioned above, user-mode emulation uses l1_map in linux-user/mmap.c > via page_{get,set}_flags, which I guess is why the lock is separate. > None of us was involved in the original multi-threaded linux-user work, > we're reverse engineering it just like you. :) While I'm investigating 'tb_lock' and 'mmap_lock' usage I am wondering why don't put 'l1_map' into 'tcg_ctx'? Kind regards, Sergey
On 12/05/2016 21:32, Sergey Fedorov wrote: >> > >> > As mentioned above, user-mode emulation uses l1_map in linux-user/mmap.c >> > via page_{get,set}_flags, which I guess is why the lock is separate. >> > None of us was involved in the original multi-threaded linux-user work, >> > we're reverse engineering it just like you. :) > While I'm investigating 'tb_lock' and 'mmap_lock' usage I am wondering > why don't put 'l1_map' into 'tcg_ctx'? Yes, that makes sense. Paolo
diff --git a/exec.c b/exec.c index f46e596..17f390e 100644 --- a/exec.c +++ b/exec.c @@ -826,6 +826,7 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags, { CPUBreakpoint *bp; + /* TODO: locking (RCU?) */ bp = g_malloc(sizeof(*bp)); bp->pc = pc; diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index bbd9807..60716ae 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -386,6 +386,7 @@ static inline void tb_set_jmp_target(TranslationBlock *tb, #endif +/* Called with tb_lock held. */ static inline void tb_add_jump(TranslationBlock *tb, int n, TranslationBlock *tb_next) { diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 6931db9..13eeaae 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -306,7 +306,10 @@ struct CPUState { void *env_ptr; /* CPUArchState */ struct TranslationBlock *current_tb; + + /* Writes protected by tb_lock, reads not thread-safe */ struct TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE]; + struct GDBRegisterState *gdb_regs; int gdb_num_regs; int gdb_num_g_regs; diff --git a/tcg/tcg.h b/tcg/tcg.h index ea4ff02..a46d17c 100644 --- a/tcg/tcg.h +++ b/tcg/tcg.h @@ -609,6 +609,7 @@ static inline bool tcg_op_buf_full(void) /* pool based memory allocation */ +/* tb_lock must be held for tcg_malloc_internal. */ void *tcg_malloc_internal(TCGContext *s, int size); void tcg_pool_reset(TCGContext *s); void tcg_pool_delete(TCGContext *s); @@ -617,6 +618,7 @@ void tb_lock(void); void tb_unlock(void); void tb_lock_reset(void); +/* Called with tb_lock held. */ static inline void *tcg_malloc(int size) { TCGContext *s = &tcg_ctx; diff --git a/translate-all.c b/translate-all.c index d923008..a7ff5e7 100644 --- a/translate-all.c +++ b/translate-all.c @@ -248,7 +248,9 @@ static int encode_search(TranslationBlock *tb, uint8_t *block) return p - block; } -/* The cpu state corresponding to 'searched_pc' is restored. */ +/* The cpu state corresponding to 'searched_pc' is restored. + * Called with tb_lock held. + */ static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb, uintptr_t searched_pc) { @@ -306,8 +308,10 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t retaddr) if (tb->cflags & CF_NOCACHE) { /* one-shot translation, invalidate it immediately */ cpu->current_tb = NULL; + tb_lock(); tb_phys_invalidate(tb, -1); tb_free(tb); + tb_unlock(); } return true; } @@ -399,6 +403,7 @@ static void page_init(void) } /* If alloc=1: + * Called with tb_lock held for system emulation. * Called with mmap_lock held for user-mode emulation. */ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc) @@ -754,8 +759,12 @@ bool tcg_enabled(void) return tcg_ctx.code_gen_buffer != NULL; } -/* Allocate a new translation block. Flush the translation buffer if - too many translation blocks or too much generated code. */ +/* + * Allocate a new translation block. Flush the translation buffer if + * too many translation blocks or too much generated code. + * + * Called with tb_lock held. + */ static TranslationBlock *tb_alloc(target_ulong pc) { TranslationBlock *tb; @@ -769,6 +778,7 @@ static TranslationBlock *tb_alloc(target_ulong pc) return tb; } +/* Called with tb_lock held. */ void tb_free(TranslationBlock *tb) { /* In practice this is mostly used for single use temporary TB @@ -874,7 +884,10 @@ static void tb_invalidate_check(target_ulong address) } } -/* verify that all the pages have correct rights for code */ +/* verify that all the pages have correct rights for code + * + * Called with tb_lock held. + */ static void tb_page_check(void) { TranslationBlock *tb; @@ -985,7 +998,10 @@ static inline void tb_jmp_unlink(TranslationBlock *tb) } } -/* invalidate one TB */ +/* invalidate one TB + * + * Called with tb_lock held. + */ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr) { CPUState *cpu; @@ -1062,6 +1078,7 @@ static void build_page_bitmap(PageDesc *p) /* add the tb in the target page and protect it if necessary * + * Called with tb_lock held. * Called with mmap_lock held for user-mode emulation. */ static inline void tb_alloc_page(TranslationBlock *tb, @@ -1429,7 +1446,9 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len) } if (!p->code_bitmap && ++p->code_write_count >= SMC_BITMAP_USE_THRESHOLD) { - /* build code bitmap */ + /* build code bitmap. FIXME: writes should be protected by + * tb_lock, reads by tb_lock or RCU. + */ build_page_bitmap(p); } if (p->code_bitmap) { @@ -1571,6 +1590,7 @@ void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr) } #endif /* !defined(CONFIG_USER_ONLY) */ +/* Called with tb_lock held. */ void tb_check_watchpoint(CPUState *cpu) { TranslationBlock *tb;