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

[Paolo Bonzini]

On 21/07/2016 10:36, Sergey Fedorov wrote:
> 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?

sizeof(bool) is sometimes 1 sometimes 4.  Since in the future we might
want to pack TranslationBlock for better locality, I thought it was
better to stick with uint*_t.

Paolo

> 
>>
>> 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?
> 
> Kind regards,
> Sergey
> 
>>>>      /* original tb when cflags has CF_NOCACHE */
>>>>
>>> Thanks,
>>> Sergey
>>>
> 
> 
>