On 21/07/16 14:25, Paolo Bonzini wrote: > > ----- Original Message ----- >> From: "Sergey Fedorov" <serge.f...@gmail.com> >> To: "Paolo Bonzini" <pbonz...@redhat.com> >> Cc: qemu-devel@nongnu.org, "sergey fedorov" <sergey.fedo...@linaro.org>, >> "alex bennee" <alex.ben...@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.f...@gmail.com> >>>> To: "Paolo Bonzini" <pbonz...@redhat.com>, qemu-devel@nongnu.org >>>> Cc: "sergey fedorov" <sergey.fedo...@linaro.org>, "alex bennee" >>>> <alex.ben...@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.f...@gmail.com> >>>>> Signed-off-by: Paolo Bonzini <pbonz...@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.
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