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

[Sergey Fedorov]

On 21/07/16 14:25, Paolo Bonzini wrote:
>
> ----- Original Message -----
>> From: "Sergey Fedorov" <address@hidden>
>> To: "Paolo Bonzini" <address@hidden>
>> Cc: address@hidden, "sergey fedorov" <address@hidden>, "alex bennee" 
>> <address@hidden>
>> 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" <address@hidden>
>>>> To: "Paolo Bonzini" <address@hidden>, address@hidden
>>>> Cc: "sergey fedorov" <address@hidden>, "alex bennee"
>>>> <address@hidden>
>>>> 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 <address@hidden>
>>>>> Signed-off-by: Paolo Bonzini <address@hidden>
>>>>> ---
>>>>>  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.

Fair enough. I think it might make sense to use uint8_t to make a hint
for possible future compaction of TranslationBlock structure.

Thanks,
Sergey