diff mbox

[RFC,v8,08/21] cpu: replay instructions sequence

Message ID 20150122085215.5276.8878.stgit@PASHA-ISP.def.inno
State New
Headers show

Commit Message

Pavel Dovgalyuk Jan. 22, 2015, 8:52 a.m. UTC
This patch adds calls to replay functions into the icount setup block.
In record mode number of executed instructions is written to the log.
In replay mode number of istructions to execute is taken from the replay log.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 cpu-exec.c      |    1 +
 cpus.c          |   28 ++++++++++++++++++----------
 replay/replay.c |   24 ++++++++++++++++++++++++
 replay/replay.h |    4 ++++
 4 files changed, 47 insertions(+), 10 deletions(-)

Comments

Paolo Bonzini Jan. 29, 2015, 9:32 a.m. UTC | #1
On 22/01/2015 09:52, Pavel Dovgalyuk wrote:
> This patch adds calls to replay functions into the icount setup block.
> In record mode number of executed instructions is written to the log.
> In replay mode number of istructions to execute is taken from the replay log.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  cpu-exec.c      |    1 +
>  cpus.c          |   28 ++++++++++++++++++----------
>  replay/replay.c |   24 ++++++++++++++++++++++++
>  replay/replay.h |    4 ++++
>  4 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 49f01f5..99a0993 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -531,6 +531,7 @@ int cpu_exec(CPUArchState *env)
>                              }
>                              cpu->exception_index = EXCP_INTERRUPT;
>                              next_tb = 0;
> +                            qemu_notify_event();

Why is this needed?

>                              cpu_loop_exit(cpu);
>                          }
>                          break;
> diff --git a/cpus.c b/cpus.c
> index c513275..8787277 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -41,6 +41,7 @@
>  #include "qemu/seqlock.h"
>  #include "qapi-event.h"
>  #include "hw/nmi.h"
> +#include "replay/replay.h"
>  
>  #ifndef _WIN32
>  #include "qemu/compatfd.h"
> @@ -1342,18 +1343,22 @@ static int tcg_cpu_exec(CPUArchState *env)
>                                      + cpu->icount_extra);
>          cpu->icount_decr.u16.low = 0;
>          cpu->icount_extra = 0;
> -        deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
> +        if (replay_mode != REPLAY_MODE_PLAY) {
> +            deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
>  
> -        /* Maintain prior (possibly buggy) behaviour where if no deadline
> -         * was set (as there is no QEMU_CLOCK_VIRTUAL timer) or it is more than
> -         * INT32_MAX nanoseconds ahead, we still use INT32_MAX
> -         * nanoseconds.
> -         */
> -        if ((deadline < 0) || (deadline > INT32_MAX)) {
> -            deadline = INT32_MAX;
> -        }
> +            /* Maintain prior (possibly buggy) behaviour where if no deadline
> +             * was set (as there is no QEMU_CLOCK_VIRTUAL timer) or it is more than
> +             * INT32_MAX nanoseconds ahead, we still use INT32_MAX
> +             * nanoseconds.
> +             */
> +            if ((deadline < 0) || (deadline > INT32_MAX)) {
> +                deadline = INT32_MAX;
> +            }
>  
> -        count = qemu_icount_round(deadline);
> +            count = qemu_icount_round(deadline);
> +        } else {
> +            count = replay_get_instructions();
> +        }

Please extract the "if" to a separate function tcg_get_icount_limit().

