Re: [Qemu-devel] [PATCH 11/10] tcg: comment on which functions have to be called with tb_lock held

[Frederic Konrad]

On 13/08/2015 14:59, Paolo Bonzini wrote:
> On 13/08/2015 14:51, Frederic Konrad wrote:
> > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > > index 77bbff2..56b1f4d 100644
> > > --- a/include/qom/cpu.h
> > > +++ b/include/qom/cpu.h
> > > @@ -285,7 +285,10 @@ struct CPUState {
> > >          void *env_ptr; /* CPUArchState */
> > >        struct TranslationBlock *current_tb;
> > > +
> > > +    /* Protected by tb_lock.  */
> > >        struct TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
> > This is temporary as a first step?
> Yes, I now saw that tb_lock has a huge contention in tb_find_fast. :)
Yes it is just enormous. Makes MTTCG 2x slower than upstream :).

> I've now extracted parts of your patch "tcg: protect TBContext with
> tb_lock" into a separate "tcg: move tb_find_fast outside the tb_lock
> critical section" that also applies to user-mode emulation.  That way I
> get good scalability on Dhrystone, same as with your branch.

I guess with the whole tlb/tb flush safe? Which is theorically protecting
tb_jmp_cache (or at least let only the right thread accessing it).
The drawback of all that is I'm not sure this is faster when we have a 
lot of
context switches. For tb_flush it's not really a problem as it happen 
approximately
never but the tb_invalidate, tlb_*_flush are more regular.

Fred
> Do you agree with the first 10 patches as a first step towards
> upstreaming the MTTCG work?
>
> Paolo
>
> > > +
> > >        struct GDBRegisterState *gdb_regs;
> > >        int gdb_num_regs;
> > >        int gdb_num_g_regs;
> > > diff --git a/tcg/tcg.h b/tcg/tcg.h
> > > index 0ae648f..a2cad31 100644
> > > --- a/tcg/tcg.h
> > > +++ b/tcg/tcg.h
> > > @@ -590,6 +590,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);
> > > @@ -598,6 +599,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 edb9cb1..17d3cd1 100644
> > > --- a/translate-all.c
> > > +++ b/translate-all.c
> > > @@ -237,6 +237,7 @@ int cpu_gen_code(CPUArchState *env,
> > > TranslationBlock *tb, int *gen_code_size_ptr
> > >    }
> > >      /* 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)
> > > @@ -424,6 +425,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)
> > > @@ -734,8 +736,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)
> > >    {
> > There is the famous tb_flush which needs to be called with tb_lock held
> > as well.
> > There are several place where it's called.
> >
> > >        TranslationBlock *tb;
> > > @@ -751,6 +757,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
> > > @@ -859,7 +866,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;
> > > @@ -947,7 +957,10 @@ static inline void tb_reset_jump(TranslationBlock
> > > *tb, int n)
> > >        tb_set_jmp_target(tb, n, (uintptr_t)(tb->tc_ptr +
> > > tb->tb_next_offset[n]));
> > >    }
> > >    -/* 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;
> > > @@ -1036,7 +1049,7 @@ static void build_page_bitmap(PageDesc *p)
> > >    }
> > >    #endif
> > >    -/* Called with mmap_lock held for user mode emulation.  */
> > > +/* Called with tb_lock held, and mmap_lock too for user mode
> > > emulation.  */
> > >    TranslationBlock *tb_gen_code(CPUState *cpu,
> > >                                  target_ulong pc, target_ulong cs_base,
> > >                                  int flags, int cflags)
> > > @@ -1234,7 +1247,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) {
> > > @@ -1324,6 +1339,7 @@ static void
> > > tb_invalidate_phys_page(tb_page_addr_t addr,
> > >      /* 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,
> > > @@ -1383,6 +1399,7 @@ static inline void
> > > tb_alloc_page(TranslationBlock *tb,
> > >    /* add a new TB and link it to the physical page tables. phys_page2 is
> > >     * (-1) to indicate that only one page contains the TB.
> > >     *
> > > + * Called with tb_lock held.
> > >     * Called with mmap_lock held for user-mode emulation.
> > >     */
> > >    static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
> > > @@ -1423,7 +1440,10 @@ static void tb_link_page(TranslationBlock *tb,
> > > tb_page_addr_t phys_pc,
> > >    }
> > >      /* find the TB 'tb' such that tb[0].tc_ptr <= tc_ptr <
> > > -   tb[1].tc_ptr. Return NULL if not found */
> > > + * tb[1].tc_ptr. Return NULL if not found
> > > + *
> > > + * Called with tb_lock held.
> > > + */
> > >    static TranslationBlock *tb_find_pc(uintptr_t tc_ptr)
> > >    {
> > >        int m_min, m_max, m;
> > > @@ -1476,6 +1496,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;