Re: [Qemu-devel] [PATCH 11/22] translate-all: use a binary search tree to track TBs in TBContext

[Emilio G. Cota]

On Wed, Jul 12, 2017 at 16:10:15 +0100, Alex Bennée wrote:
> Emilio G. Cota <address@hidden> writes:
(snip)
> > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> > index fd20bca..673b26d 100644
> > --- a/include/exec/exec-all.h
> > +++ b/include/exec/exec-all.h
> > @@ -320,14 +320,25 @@ struct TranslationBlock {
> >      uint16_t size;      /* size of target code for this block (1 <=
> >                             size <= TARGET_PAGE_SIZE) */
> >      uint16_t icount;
> > -    uint32_t cflags;    /* compile flags */
> > +    /*
> > +     * @tc_size must be kept right after @tc_ptr to facilitate TB lookups 
> > in a
> > +     * binary search tree -- see struct ptr_size.
> > +     * We use an anonymous struct here to avoid updating all calling code,
> > +     * which would be quite a lot of churn.
> > +     * The only reason to bring @cflags into the anonymous struct is to
> > +     * avoid inducing a hole in TranslationBlock.
> > +     */
> > +    struct {
> > +        void *tc_ptr;    /* pointer to the translated code */
> > +        uint32_t tc_size; /* size of translated code for this block */
> > +
> > +        uint32_t cflags;    /* compile flags */
> >  #define CF_COUNT_MASK  0x7fff
> >  #define CF_LAST_IO     0x8000 /* Last insn may be an IO access.  */
> >  #define CF_NOCACHE     0x10000 /* To be freed after execution */
> >  #define CF_USE_ICOUNT  0x20000
> >  #define CF_IGNORE_ICOUNT 0x40000 /* Do not generate icount code */
> > -
> > -    void *tc_ptr;    /* pointer to the translated code */
> > +    };
> 
> Why not just have a named structure for this so there isn't ambiguity
> between struct ptrsize and this thing.

Yeah I did v2 of this patch yesterday. Turns out using an intermediate
struct here (and in the comparison code) doesn't end up in as much churn
as I expected, so I went with that.

(snip)
> > @@ -827,16 +853,7 @@ void tb_free(TranslationBlock *tb)
> >  {
> >      assert_tb_locked();
> >
> > -    /* In practice this is mostly used for single use temporary TB
> > -       Ignore the hard cases and just back up if this TB happens to
> > -       be the last one generated.  */
> > -    if (tcg_ctx.tb_ctx.nb_tbs > 0 &&
> > -            tb == tcg_ctx.tb_ctx.tbs[tcg_ctx.tb_ctx.nb_tbs - 1]) {
> > -        size_t struct_size = ROUND_UP(sizeof(*tb), qemu_icache_linesize);
> > -
> > -        tcg_ctx.code_gen_ptr = tb->tc_ptr - struct_size;
> > -        tcg_ctx.tb_ctx.nb_tbs--;
> > -    }
> > +    g_tree_remove(tcg_ctx.tb_ctx.tb_tree, &tb->tc_ptr);
> >  }
> 
> This function should be renamed as we never attempt to free (and it was
> pretty half hearted before). Maybe tb_remove or tb_deref?

Good point. I like tb_remove.

                Emilio