>          timers_state.qemu_icount += count;
>          decr = (count > 0xffff) ? 0xffff : count;
>          count -= decr;
> @@ -1371,6 +1376,9 @@ static int tcg_cpu_exec(CPUArchState *env)
>                          + cpu->icount_extra);
>          cpu->icount_decr.u32 = 0;
>          cpu->icount_extra = 0;
> +        if (replay_mode == REPLAY_MODE_PLAY) {
> +            replay_exec_instructions();

replay_account_executed_instructions()

> +        }
>      }
>      return ret;
>  }
> diff --git a/replay/replay.c b/replay/replay.c
> index a43bbbc..d6f5c4b 100755
> --- a/replay/replay.c
> +++ b/replay/replay.c
> @@ -58,3 +58,27 @@ uint64_t replay_get_current_step(void)
>  {
>      return cpu_get_icount_raw();
>  }
> +
> +int replay_get_instructions(void)
> +{
> +    int res = 0;
> +    replay_mutex_lock();
> +    if (skip_async_events(EVENT_INSTRUCTION)) {
> +        res = replay_state.instructions_count;
> +    }
> +    replay_mutex_unlock();
> +    return res;
> +}
> +
> +void replay_exec_instructions(void)
> +{
> +    if (replay_state.instructions_count > 0) {
> +        int count = (int)(replay_get_current_step()
> +                          - replay_state.current_step);
> +        replay_state.instructions_count -= count;
> +        replay_state.current_step += count;
> +        if (replay_state.instructions_count == 0 && count != 0) {

If replay_state.instructions_count is now zero, count must be nonzero
(because replay_state.instructions_count was > 0) before.

> +            replay_has_unread_data = 0;
> +        }

Can replay_state.instructions_count be < count at all?  If not, and if
replay_state.instructions_count is zero, then count must also be zero.

If so, I suggest rewriting as

        int count = (int)(replay_get_current_step()
                          - replay_state.current_step);
        assert(replay_state.instructions_count >= count);
        replay_state.instructions_count -= count;
        replay_state.current_step += count;
        if (replay_state.instructions_count == 0) {
            replay_has_unread_data = 0;
        }

Paolo

> +    }
> +}
> diff --git a/replay/replay.h b/replay/replay.h
> index a03c748..e425dea 100755
> --- a/replay/replay.h
> +++ b/replay/replay.h
> @@ -22,5 +22,9 @@ extern ReplayMode replay_mode;
>  
>  /*! Returns number of executed instructions. */
>  uint64_t replay_get_current_step(void);
> +/*! Returns number of instructions to execute in replay mode. */
> +int replay_get_instructions(void);
> +/*! Updates instructions counter in replay mode. */
> +void replay_exec_instructions(void);
>  
>  #endif
>
Pavel Dovgalyuk Feb. 2, 2015, 12:28 p.m. UTC | #2
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 22/01/2015 09:52, Pavel Dovgalyuk wrote:
> > This patch adds calls to replay functions into the icount setup block.
> > In record mode number of executed instructions is written to the log.
> > In replay mode number of istructions to execute is taken from the replay log.
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> >  cpu-exec.c      |    1 +
> >  cpus.c          |   28 ++++++++++++++++++----------
> >  replay/replay.c |   24 ++++++++++++++++++++++++
> >  replay/replay.h |    4 ++++
> >  4 files changed, 47 insertions(+), 10 deletions(-)
> >
> > diff --git a/cpu-exec.c b/cpu-exec.c
> > index 49f01f5..99a0993 100644
> > --- a/cpu-exec.c
> > +++ b/cpu-exec.c
> > @@ -531,6 +531,7 @@ int cpu_exec(CPUArchState *env)
> >                              }
> >                              cpu->exception_index = EXCP_INTERRUPT;
> >                              next_tb = 0;
> > +                            qemu_notify_event();
> 
> Why is this needed?

It is needed to wake up iothread in replay mode.
Otherwise it waits for additional time and replay becomes too slow.

> 
> >                              cpu_loop_exit(cpu);
> >                          }
> >                          break;

Pavel Dovgalyuk
Paolo Bonzini Feb. 2, 2015, 12:38 p.m. UTC | #3
On 02/02/2015 13:28, Pavel Dovgaluk wrote:
>>> > >                              cpu->exception_index = EXCP_INTERRUPT;
>>> > >                              next_tb = 0;
>>> > > +                            qemu_notify_event();
>> > 
>> > Why is this needed?
> It is needed to wake up iothread in replay mode.
> Otherwise it waits for additional time and replay becomes too slow.

