Re: [Qemu-devel] [PATCH 05/10] tcg: Prepare TB invalidation for lockless TB lookup

[Paolo Bonzini <pbonzini@redhat.com>]

----- Original Message -----
> From: "Sergey Fedorov" <serge.fdrv@gmail.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, "sergey fedorov" <sergey.fedorov@linaro.org>, "alex bennee" <alex.bennee@linaro.org>
> Sent: Thursday, July 21, 2016 10:36:35 AM
> Subject: Re: [PATCH 05/10] tcg: Prepare TB invalidation for lockless TB lookup
> 
> On 20/07/16 01:27, Paolo Bonzini wrote:
> >
> > ----- Original Message -----
> >> From: "Sergey Fedorov" <serge.fdrv@gmail.com>
> >> To: "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
> >> Cc: "sergey fedorov" <sergey.fedorov@linaro.org>, "alex bennee"
> >> <alex.bennee@linaro.org>
> >> Sent: Tuesday, July 19, 2016 9:56:49 PM
> >> Subject: Re: [PATCH 05/10] tcg: Prepare TB invalidation for lockless TB
> >> lookup
> >>
> >> On 19/07/16 11:32, Paolo Bonzini wrote:
> >> It looks much better now :)
> >>
> >>> When invalidating a translation block, set an invalid flag into the
> >>> TranslationBlock structure first.  It is also necessary to check whether
> >>> the target TB is still valid after acquiring 'tb_lock' but before calling
> >>> tb_add_jump() since TB lookup is to be performed out of 'tb_lock' in
> >>> future. Note that we don't have to check 'last_tb'; an already
> >>> invalidated
> >>> TB will not be executed anyway and it is thus safe to patch it.
> >>>
> >>> Suggested-by: Sergey Fedorov <serge.fdrv@gmail.com>
> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >>> ---
> >>>  cpu-exec.c              | 5 +++--
> >>>  include/exec/exec-all.h | 2 ++
> >>>  translate-all.c         | 3 +++
> >>>  3 files changed, 8 insertions(+), 2 deletions(-)
> >> (snip)
> >>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> >>> index acda7b6..bc0bcc5 100644
> >>> --- a/include/exec/exec-all.h
> >>> +++ b/include/exec/exec-all.h
> >>> @@ -213,6 +213,8 @@ struct TranslationBlock {
> >>>  #define CF_USE_ICOUNT  0x20000
> >>>  #define CF_IGNORE_ICOUNT 0x40000 /* Do not generate icount code */
> >>>  
> >>> +    uint16_t invalid;
> >> Why not "int"?
> > There's a hole there, we may want to move something else so I
> > used a smaller data type.  Even uint8_t would do.
> 
> But could simple "bool" work as well here?
> 
> >
> > Paolo
> >>> +
> >>>      void *tc_ptr;    /* pointer to the translated code */
> >>>      uint8_t *tc_search;  /* pointer to search data */
> 
> Are you sure that the hole is over there, not here?

Yes, all pointers have the same size.  For 32-bit hosts, my
patch introduces a 2-byte hole.  For 64-bit hosts, it reduces
a 4-byte hole to 2-byte.

Before:

    target_ulong pc;      /* 0 */ 
    target_ulong cs_base; /* 4 */ 
    uint32_t flags;       /* 8  */
    uint16_t size;        /* 12 */
    uint16_t icount;      /* 14 */
    uint32_t cflags;      /* 16 */
    /* 4 byte padding     ** 20 on 64-bit systems */
    void *tc_ptr;         /* 24 on 64-bit systems, 20 on 32-bit */

After:

    target_ulong pc;      /* 0 */ 
    target_ulong cs_base; /* 4 */ 
    uint32_t flags;       /* 8  */
    uint16_t size;        /* 12 */
    uint16_t icount;      /* 14 */
    uint32_t cflags;      /* 16 */
    uint16_t invalid;     /* 20 */
    /* 2 byte padding     ** 22 */
    void *tc_ptr;         /* 24 */

BTW, another reason to use uint16_t is that I suspect tb->icount can
be made redundant with cflags, so we might move tb->invalid up if
tb->icount is removed.

Thanks,

Paolo