[Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Paolo Bonzini]

QEMU running with options "-icount,sleep=off -rtc clock=vm" doesn't
execute emulation at maximum possible speed.
Target virtual clock may run faster or slower than realtime clock by N
times, where N value depends on various unrelated conditions (i.e. random
from the user point of view), or possibly the target hangs completely.
Bisection shows commit 281b2201e4 ("icount: remove obsolete warp call",
2016-03-15) to be the culprit, revert it.

Suggested-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/cpus.c b/cpus.c
index 361678e459..2872e7e37c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1224,6 +1224,10 @@ static void qemu_tcg_rr_wait_io_event(CPUState *cpu)
 {
     while (all_cpu_threads_idle()) {
         stop_tcg_kick_timer();
+
+        /* Start accounting real time to the virtual clock if the CPUs
+           are idle.  */
+        qemu_clock_warp(QEMU_CLOCK_VIRTUAL);
         qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
     }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> QEMU running with options "-icount,sleep=off -rtc clock=vm" doesn't
> execute emulation at maximum possible speed.
> Target virtual clock may run faster or slower than realtime clock by N
> times, where N value depends on various unrelated conditions (i.e. random
> from the user point of view), or possibly the target hangs completely.
> Bisection shows commit 281b2201e4 ("icount: remove obsolete warp call",
> 2016-03-15) to be the culprit, revert it.
> 
> Suggested-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cpus.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/cpus.c b/cpus.c
> index 361678e459..2872e7e37c 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1224,6 +1224,10 @@ static void qemu_tcg_rr_wait_io_event(CPUState *cpu)
>  {
>      while (all_cpu_threads_idle()) {
>          stop_tcg_kick_timer();
> +
> +        /* Start accounting real time to the virtual clock if the CPUs
> +           are idle.  */
> +        qemu_clock_warp(QEMU_CLOCK_VIRTUAL);

Can't apply, because this function does not exist anymore.

>          qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
>      }
> 
> --
> 2.17.1



Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Artem Pisarenko]

See my last comment in bug report. This kind of modification, even adapted
to changed function name, doesn't solve issue.
I thought long time that it does, but once I catched qemu with a hang. And
of course, I wasn't able to reproduce it. So it just better hides issue.
Take a look at alternative solution from QBox:
https://git.greensocs.com/qemu/qbox/commit/a8ed106032e375e715a531d6e93e4d9ec295dbdb
I didn't catched fail with it (yet).
-- 

С уважением,
  Артем Писаренко

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Paolo Bonzini]

On 17/10/2018 11:53, Artem Pisarenko wrote:
> See my last comment in bug report. This kind of modification, even
> adapted to changed function name, doesn't solve issue.
> I thought long time that it does, but once I catched qemu with a hang.
> And of course, I wasn't able to reproduce it. So it just better hides issue.
> Take a look at alternative solution from
> QBox: https://git.greensocs.com/qemu/qbox/commit/a8ed106032e375e715a531d6e93e4d9ec295dbdb
> I didn't catched fail with it (yet).

Makes sense (and the patch I submitted was stupid).  Clement, can you
submit the patch from qbox with Signed-off-by?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 17/10/2018 11:53, Artem Pisarenko wrote:
> > See my last comment in bug report. This kind of modification, even
> > adapted to changed function name, doesn't solve issue.
> > I thought long time that it does, but once I catched qemu with a hang.
> > And of course, I wasn't able to reproduce it. So it just better hides issue.
> > Take a look at alternative solution from
> > QBox: https://git.greensocs.com/qemu/qbox/commit/a8ed106032e375e715a531d6e93e4d9ec295dbdb
> > I didn't catched fail with it (yet).

Tried to test it, but rr seems to be broken again.
I'll try to bisect now.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Paolo Bonzini]

On 17/10/2018 13:38, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 17/10/2018 11:53, Artem Pisarenko wrote:
>>> See my last comment in bug report. This kind of modification, even
>>> adapted to changed function name, doesn't solve issue.
>>> I thought long time that it does, but once I catched qemu with a hang.
>>> And of course, I wasn't able to reproduce it. So it just better hides issue.
>>> Take a look at alternative solution from
>>> QBox: https://git.greensocs.com/qemu/qbox/commit/a8ed106032e375e715a531d6e93e4d9ec295dbdb
>>> I didn't catched fail with it (yet).
> 
> Tried to test it, but rr seems to be broken again.
> I'll try to bisect now.

Can we add a test that runs with "make check" and covers the basics of
record/replay's cpus.c bits?

rr is very cool, and we fixed/understood a lot of stuff when getting it
ready for inclusion.  But now it's constantly broken and every time we
change rr we also risk breaking icount.