What event (something from a timerlist?) is ready, that has not been
notified to the iothread yet?  qemu_notify_event() should never be
needed in common case.  It's probably missing somewhere else.

Paolo
Pavel Dovgalyuk Feb. 2, 2015, 12:42 p.m. UTC | #4
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 02/02/2015 13:28, Pavel Dovgaluk wrote:
> >>> > >                              cpu->exception_index = EXCP_INTERRUPT;
> >>> > >                              next_tb = 0;
> >>> > > +                            qemu_notify_event();
> >> >
> >> > Why is this needed?
> > It is needed to wake up iothread in replay mode.
> > Otherwise it waits for additional time and replay becomes too slow.
> 
> What event (something from a timerlist?) is ready, that has not been
> notified to the iothread yet?  qemu_notify_event() should never be
> needed in common case.  It's probably missing somewhere else.

I think in this case there are no events at all - just reading timers values
that were made while recording.
We have to replay these reads by waking iothread.

Pavel Dovgalyuk
Paolo Bonzini Feb. 2, 2015, 1:18 p.m. UTC | #5
On 02/02/2015 13:42, Pavel Dovgaluk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 02/02/2015 13:28, Pavel Dovgaluk wrote:
>>>>>>>                              cpu->exception_index = EXCP_INTERRUPT;
>>>>>>>                              next_tb = 0;
>>>>>>> +                            qemu_notify_event();
>>>>>
>>>>> Why is this needed?
>>> It is needed to wake up iothread in replay mode.
>>> Otherwise it waits for additional time and replay becomes too slow.
>>
>> What event (something from a timerlist?) is ready, that has not been
>> notified to the iothread yet?  qemu_notify_event() should never be
>> needed in common case.  It's probably missing somewhere else.
> 
> I think in this case there are no events at all - just reading timers values
> that were made while recording.
> We have to replay these reads by waking iothread.

I think the right place for this is in replay_read_next_clock then.

Paolo
Pavel Dovgalyuk Feb. 16, 2015, 12:26 p.m. UTC | #6
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> On 02/02/2015 13:42, Pavel Dovgaluk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 02/02/2015 13:28, Pavel Dovgaluk wrote:
> >>>>>>>                              cpu->exception_index = EXCP_INTERRUPT;
> >>>>>>>                              next_tb = 0;
> >>>>>>> +                            qemu_notify_event();
> >>>>>
> >>>>> Why is this needed?
> >>> It is needed to wake up iothread in replay mode.
> >>> Otherwise it waits for additional time and replay becomes too slow.
> >>
> >> What event (something from a timerlist?) is ready, that has not been
> >> notified to the iothread yet?  qemu_notify_event() should never be
> >> needed in common case.  It's probably missing somewhere else.
> >
> > I think in this case there are no events at all - just reading timers values
> > that were made while recording.
> > We have to replay these reads by waking iothread.
> 
> I think the right place for this is in replay_read_next_clock then.

It doesn't fit. Log file is not read until all instructions are executed.
And the next read from the file should be performed by iothread which should
be notified and waked up.

Pavel Dovgalyuk
Paolo Bonzini Feb. 16, 2015, 12:59 p.m. UTC | #7
On 16/02/2015 13:26, Pavel Dovgaluk wrote:
>>> > > I think in this case there are no events at all - just reading timers values
>>> > > that were made while recording.
>>> > > We have to replay these reads by waking iothread.
>> > 
>> > I think the right place for this is in replay_read_next_clock then.
> It doesn't fit. Log file is not read until all instructions are executed.
> And the next read from the file should be performed by iothread which should
> be notified and waked up.

I still don't understand.  If you're getting EXCP_INTERRUPT it means:

1) that cpu_signal was called

