From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40845) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aykOe-00043r-2Q for qemu-devel@nongnu.org; Fri, 06 May 2016 14:22:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aykOR-00073T-WB for qemu-devel@nongnu.org; Fri, 06 May 2016 14:22:34 -0400 Received: from mail-lf0-x242.google.com ([2a00:1450:4010:c07::242]:33829) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aykOQ-0006xl-IE for qemu-devel@nongnu.org; Fri, 06 May 2016 14:22:27 -0400 Received: by mail-lf0-x242.google.com with SMTP id m101so14148840lfi.1 for ; Fri, 06 May 2016 11:22:12 -0700 (PDT) References: <1459870344-16773-1-git-send-email-alex.bennee@linaro.org> <1459870344-16773-5-git-send-email-alex.bennee@linaro.org> From: Sergey Fedorov Message-ID: <572CE0CD.3040004@gmail.com> Date: Fri, 6 May 2016 21:22:05 +0300 MIME-Version: 1.0 In-Reply-To: <1459870344-16773-5-git-send-email-alex.bennee@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC v2 04/11] tcg: comment on which functions have to be called with tb_lock held List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Alex_Benn=c3=a9e?= , mttcg@listserver.greensocs.com, fred.konrad@greensocs.com, a.rigo@virtualopensystems.com, cota@braap.org Cc: qemu-devel@nongnu.org, mark.burton@greensocs.com, pbonzini@redhat.com, jan.kiszka@siemens.com, rth@twiddle.net, peter.maydell@linaro.org, claudio.fontana@huawei.com, Peter Crosthwaite , =?UTF-8?Q?Andreas_F=c3=a4rber?= On 05/04/16 18:32, Alex Bennée wrote: > From: Paolo Bonzini > > 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 > Signed-off-by: Alex Bennée (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