Paolo

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 17/10/2018 13:38, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 17/10/2018 11:53, Artem Pisarenko wrote:
> >>> See my last comment in bug report. This kind of modification, even
> >>> adapted to changed function name, doesn't solve issue.
> >>> I thought long time that it does, but once I catched qemu with a hang.
> >>> And of course, I wasn't able to reproduce it. So it just better hides issue.
> >>> Take a look at alternative solution from
> >>> QBox: https://git.greensocs.com/qemu/qbox/commit/a8ed106032e375e715a531d6e93e4d9ec295dbdb
> >>> I didn't catched fail with it (yet).
> >
> > Tried to test it, but rr seems to be broken again.
> > I'll try to bisect now.
> 
> Can we add a test that runs with "make check" and covers the basics of
> record/replay's cpus.c bits?

I thought Alex is trying to create some tests.
Alex, am I right?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 17/10/2018 13:38, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 17/10/2018 11:53, Artem Pisarenko wrote:
> >>> See my last comment in bug report. This kind of modification, even
> >>> adapted to changed function name, doesn't solve issue.
> >>> I thought long time that it does, but once I catched qemu with a hang.
> >>> And of course, I wasn't able to reproduce it. So it just better hides issue.
> >>> Take a look at alternative solution from
> >>> QBox: https://git.greensocs.com/qemu/qbox/commit/a8ed106032e375e715a531d6e93e4d9ec295dbdb
> >>> I didn't catched fail with it (yet).
> >
> > Tried to test it, but rr seems to be broken again.
> > I'll try to bisect now.
> 
> Can we add a test that runs with "make check" and covers the basics of
> record/replay's cpus.c bits?
> 
> rr is very cool, and we fixed/understood a lot of stuff when getting it
> ready for inclusion.  But now it's constantly broken and every time we
> change rr we also risk breaking icount.

In addition to some rr bug I encountered the following (non-stable)
error message:

ERROR:/qemu/work/qemu/accel/tcg/translate-all.c:1346:tb_page_remove: code should not be reached

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Alex Bennée]

Pavel Dovgalyuk <dovgaluk@ispras.ru> writes:

>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 17/10/2018 13:38, Pavel Dovgalyuk wrote:
>> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> >> On 17/10/2018 11:53, Artem Pisarenko wrote:
>> >>> See my last comment in bug report. This kind of modification, even
>> >>> adapted to changed function name, doesn't solve issue.
>> >>> I thought long time that it does, but once I catched qemu with a hang.
>> >>> And of course, I wasn't able to reproduce it. So it just better hides issue.
>> >>> Take a look at alternative solution from
>> >>> QBox: https://git.greensocs.com/qemu/qbox/commit/a8ed106032e375e715a531d6e93e4d9ec295dbdb
>> >>> I didn't catched fail with it (yet).
>> >
>> > Tried to test it, but rr seems to be broken again.
>> > I'll try to bisect now.
>>
>> Can we add a test that runs with "make check" and covers the basics of
>> record/replay's cpus.c bits?
>
> I thought Alex is trying to create some tests.
> Alex, am I right?

Not actively at the moment but we need to get there. As rr only works
with system emulation we need to update the check-tcg machinery to be
able to build some system binaries. A number of the tests/tcg targets
already have some system binaries waiting to be ported to the new make
system.

The next question is what we need to exercise rr? The migration tests
have added some very simple ping-pong type "boot sectors" so perhaps
putting the source for those into check-tcg we can build them and use it
for rr testing?

--
Alex Bennée

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 17/10/2018 13:38, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 17/10/2018 11:53, Artem Pisarenko wrote:
> >>> See my last comment in bug report. This kind of modification, even
> >>> adapted to changed function name, doesn't solve issue.
> >>> I thought long time that it does, but once I catched qemu with a hang.
> >>> And of course, I wasn't able to reproduce it. So it just better hides issue.
> >>> Take a look at alternative solution from
> >>> QBox: https://git.greensocs.com/qemu/qbox/commit/a8ed106032e375e715a531d6e93e4d9ec295dbdb
> >>> I didn't catched fail with it (yet).
> >
> > Tried to test it, but rr seems to be broken again.
> > I'll try to bisect now.
> 
> Can we add a test that runs with "make check" and covers the basics of
> record/replay's cpus.c bits?
> 
> rr is very cool, and we fixed/understood a lot of stuff when getting it
> ready for inclusion.  But now it's constantly broken and every time we
> change rr we also risk breaking icount.

I found the source of the bug. As QEMU becomes more multi-threaded and non-synchronized,
checkpoints move from thread to thread.
And the event queue that processed at checkpoints should belong to the same thread
in both record and replay executions.

Current problem was with the checkpoint for virtual timers. They are processed from different threads:
from vCPU and from aio_dispatch function.

Therefore the following patch fixes the problem, but I think that this part has to be refactored.
There should be nailed-to-thread events that process the event queue. Then checkpoints can become
just synchronization events and therefore omitted for empty timer lists, for example.