2) in turn this means that qemu_cpu_kick was called

3) in turn this means that the iothread is trying to run via
qemu_mutex_lock_iothread (the iothread_requesting_mutex stuff).  So why
do you need an explicit qemu_notify_event?

Paolo
Pavel Dovgalyuk Feb. 16, 2015, 1:27 p.m. UTC | #8
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> On 16/02/2015 13:26, Pavel Dovgaluk wrote:
> >>> > > I think in this case there are no events at all - just reading timers values
> >>> > > that were made while recording.
> >>> > > We have to replay these reads by waking iothread.
> >> >
> >> > I think the right place for this is in replay_read_next_clock then.
> > It doesn't fit. Log file is not read until all instructions are executed.
> > And the next read from the file should be performed by iothread which should
> > be notified and waked up.
> 
> I still don't understand.  If you're getting EXCP_INTERRUPT it means:
> 
> 1) that cpu_signal was called

No, it isn't. That is the branch when icount is expired.
And when it is expired in replay mode we have to wake up iothread,
because nobody will care about this.

> 
> 2) in turn this means that qemu_cpu_kick was called
> 
> 3) in turn this means that the iothread is trying to run via
> qemu_mutex_lock_iothread (the iothread_requesting_mutex stuff).  So why
> do you need an explicit qemu_notify_event?

Pavel Dovgalyuk
Paolo Bonzini Feb. 16, 2015, 1:31 p.m. UTC | #9
On 16/02/2015 14:27, Pavel Dovgaluk wrote:
>> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
>> On 16/02/2015 13:26, Pavel Dovgaluk wrote:
>>>>>>> I think in this case there are no events at all - just reading timers values
>>>>>>> that were made while recording.
>>>>>>> We have to replay these reads by waking iothread.
>>>>>
>>>>> I think the right place for this is in replay_read_next_clock then.
>>> It doesn't fit. Log file is not read until all instructions are executed.
>>> And the next read from the file should be performed by iothread which should
>>> be notified and waked up.
>>
>> I still don't understand.  If you're getting EXCP_INTERRUPT it means:
>>
>> 1) that cpu_signal was called
> 
> No, it isn't. That is the branch when icount is expired.
> And when it is expired in replay mode we have to wake up iothread,
> because nobody will care about this.

Then it's done here in qemu_tcg_cpu_thread_fn:

        if (use_icount) {
            int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);

            if (deadline == 0) {
                qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
            }
        }

If you need to move the 4 lines inside the if elsewhere, that I guess it's okay.

Paolo

>>
>> 2) in turn this means that qemu_cpu_kick was called
>>
>> 3) in turn this means that the iothread is trying to run via
>> qemu_mutex_lock_iothread (the iothread_requesting_mutex stuff).  So why
>> do you need an explicit qemu_notify_event?
> 
> Pavel Dovgalyuk
> 
> 
>
Pavel Dovgalyuk Feb. 16, 2015, 1:37 p.m. UTC | #10
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> On 16/02/2015 14:27, Pavel Dovgaluk wrote:
> >> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> >> On 16/02/2015 13:26, Pavel Dovgaluk wrote:
> >>>>>>> I think in this case there are no events at all - just reading timers values
> >>>>>>> that were made while recording.
> >>>>>>> We have to replay these reads by waking iothread.
> >>>>>
> >>>>> I think the right place for this is in replay_read_next_clock then.
> >>> It doesn't fit. Log file is not read until all instructions are executed.
> >>> And the next read from the file should be performed by iothread which should
> >>> be notified and waked up.
> >>
> >> I still don't understand.  If you're getting EXCP_INTERRUPT it means:
> >>
> >> 1) that cpu_signal was called
> >
> > No, it isn't. That is the branch when icount is expired.
> > And when it is expired in replay mode we have to wake up iothread,
> > because nobody will care about this.
> 
> Then it's done here in qemu_tcg_cpu_thread_fn:

Do you mean that I should put iothread notification right here?
Or that this code duplicates my patch?
If the second one then I guess that it doesn't help and I need to make additional checks about it.

> 
>         if (use_icount) {
>             int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
> 
>             if (deadline == 0) {
>                 qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>             }
>         }
> 
> If you need to move the 4 lines inside the if elsewhere, that I guess it's okay.

Pavel Dovgalyuk
Paolo Bonzini Feb. 16, 2015, 1:53 p.m. UTC | #11
On 16/02/2015 14:37, Pavel Dovgaluk wrote:
>> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
>> On 16/02/2015 14:27, Pavel Dovgaluk wrote:
>>>> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
>>>> On 16/02/2015 13:26, Pavel Dovgaluk wrote:
>>>>>>>>> I think in this case there are no events at all - just reading timers values
>>>>>>>>> that were made while recording.
>>>>>>>>> We have to replay these reads by waking iothread.
>>>>>>>
>>>>>>> I think the right place for this is in replay_read_next_clock then.
>>>>> It doesn't fit. Log file is not read until all instructions are executed.
>>>>> And the next read from the file should be performed by iothread which should
>>>>> be notified and waked up.
>>>>
>>>> I still don't understand.  If you're getting EXCP_INTERRUPT it means:
>>>>
>>>> 1) that cpu_signal was called
>>>
>>> No, it isn't. That is the branch when icount is expired.
>>> And when it is expired in replay mode we have to wake up iothread,
>>> because nobody will care about this.
>>
>> Then it's done here in qemu_tcg_cpu_thread_fn:
> 
> Do you mean that I should put iothread notification right here?

It already notifies the iothread.

> Or that this code duplicates my patch?
> If the second one then I guess that it doesn't help and I need to make additional checks about it.

Yes.  You can modify your patch to do:

     int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
     if (deadline == 0) {
         qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
     }

instead of qemu_notify_event(), and remove these lines from
qemu_tcg_cpu_thread_fn.

Paolo

>>
>>         if (use_icount) {
>>             int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
>>
>>             if (deadline == 0) {
>>                 qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>>             }
>>         }
>>
>> If you need to move the 4 lines inside the if elsewhere, that I guess it's okay.
> 
> Pavel Dovgalyuk
> 
> 
>
Pavel Dovgalyuk Feb. 17, 2015, 8:43 a.m. UTC | #12
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> On 16/02/2015 14:37, Pavel Dovgaluk wrote:
> >> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> >> On 16/02/2015 14:27, Pavel Dovgaluk wrote:
> >>>> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> >>>> On 16/02/2015 13:26, Pavel Dovgaluk wrote:
> >>>>>>>>> I think in this case there are no events at all - just reading timers values
> >>>>>>>>> that were made while recording.
> >>>>>>>>> We have to replay these reads by waking iothread.
> >>>>>>>
> >>>>>>> I think the right place for this is in replay_read_next_clock then.
> >>>>> It doesn't fit. Log file is not read until all instructions are executed.
> >>>>> And the next read from the file should be performed by iothread which should
> >>>>> be notified and waked up.
> >>>>
> >>>> I still don't understand.  If you're getting EXCP_INTERRUPT it means:
> >>>>
> >>>> 1) that cpu_signal was called
> >>>
> >>> No, it isn't. That is the branch when icount is expired.
> >>> And when it is expired in replay mode we have to wake up iothread,
> >>> because nobody will care about this.
> >>
> >> Then it's done here in qemu_tcg_cpu_thread_fn:
> >
> > Do you mean that I should put iothread notification right here?
> 
> It already notifies the iothread.
> 
> > Or that this code duplicates my patch?
> > If the second one then I guess that it doesn't help and I need to make additional checks
> about it.
> 
> Yes.  You can modify your patch to do:
> 
>      int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
>      if (deadline == 0) {
>          qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>      }
> 
> instead of qemu_notify_event(), and remove these lines from
> qemu_tcg_cpu_thread_fn.

