From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38873) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asAkF-0005u5-9Z for qemu-devel@nongnu.org; Mon, 18 Apr 2016 11:05:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1asAkB-00081I-8C for qemu-devel@nongnu.org; Mon, 18 Apr 2016 11:05:47 -0400 Received: from mail-lf0-x236.google.com ([2a00:1450:4010:c07::236]:36581) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asAkA-00081C-SD for qemu-devel@nongnu.org; Mon, 18 Apr 2016 11:05:43 -0400 Received: by mail-lf0-x236.google.com with SMTP id g184so220280593lfb.3 for ; Mon, 18 Apr 2016 08:05:42 -0700 (PDT) References: <1460666749-24452-1-git-send-email-sergey.fedorov@linaro.org> <1460666749-24452-5-git-send-email-sergey.fedorov@linaro.org> <87inzfvwiq.fsf@linaro.org> From: Sergey Fedorov Message-ID: <5714F7C4.6040306@gmail.com> Date: Mon, 18 Apr 2016 18:05:40 +0300 MIME-Version: 1.0 In-Reply-To: <87inzfvwiq.fsf@linaro.org> Content-Type: multipart/alternative; boundary="------------050704000806080402080707" Subject: Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Alex_Benn=c3=a9e?= , Sergey Fedorov Cc: qemu-devel@nongnu.org, Paolo Bonzini , Peter Crosthwaite , Richard Henderson , =?UTF-8?Q?Andreas_F=c3=a4rber?= This is a multi-part message in MIME format. --------------050704000806080402080707 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 18/04/16 17:09, Alex Bennée wrote: > Sergey Fedorov writes: >> From: Sergey Fedorov >> >> 'tb_invalidated_flag' was meant to catch two events: >> * some TB has been invalidated by tb_phys_invalidate(); >> * the whole translation buffer has been flushed by tb_flush(). >> >> Then it was checked: >> * in cpu_exec() to ensure that the last executed TB can be safely >> linked to directly call the next one; >> * in cpu_exec_nocache() to decide if the original TB should be provided >> for further possible invalidation along with the temporarily >> generated TB. >> >> It is always safe to patch an invalidated TB since it is not going to be >> used anyway. > Wouldn't that have implications for code searching through the linked > list of jump patched TBs? The only implication I can see is that the jump in that already invalidated TB could just get reset back later on in tb_phys_invalidate(). We could keep track of invalidated TB's and skip patching those but it's also some overhead in the main CPU execution loop wich I'm not sure is worth to be introduced. (snip) >> diff --git a/cpu-exec.c b/cpu-exec.c (snip) >> @@ -507,14 +510,12 @@ int cpu_exec(CPUState *cpu) >> } >> tb_lock(); >> tb = tb_find_fast(cpu); >> - /* Note: we do it here to avoid a gcc bug on Mac OS X when >> - doing it in tb_find_slow */ > Is this still true? Would it make more sense to push the patching down > to the gen_code? This comment comes up to the commit: commit 1538800276aa7228d74f9d00bf275f54dc9e9b43 Author: bellard Date: Mon Dec 19 01:42:32 2005 +0000 workaround for gcc bug on PowerPC It was added more than ten years ago. Anyway, now this code is here not because of the bug: we need to reset 'next_tb' which is a local variable in cpu_exec(). Personally, I don't think it would be neater to hide it into gen_code(). Do you have some thoughts on how we could benefit from doing so? BTW, I had a feeling that it may be useful to reorganize cpu_exec() a bit, although I don't have a solid idea of how to do this so far. > > I got slightly confused as to what next_tb ends up meaning at what point > in the run loop. Yes, it seems to be a misleading name for this variable. As it was discussed on IRC, I'd like to break it into two variables, say 'last_tb' and 'tb_exit_idx', as soon as cpu_tb_exec() returns. Probably this series could also include such a patch. Kind regards, Sergey --------------050704000806080402080707 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit
On 18/04/16 17:09, Alex Bennée wrote:
Sergey Fedorov <sergey.fedorov@linaro.org> writes:

From: Sergey Fedorov <serge.fdrv@gmail.com>

'tb_invalidated_flag' was meant to catch two events:
 * some TB has been invalidated by tb_phys_invalidate();
 * the whole translation buffer has been flushed by tb_flush().

Then it was checked:
 * in cpu_exec() to ensure that the last executed TB can be safely
   linked to directly call the next one;
 * in cpu_exec_nocache() to decide if the original TB should be provided
   for further possible invalidation along with the temporarily
   generated TB.

It is always safe to patch an invalidated TB since it is not going to be
used anyway.

Wouldn't that have implications for code searching through the linked
list of jump patched TBs?

The only implication I can see is that the jump in that already invalidated TB could just get reset back later on in tb_phys_invalidate(). We could keep track of invalidated TB's and skip patching those but it's also some overhead in the main CPU execution loop wich I'm not sure is worth to be introduced.

(snip)
diff --git a/cpu-exec.c b/cpu-exec.c
(snip)
@@ -507,14 +510,12 @@ int cpu_exec(CPUState *cpu)
                 }
                 tb_lock();
                 tb = tb_find_fast(cpu);
-                /* Note: we do it here to avoid a gcc bug on Mac OS X when
-                   doing it in tb_find_slow */

Is this still true? Would it make more sense to push the patching down
to the gen_code?

This comment comes up to the commit:

commit 1538800276aa7228d74f9d00bf275f54dc9e9b43              
Author: bellard <bellard@c046a42c-6fe2-441c-8c8c-71466251a162>
Date:   Mon Dec 19 01:42:32 2005 +0000                       

    workaround for gcc bug on PowerPC

It was added more than ten years ago. Anyway, now this code is here not because of the bug: we need to reset 'next_tb' which is a local variable in cpu_exec(). Personally, I don't think it would be neater to hide it into gen_code(). Do you have some thoughts on how we could benefit from doing so? BTW, I had a feeling that it may be useful to reorganize cpu_exec() a bit, although I don't have a solid idea of how to do this so far.


I got slightly confused as to what next_tb ends up meaning at what point
in the run loop.

Yes, it seems to be a misleading name for this variable. As it was discussed on IRC, I'd like to break it into two variables, say 'last_tb' and 'tb_exit_idx', as soon as cpu_tb_exec() returns. Probably this series could also include such a patch.

Kind regards,
Sergey
--------------050704000806080402080707--