diff --git a/replay/replay-events.c b/replay/replay-events.c
index 83a7d81..60e8c21 100644
--- a/replay/replay-events.c
+++ b/replay/replay-events.c
@@ -205,6 +205,7 @@ void replay_save_events(int checkpoint)
 {
     g_assert(replay_mutex_locked());
     g_assert(checkpoint != CHECKPOINT_CLOCK_WARP_START);
+    g_assert(checkpoint != CHECKPOINT_CLOCK_VIRTUAL);
     while (!QTAILQ_EMPTY(&events_list)) {
         Event *event = QTAILQ_FIRST(&events_list);
         replay_save_event(event, checkpoint);
diff --git a/replay/replay.c b/replay/replay.c
index 93d2573..79b8485 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -231,7 +231,8 @@ bool replay_checkpoint(ReplayCheckpoint checkpoint)
         /* This checkpoint belongs to several threads.
            Processing events from different threads is
            non-deterministic */
-        if (checkpoint != CHECKPOINT_CLOCK_WARP_START) {
+        if (checkpoint != CHECKPOINT_CLOCK_WARP_START
+            && checkpoint != CHECKPOINT_CLOCK_VIRTUAL) {
             replay_save_events(checkpoint);
         }
         res = true;

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Artem Pisarenko]

> I found the source of the bug. As QEMU becomes more multi-threaded and
non-> synchronized,
> checkpoints move from thread to thread.
> And the event queue that processed at checkpoints should belong to the
same thread
> in both record and replay executions.
>
> Current problem was with the checkpoint for virtual timers. They are
processed from different threads:
> from vCPU and from aio_dispatch function.

What bug you're talking about ? This patch thread started with intention to
fix generic bug with icount, which is being encountered in non-rr mode. As
far as I understood your words (and patch content) it will not fix that
generic bug.
Anyway, it's a good news!
-- 

С уважением,
  Артем Писаренко

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Paolo Bonzini]

On 17/10/2018 15:20, Pavel Dovgalyuk wrote:
> I found the source of the bug. As QEMU becomes more multi-threaded
> and non-synchronized, checkpoints move from thread to thread. And the
> event queue that processed at checkpoints should belong to the same
> thread in both record and replay executions.
> 
> Current problem was with the checkpoint for virtual timers. They are
> processed from different threads: from vCPU and from aio_dispatch
> function.
> 
> Therefore the following patch fixes the problem, but I think that
> this part has to be refactored. There should be nailed-to-thread
> events that process the event queue. Then checkpoints can become just
> synchronization events and therefore omitted for empty timer lists,
> for example.

Can you add a FIXME comment and submit this as a full patch?

Paolo

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Clement Deschamps]

Hi all,

I'm traveling right now, but as soon as I get home, I'll send my patch
with the signed-off.


Best,

Clément

On 10/17/18 7:43 AM, Paolo Bonzini wrote:
> On 17/10/2018 15:20, Pavel Dovgalyuk wrote:
>> I found the source of the bug. As QEMU becomes more multi-threaded
>> and non-synchronized, checkpoints move from thread to thread. And the
>> event queue that processed at checkpoints should belong to the same
>> thread in both record and replay executions.
>>
>> Current problem was with the checkpoint for virtual timers. They are
>> processed from different threads: from vCPU and from aio_dispatch
>> function.
>>
>> Therefore the following patch fixes the problem, but I think that
>> this part has to be refactored. There should be nailed-to-thread
>> events that process the event queue. Then checkpoints can become just
>> synchronization events and therefore omitted for empty timer lists,
>> for example.
> Can you add a FIXME comment and submit this as a full patch?
>
> Paolo

Re: [Qemu-devel] [PATCH] Revert "icount: remove obsolete warp call"

[Pavel Dovgalyuk]

I made a fix to ensure that QBox patch doesn’t break the replay.

Now I tried it on couple of tests and didn’t see any regression.

 

Pavel Dovgalyuk

 

From: Artem Pisarenko [mailto:artem.k.pisarenko@gmail.com] 
Sent: Wednesday, October 17, 2018 4:32 PM
To: Pavel Dovgalyuk
Cc: Paolo Bonzini; Clement Deschamps; qemu-devel@nongnu.org; Pavel Dovgalyuk
Subject: Re: [PATCH] Revert "icount: remove obsolete warp call"

 

> I found the source of the bug. As QEMU becomes more multi-threaded and non-> synchronized,

> checkpoints move from thread to thread.
> And the event queue that processed at checkpoints should belong to the same thread
> in both record and replay executions.
> 
> Current problem was with the checkpoint for virtual timers. They are processed from different threads:
> from vCPU and from aio_dispatch function.

 

What bug you're talking about ? This patch thread started with intention to fix generic bug with icount, which is being encountered in non-rr mode. As far as I understood your words (and patch content) it will not fix that generic bug.

Anyway, it's a good news!

-- 

С уважением,
  Артем Писаренко