I tried this one. But there is one problem.
Expiring of the virtual timers is not the only reason of icount expiration in replay mode.
It may be caused by host timers deadline or poll timeout in record mode. In this case 
qemu_clock_notify(QEMU_CLOCK_VIRTUAL) will not be called in replay mode and we'll waste time for iothread sleeping.


Pavel Dovgalyuk
Paolo Bonzini Feb. 17, 2015, 10:58 a.m. UTC | #13
On 17/02/2015 09:43, Pavel Dovgaluk wrote:
>> >      int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
>> >      if (deadline == 0) {
>> >          qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>> >      }
>> > 
>> > instead of qemu_notify_event(), and remove these lines from
>> > qemu_tcg_cpu_thread_fn.
> I tried this one. But there is one problem.
> Expiring of the virtual timers is not the only reason of icount expiration in replay mode.
> It may be caused by host timers deadline or poll timeout in record mode. In this case 
> qemu_clock_notify(QEMU_CLOCK_VIRTUAL) will not be called in replay mode and we'll waste time for iothread sleeping.

Sure, but unconditional qemu_notify_event() is also wrong.  So, to sum up:

- it's okay to move code from qemu_tcg_cpu_thread_fn to cpu-exec.c

- it's okay to add more qemu_clock_notify calls than just
qemu_clock_notify(QEMU_CLOCK_VIRTUAL), each with its own condition

- it's better if all these, after being moved to cpu-exec.c, are also
extracted in a separate function

- it's not okay to do an unconditional qemu_notify_event() in
cpu-exec.c, even if it's under "if (replay_mode != NONE)".

Thanks for your understanding! :)

Paolo
Pavel Dovgalyuk Feb. 17, 2015, 11:35 a.m. UTC | #14
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 17/02/2015 09:43, Pavel Dovgaluk wrote:
> >> >      int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
> >> >      if (deadline == 0) {
> >> >          qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
> >> >      }
> >> >
> >> > instead of qemu_notify_event(), and remove these lines from
> >> > qemu_tcg_cpu_thread_fn.
> > I tried this one. But there is one problem.
> > Expiring of the virtual timers is not the only reason of icount expiration in replay mode.
> > It may be caused by host timers deadline or poll timeout in record mode. In this case
> > qemu_clock_notify(QEMU_CLOCK_VIRTUAL) will not be called in replay mode and we'll waste time
> for iothread sleeping.
> 
> Sure, but unconditional qemu_notify_event() is also wrong.  So, to sum up:
> 
> - it's okay to move code from qemu_tcg_cpu_thread_fn to cpu-exec.c
> 
> - it's okay to add more qemu_clock_notify calls than just
> qemu_clock_notify(QEMU_CLOCK_VIRTUAL), each with its own condition
> 
> - it's better if all these, after being moved to cpu-exec.c, are also
> extracted in a separate function
> 
> - it's not okay to do an unconditional qemu_notify_event() in
> cpu-exec.c, even if it's under "if (replay_mode != NONE)".

How can I wake up iothread if there are no pending timers?
deadline will (almost) never become zero in my case, because there is 
another kind of event in the log (not the timer one).
Should I fetch the event and call qemu_notify_event() from replay module?

> 
> Thanks for your understanding! :)

Pavel Dovgalyuk
Paolo Bonzini Feb. 17, 2015, 12:21 p.m. UTC | #15
On 17/02/2015 12:35, Pavel Dovgaluk wrote:
> How can I wake up iothread if there are no pending timers?
> deadline will (almost) never become zero in my case, because there is 
> another kind of event in the log (not the timer one).
> Should I fetch the event and call qemu_notify_event() from replay module?

Yes, a few mails ago I suggested that (maybe I got the wrong function in
the replay module).

Paolo
diff mbox

Patch

