From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48715) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQC70-0002XE-Ko for qemu-devel@nongnu.org; Thu, 21 Jul 2016 07:25:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQC6v-0005L4-1e for qemu-devel@nongnu.org; Thu, 21 Jul 2016 07:25:53 -0400 Received: from mx5-phx2.redhat.com ([209.132.183.37]:56636) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQC6u-0005Kn-PG for qemu-devel@nongnu.org; Thu, 21 Jul 2016 07:25:48 -0400 Date: Thu, 21 Jul 2016 07:25:44 -0400 (EDT) From: Paolo Bonzini Message-ID: <1521389053.9427118.1469100344429.JavaMail.zimbra@redhat.com> In-Reply-To: <57908993.50100@gmail.com> References: <1468917141-8155-1-git-send-email-pbonzini@redhat.com> <1468917141-8155-6-git-send-email-pbonzini@redhat.com> <578E8601.1080107@gmail.com> <1825442518.8624290.1468967262856.JavaMail.zimbra@redhat.com> <57908993.50100@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 05/10] tcg: Prepare TB invalidation for lockless TB lookup List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Fedorov Cc: qemu-devel@nongnu.org, sergey fedorov , alex bennee ----- Original Message ----- > From: "Sergey Fedorov" > To: "Paolo Bonzini" > Cc: qemu-devel@nongnu.org, "sergey fedorov" , "alex bennee" > 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" > >> To: "Paolo Bonzini" , qemu-devel@nongnu.org > >> Cc: "sergey fedorov" , "alex bennee" > >> > >> 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 > >>> Signed-off-by: Paolo Bonzini > >>> --- > >>> 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