[PATCH] replay: notify CPU on event

[PATCH] replay: notify CPU on event

[Pavel Dovgalyuk]

This patch enables vCPU notification to wake it up
when new async event comes in replay mode.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
---
 replay/replay-events.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/replay/replay-events.c b/replay/replay-events.c
index a1c6bb934e..92dc800219 100644
--- a/replay/replay-events.c
+++ b/replay/replay-events.c
@@ -126,6 +126,7 @@ void replay_add_event(ReplayAsyncEventKind event_kind,
 
     g_assert(replay_mutex_locked());
     QTAILQ_INSERT_TAIL(&events_list, event, events);
+    qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
 }
 
 void replay_bh_schedule_event(QEMUBH *bh)

Re: [PATCH] replay: notify CPU on event

[Paolo Bonzini]

On 15/02/21 13:58, Pavel Dovgalyuk wrote:
> This patch enables vCPU notification to wake it up
> when new async event comes in replay mode.
> 
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> ---
>   replay/replay-events.c |    1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/replay/replay-events.c b/replay/replay-events.c
> index a1c6bb934e..92dc800219 100644
> --- a/replay/replay-events.c
> +++ b/replay/replay-events.c
> @@ -126,6 +126,7 @@ void replay_add_event(ReplayAsyncEventKind event_kind,
>   
>       g_assert(replay_mutex_locked());
>       QTAILQ_INSERT_TAIL(&events_list, event, events);
> +    qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>   }
>   
>   void replay_bh_schedule_event(QEMUBH *bh)
> 

Do you really want to notify the vCPU, or rather the main loop (which 
will run as a side effect of the lockstep behavior that RR puts in place)?

The reason I doubt you need to notify the vCPU, is that I'm not sure why 
an incoming event would cause you to recalculate the QEMU_CLOCK_VIRTUAL 
deadline.  Rather, perhaps the problem is that a bottom half cannot be 
run right away?  And if so, probably the issue only happens for a 
running vCPU (not a sleeping one) so you only need 
qemu_cpu_kick(current_cpu)?

Either way, your commit message does not explain why it is needed and 
how event are missed or delayed without the patch.  This is especially 
important for something like RR, which is pretty invasive and understood 
only by very few people (I don't put myself in the group, in fact).

Thanks,

Paolo

Re: [PATCH] replay: notify CPU on event

[Pavel Dovgalyuk]

On 15.02.2021 16:29, Paolo Bonzini wrote:
> On 15/02/21 13:58, Pavel Dovgalyuk wrote:
>> This patch enables vCPU notification to wake it up
>> when new async event comes in replay mode.
>>
>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
>> ---
>>   replay/replay-events.c |    1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/replay/replay-events.c b/replay/replay-events.c
>> index a1c6bb934e..92dc800219 100644
>> --- a/replay/replay-events.c
>> +++ b/replay/replay-events.c
>> @@ -126,6 +126,7 @@ void replay_add_event(ReplayAsyncEventKind 
>> event_kind,
>>       g_assert(replay_mutex_locked());
>>       QTAILQ_INSERT_TAIL(&events_list, event, events);
>> +    qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>>   }
>>   void replay_bh_schedule_event(QEMUBH *bh)
>>
> 
> Do you really want to notify the vCPU, or rather the main loop (which 
> will run as a side effect of the lockstep behavior that RR puts in place)?
> 
> The reason I doubt you need to notify the vCPU, is that I'm not sure why 
> an incoming event would cause you to recalculate the QEMU_CLOCK_VIRTUAL 
> deadline.  Rather, perhaps the problem is that a bottom half cannot be 
> run right away?  And if so, probably the issue only happens for a 
> running vCPU (not a sleeping one) so you only need 
> qemu_cpu_kick(current_cpu)?

I've got the following issue:
vCPU thread is sleeping in rr_wait_io_event.
The next event is block async callback linked with 
CHECKPOINT_CLOCK_WARP_ACCOUNT.
Therefore this event should be processed by vCPU, which is sleeping.

> 
> Either way, your commit message does not explain why it is needed and 
> how event are missed or delayed without the patch.  This is especially 
> important for something like RR, which is pretty invasive and understood 
> only by very few people (I don't put myself in the group, in fact).

Ok, I'll update the comment.

Pavel Dovgalyuk