diff --git a/cpu-exec.c b/cpu-exec.c
index 49f01f5..99a0993 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -531,6 +531,7 @@  int cpu_exec(CPUArchState *env)
                             }
                             cpu->exception_index = EXCP_INTERRUPT;
                             next_tb = 0;
+                            qemu_notify_event();
                             cpu_loop_exit(cpu);
                         }
                         break;
diff --git a/cpus.c b/cpus.c
index c513275..8787277 100644
--- a/cpus.c
+++ b/cpus.c
@@ -41,6 +41,7 @@ 
 #include "qemu/seqlock.h"
 #include "qapi-event.h"
 #include "hw/nmi.h"
+#include "replay/replay.h"
 
 #ifndef _WIN32
 #include "qemu/compatfd.h"
@@ -1342,18 +1343,22 @@  static int tcg_cpu_exec(CPUArchState *env)
                                     + cpu->icount_extra);
         cpu->icount_decr.u16.low = 0;
         cpu->icount_extra = 0;
-        deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
+        if (replay_mode != REPLAY_MODE_PLAY) {
+            deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
 
-        /* Maintain prior (possibly buggy) behaviour where if no deadline
-         * was set (as there is no QEMU_CLOCK_VIRTUAL timer) or it is more than
-         * INT32_MAX nanoseconds ahead, we still use INT32_MAX
-         * nanoseconds.
-         */
-        if ((deadline < 0) || (deadline > INT32_MAX)) {
-            deadline = INT32_MAX;
-        }
+            /* Maintain prior (possibly buggy) behaviour where if no deadline
+             * was set (as there is no QEMU_CLOCK_VIRTUAL timer) or it is more than
+             * INT32_MAX nanoseconds ahead, we still use INT32_MAX
+             * nanoseconds.
+             */
+            if ((deadline < 0) || (deadline > INT32_MAX)) {
+                deadline = INT32_MAX;
+            }
 
-        count = qemu_icount_round(deadline);
+            count = qemu_icount_round(deadline);
+        } else {
+            count = replay_get_instructions();
+        }
         timers_state.qemu_icount += count;
         decr = (count > 0xffff) ? 0xffff : count;
         count -= decr;
@@ -1371,6 +1376,9 @@  static int tcg_cpu_exec(CPUArchState *env)
                         + cpu->icount_extra);
         cpu->icount_decr.u32 = 0;
         cpu->icount_extra = 0;
+        if (replay_mode == REPLAY_MODE_PLAY) {
+            replay_exec_instructions();
+        }
     }
     return ret;
 }
diff --git a/replay/replay.c b/replay/replay.c
index a43bbbc..d6f5c4b 100755
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -58,3 +58,27 @@  uint64_t replay_get_current_step(void)
 {
     return cpu_get_icount_raw();
 }
+
+int replay_get_instructions(void)
+{
+    int res = 0;
+    replay_mutex_lock();
+    if (skip_async_events(EVENT_INSTRUCTION)) {
+        res = replay_state.instructions_count;
+    }
+    replay_mutex_unlock();
+    return res;
+}
+
+void replay_exec_instructions(void)
+{
+    if (replay_state.instructions_count > 0) {
+        int count = (int)(replay_get_current_step()
+                          - replay_state.current_step);
+        replay_state.instructions_count -= count;
+        replay_state.current_step += count;
+        if (replay_state.instructions_count == 0 && count != 0) {
+            replay_has_unread_data = 0;
+        }
+    }
+}
diff --git a/replay/replay.h b/replay/replay.h
index a03c748..e425dea 100755
--- a/replay/replay.h
+++ b/replay/replay.h
@@ -22,5 +22,9 @@  extern ReplayMode replay_mode;
 
 /*! Returns number of executed instructions. */
 uint64_t replay_get_current_step(void);
+/*! Returns number of instructions to execute in replay mode. */
+int replay_get_instructions(void);
+/*! Updates instructions counter in replay mode. */
+void replay_exec_instructions(void);
 
 #endif