* Re: [PATCH] replay: improve determinism of virtio-net
2021-05-17 13:04 [PATCH] replay: improve determinism of virtio-net Pavel Dovgalyuk
@ 2021-05-31 4:46 ` Pavel Dovgalyuk
2021-05-31 4:55 ` Jason Wang
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Pavel Dovgalyuk @ 2021-05-31 4:46 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, jasowang, alex.bennee, mst
ping
On 17.05.2021 16:04, Pavel Dovgalyuk wrote:
> virtio-net device uses bottom halves for callbacks.
> These callbacks should be deterministic, because they affect VM state.
> This patch replaces BH invocations with corresponding replay functions,
> making them deterministic in record/replay mode.
> This patch also disables guest announce timers for record/replay,
> because they break correct loadvm in deterministic mode.
>
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> ---
> hw/net/virtio-net.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 6b7e8dd04e..e876363236 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -44,6 +44,7 @@
> #include "hw/pci/pci.h"
> #include "net_rx_pkt.h"
> #include "hw/virtio/vhost.h"
> +#include "sysemu/replay.h"
>
> #define VIRTIO_NET_VM_VERSION 11
>
> @@ -394,7 +395,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> timer_mod(q->tx_timer,
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + n->tx_timeout);
> } else {
> - qemu_bh_schedule(q->tx_bh);
> + replay_bh_schedule_event(q->tx_bh);
> }
> } else {
> if (q->tx_timer) {
> @@ -2546,7 +2547,7 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
> return;
> }
> virtio_queue_set_notification(vq, 0);
> - qemu_bh_schedule(q->tx_bh);
> + replay_bh_schedule_event(q->tx_bh);
> }
>
> static void virtio_net_tx_timer(void *opaque)
> @@ -2602,7 +2603,7 @@ static void virtio_net_tx_bh(void *opaque)
> /* If we flush a full burst of packets, assume there are
> * more coming and immediately reschedule */
> if (ret >= n->tx_burst) {
> - qemu_bh_schedule(q->tx_bh);
> + replay_bh_schedule_event(q->tx_bh);
> q->tx_waiting = 1;
> return;
> }
> @@ -2616,7 +2617,7 @@ static void virtio_net_tx_bh(void *opaque)
> return;
> } else if (ret > 0) {
> virtio_queue_set_notification(q->tx_vq, 0);
> - qemu_bh_schedule(q->tx_bh);
> + replay_bh_schedule_event(q->tx_bh);
> q->tx_waiting = 1;
> }
> }
> @@ -3206,6 +3207,10 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
> n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
> }
>
> + if (replay_mode != REPLAY_MODE_NONE) {
> + n->host_features &= ~(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE);
> + }
> +
> if (n->net_conf.duplex_str) {
> if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
> n->net_conf.duplex = DUPLEX_HALF;
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] replay: improve determinism of virtio-net
2021-05-17 13:04 [PATCH] replay: improve determinism of virtio-net Pavel Dovgalyuk
2021-05-31 4:46 ` Pavel Dovgalyuk
@ 2021-05-31 4:55 ` Jason Wang
2021-05-31 6:35 ` Pavel Dovgalyuk
2021-07-02 15:11 ` Michael S. Tsirkin
2021-07-02 15:22 ` Philippe Mathieu-Daudé
3 siblings, 1 reply; 11+ messages in thread
From: Jason Wang @ 2021-05-31 4:55 UTC (permalink / raw)
To: Pavel Dovgalyuk, qemu-devel; +Cc: pbonzini, alex.bennee, mst
在 2021/5/17 下午9:04, Pavel Dovgalyuk 写道:
> virtio-net device uses bottom halves for callbacks.
> These callbacks should be deterministic, because they affect VM state.
> This patch replaces BH invocations with corresponding replay functions,
> making them deterministic in record/replay mode.
> This patch also disables guest announce timers for record/replay,
> because they break correct loadvm in deterministic mode.
Virtio-net can be configured to work in tx timer mode. Do we need to
care about that?
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> ---
> hw/net/virtio-net.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 6b7e8dd04e..e876363236 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -44,6 +44,7 @@
> #include "hw/pci/pci.h"
> #include "net_rx_pkt.h"
> #include "hw/virtio/vhost.h"
> +#include "sysemu/replay.h"
>
> #define VIRTIO_NET_VM_VERSION 11
>
> @@ -394,7 +395,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> timer_mod(q->tx_timer,
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + n->tx_timeout);
> } else {
> - qemu_bh_schedule(q->tx_bh);
> + replay_bh_schedule_event(q->tx_bh);
> }
> } else {
> if (q->tx_timer) {
> @@ -2546,7 +2547,7 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
> return;
> }
> virtio_queue_set_notification(vq, 0);
> - qemu_bh_schedule(q->tx_bh);
> + replay_bh_schedule_event(q->tx_bh);
Not familiar with replay but any chance to change qemu_bh_schedule()
instead?
Thanks
> }
>
> static void virtio_net_tx_timer(void *opaque)
> @@ -2602,7 +2603,7 @@ static void virtio_net_tx_bh(void *opaque)
> /* If we flush a full burst of packets, assume there are
> * more coming and immediately reschedule */
> if (ret >= n->tx_burst) {
> - qemu_bh_schedule(q->tx_bh);
> + replay_bh_schedule_event(q->tx_bh);
> q->tx_waiting = 1;
> return;
> }
> @@ -2616,7 +2617,7 @@ static void virtio_net_tx_bh(void *opaque)
> return;
> } else if (ret > 0) {
> virtio_queue_set_notification(q->tx_vq, 0);
> - qemu_bh_schedule(q->tx_bh);
> + replay_bh_schedule_event(q->tx_bh);
> q->tx_waiting = 1;
> }
> }
> @@ -3206,6 +3207,10 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
> n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
> }
>
> + if (replay_mode != REPLAY_MODE_NONE) {
> + n->host_features &= ~(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE);
> + }
> +
> if (n->net_conf.duplex_str) {
> if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
> n->net_conf.duplex = DUPLEX_HALF;
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] replay: improve determinism of virtio-net
2021-05-31 4:55 ` Jason Wang
@ 2021-05-31 6:35 ` Pavel Dovgalyuk
2021-05-31 6:39 ` Jason Wang
2021-10-20 13:40 ` Alex Bennée
0 siblings, 2 replies; 11+ messages in thread
From: Pavel Dovgalyuk @ 2021-05-31 6:35 UTC (permalink / raw)
To: Jason Wang, qemu-devel; +Cc: pbonzini, alex.bennee, mst
On 31.05.2021 07:55, Jason Wang wrote:
>
> 在 2021/5/17 下午9:04, Pavel Dovgalyuk 写道:
>> virtio-net device uses bottom halves for callbacks.
>> These callbacks should be deterministic, because they affect VM state.
>> This patch replaces BH invocations with corresponding replay functions,
>> making them deterministic in record/replay mode.
>> This patch also disables guest announce timers for record/replay,
>> because they break correct loadvm in deterministic mode.
>
>
> Virtio-net can be configured to work in tx timer mode. Do we need to
> care about that?
What does it mean? This patch fixes interaction with TX timer. Is it
related to that mode?
>
>
>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
>> ---
>> hw/net/virtio-net.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 6b7e8dd04e..e876363236 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -44,6 +44,7 @@
>> #include "hw/pci/pci.h"
>> #include "net_rx_pkt.h"
>> #include "hw/virtio/vhost.h"
>> +#include "sysemu/replay.h"
>> #define VIRTIO_NET_VM_VERSION 11
>> @@ -394,7 +395,7 @@ static void virtio_net_set_status(struct
>> VirtIODevice *vdev, uint8_t status)
>> timer_mod(q->tx_timer,
>> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
>> + n->tx_timeout);
>> } else {
>> - qemu_bh_schedule(q->tx_bh);
>> + replay_bh_schedule_event(q->tx_bh);
>> }
>> } else {
>> if (q->tx_timer) {
>> @@ -2546,7 +2547,7 @@ static void virtio_net_handle_tx_bh(VirtIODevice
>> *vdev, VirtQueue *vq)
>> return;
>> }
>> virtio_queue_set_notification(vq, 0);
>> - qemu_bh_schedule(q->tx_bh);
>> + replay_bh_schedule_event(q->tx_bh);
>
>
> Not familiar with replay but any chance to change qemu_bh_schedule()
> instead?
It would be better, but sometimes qemu_bh_schedule is used for the
callbacks that are not related to the guest state change.
>
> Thanks
>
>
>> }
>> static void virtio_net_tx_timer(void *opaque)
>> @@ -2602,7 +2603,7 @@ static void virtio_net_tx_bh(void *opaque)
>> /* If we flush a full burst of packets, assume there are
>> * more coming and immediately reschedule */
>> if (ret >= n->tx_burst) {
>> - qemu_bh_schedule(q->tx_bh);
>> + replay_bh_schedule_event(q->tx_bh);
>> q->tx_waiting = 1;
>> return;
>> }
>> @@ -2616,7 +2617,7 @@ static void virtio_net_tx_bh(void *opaque)
>> return;
>> } else if (ret > 0) {
>> virtio_queue_set_notification(q->tx_vq, 0);
>> - qemu_bh_schedule(q->tx_bh);
>> + replay_bh_schedule_event(q->tx_bh);
>> q->tx_waiting = 1;
>> }
>> }
>> @@ -3206,6 +3207,10 @@ static void
>> virtio_net_device_realize(DeviceState *dev, Error **errp)
>> n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
>> }
>> + if (replay_mode != REPLAY_MODE_NONE) {
>> + n->host_features &= ~(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE);
>> + }
>> +
>> if (n->net_conf.duplex_str) {
>> if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
>> n->net_conf.duplex = DUPLEX_HALF;
>>
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] replay: improve determinism of virtio-net
2021-05-31 6:35 ` Pavel Dovgalyuk
@ 2021-05-31 6:39 ` Jason Wang
2021-05-31 8:47 ` Pavel Dovgalyuk
2021-06-01 10:33 ` Pavel Dovgalyuk
2021-10-20 13:40 ` Alex Bennée
1 sibling, 2 replies; 11+ messages in thread
From: Jason Wang @ 2021-05-31 6:39 UTC (permalink / raw)
To: Pavel Dovgalyuk, qemu-devel; +Cc: pbonzini, alex.bennee, mst
在 2021/5/31 下午2:35, Pavel Dovgalyuk 写道:
> On 31.05.2021 07:55, Jason Wang wrote:
>>
>> 在 2021/5/17 下午9:04, Pavel Dovgalyuk 写道:
>>> virtio-net device uses bottom halves for callbacks.
>>> These callbacks should be deterministic, because they affect VM state.
>>> This patch replaces BH invocations with corresponding replay functions,
>>> making them deterministic in record/replay mode.
>>> This patch also disables guest announce timers for record/replay,
>>> because they break correct loadvm in deterministic mode.
>>
>>
>> Virtio-net can be configured to work in tx timer mode. Do we need to
>> care about that?
>
> What does it mean? This patch fixes interaction with TX timer. Is it
> related to that mode?
I meant is the timer used by virtio_net_handle_tx_timer() safe consider
you disable announce timer.
Thanks
>
>>
>>
>>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
>>> ---
>>> hw/net/virtio-net.c | 13 +++++++++----
>>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 6b7e8dd04e..e876363236 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -44,6 +44,7 @@
>>> #include "hw/pci/pci.h"
>>> #include "net_rx_pkt.h"
>>> #include "hw/virtio/vhost.h"
>>> +#include "sysemu/replay.h"
>>> #define VIRTIO_NET_VM_VERSION 11
>>> @@ -394,7 +395,7 @@ static void virtio_net_set_status(struct
>>> VirtIODevice *vdev, uint8_t status)
>>> timer_mod(q->tx_timer,
>>> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + n->tx_timeout);
>>> } else {
>>> - qemu_bh_schedule(q->tx_bh);
>>> + replay_bh_schedule_event(q->tx_bh);
>>> }
>>> } else {
>>> if (q->tx_timer) {
>>> @@ -2546,7 +2547,7 @@ static void
>>> virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
>>> return;
>>> }
>>> virtio_queue_set_notification(vq, 0);
>>> - qemu_bh_schedule(q->tx_bh);
>>> + replay_bh_schedule_event(q->tx_bh);
>>
>>
>> Not familiar with replay but any chance to change qemu_bh_schedule()
>> instead?
>
> It would be better, but sometimes qemu_bh_schedule is used for the
> callbacks that are not related to the guest state change.
>
>>
>> Thanks
>>
>>
>>> }
>>> static void virtio_net_tx_timer(void *opaque)
>>> @@ -2602,7 +2603,7 @@ static void virtio_net_tx_bh(void *opaque)
>>> /* If we flush a full burst of packets, assume there are
>>> * more coming and immediately reschedule */
>>> if (ret >= n->tx_burst) {
>>> - qemu_bh_schedule(q->tx_bh);
>>> + replay_bh_schedule_event(q->tx_bh);
>>> q->tx_waiting = 1;
>>> return;
>>> }
>>> @@ -2616,7 +2617,7 @@ static void virtio_net_tx_bh(void *opaque)
>>> return;
>>> } else if (ret > 0) {
>>> virtio_queue_set_notification(q->tx_vq, 0);
>>> - qemu_bh_schedule(q->tx_bh);
>>> + replay_bh_schedule_event(q->tx_bh);
>>> q->tx_waiting = 1;
>>> }
>>> }
>>> @@ -3206,6 +3207,10 @@ static void
>>> virtio_net_device_realize(DeviceState *dev, Error **errp)
>>> n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
>>> }
>>> + if (replay_mode != REPLAY_MODE_NONE) {
>>> + n->host_features &= ~(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE);
>>> + }
>>> +
>>> if (n->net_conf.duplex_str) {
>>> if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
>>> n->net_conf.duplex = DUPLEX_HALF;
>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] replay: improve determinism of virtio-net
2021-05-31 6:39 ` Jason Wang
@ 2021-05-31 8:47 ` Pavel Dovgalyuk
2021-06-01 10:33 ` Pavel Dovgalyuk
1 sibling, 0 replies; 11+ messages in thread
From: Pavel Dovgalyuk @ 2021-05-31 8:47 UTC (permalink / raw)
To: Jason Wang, qemu-devel; +Cc: pbonzini, alex.bennee, mst
On 31.05.2021 09:39, Jason Wang wrote:
>
> 在 2021/5/31 下午2:35, Pavel Dovgalyuk 写道:
>> On 31.05.2021 07:55, Jason Wang wrote:
>>>
>>> 在 2021/5/17 下午9:04, Pavel Dovgalyuk 写道:
>>>> virtio-net device uses bottom halves for callbacks.
>>>> These callbacks should be deterministic, because they affect VM state.
>>>> This patch replaces BH invocations with corresponding replay functions,
>>>> making them deterministic in record/replay mode.
>>>> This patch also disables guest announce timers for record/replay,
>>>> because they break correct loadvm in deterministic mode.
>>>
>>>
>>> Virtio-net can be configured to work in tx timer mode. Do we need to
>>> care about that?
>>
>> What does it mean? This patch fixes interaction with TX timer. Is it
>> related to that mode?
>
>
> I meant is the timer used by virtio_net_handle_tx_timer() safe consider
> you disable announce timer.
I'm not sure that tx_timer is ok. It uses virtual time, but is not saved
in vmstate.
>
> Thanks
>
>
>>
>>>
>>>
>>>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
>>>> ---
>>>> hw/net/virtio-net.c | 13 +++++++++----
>>>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 6b7e8dd04e..e876363236 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -44,6 +44,7 @@
>>>> #include "hw/pci/pci.h"
>>>> #include "net_rx_pkt.h"
>>>> #include "hw/virtio/vhost.h"
>>>> +#include "sysemu/replay.h"
>>>> #define VIRTIO_NET_VM_VERSION 11
>>>> @@ -394,7 +395,7 @@ static void virtio_net_set_status(struct
>>>> VirtIODevice *vdev, uint8_t status)
>>>> timer_mod(q->tx_timer,
>>>> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + n->tx_timeout);
>>>> } else {
>>>> - qemu_bh_schedule(q->tx_bh);
>>>> + replay_bh_schedule_event(q->tx_bh);
>>>> }
>>>> } else {
>>>> if (q->tx_timer) {
>>>> @@ -2546,7 +2547,7 @@ static void
>>>> virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
>>>> return;
>>>> }
>>>> virtio_queue_set_notification(vq, 0);
>>>> - qemu_bh_schedule(q->tx_bh);
>>>> + replay_bh_schedule_event(q->tx_bh);
>>>
>>>
>>> Not familiar with replay but any chance to change qemu_bh_schedule()
>>> instead?
>>
>> It would be better, but sometimes qemu_bh_schedule is used for the
>> callbacks that are not related to the guest state change.
>>
>>>
>>> Thanks
>>>
>>>
>>>> }
>>>> static void virtio_net_tx_timer(void *opaque)
>>>> @@ -2602,7 +2603,7 @@ static void virtio_net_tx_bh(void *opaque)
>>>> /* If we flush a full burst of packets, assume there are
>>>> * more coming and immediately reschedule */
>>>> if (ret >= n->tx_burst) {
>>>> - qemu_bh_schedule(q->tx_bh);
>>>> + replay_bh_schedule_event(q->tx_bh);
>>>> q->tx_waiting = 1;
>>>> return;
>>>> }
>>>> @@ -2616,7 +2617,7 @@ static void virtio_net_tx_bh(void *opaque)
>>>> return;
>>>> } else if (ret > 0) {
>>>> virtio_queue_set_notification(q->tx_vq, 0);
>>>> - qemu_bh_schedule(q->tx_bh);
>>>> + replay_bh_schedule_event(q->tx_bh);
>>>> q->tx_waiting = 1;
>>>> }
>>>> }
>>>> @@ -3206,6 +3207,10 @@ static void
>>>> virtio_net_device_realize(DeviceState *dev, Error **errp)
>>>> n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
>>>> }
>>>> + if (replay_mode != REPLAY_MODE_NONE) {
>>>> + n->host_features &= ~(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE);
>>>> + }
>>>> +
>>>> if (n->net_conf.duplex_str) {
>>>> if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
>>>> n->net_conf.duplex = DUPLEX_HALF;
>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] replay: improve determinism of virtio-net
2021-05-31 6:39 ` Jason Wang
2021-05-31 8:47 ` Pavel Dovgalyuk
@ 2021-06-01 10:33 ` Pavel Dovgalyuk
1 sibling, 0 replies; 11+ messages in thread
From: Pavel Dovgalyuk @ 2021-06-01 10:33 UTC (permalink / raw)
To: Jason Wang, qemu-devel; +Cc: pbonzini, alex.bennee, mst
On 31.05.2021 09:39, Jason Wang wrote:
>
> 在 2021/5/31 下午2:35, Pavel Dovgalyuk 写道:
>> On 31.05.2021 07:55, Jason Wang wrote:
>>>
>>> 在 2021/5/17 下午9:04, Pavel Dovgalyuk 写道:
>>>> virtio-net device uses bottom halves for callbacks.
>>>> These callbacks should be deterministic, because they affect VM state.
>>>> This patch replaces BH invocations with corresponding replay functions,
>>>> making them deterministic in record/replay mode.
>>>> This patch also disables guest announce timers for record/replay,
>>>> because they break correct loadvm in deterministic mode.
>>>
>>>
>>> Virtio-net can be configured to work in tx timer mode. Do we need to
>>> care about that?
>>
>> What does it mean? This patch fixes interaction with TX timer. Is it
>> related to that mode?
>
>
> I meant is the timer used by virtio_net_handle_tx_timer() safe consider
> you disable announce timer.
It is related to virtio queue. I looked through it and it looks safe to me.
>
> Thanks
>
>
>>
>>>
>>>
>>>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
>>>> ---
>>>> hw/net/virtio-net.c | 13 +++++++++----
>>>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 6b7e8dd04e..e876363236 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -44,6 +44,7 @@
>>>> #include "hw/pci/pci.h"
>>>> #include "net_rx_pkt.h"
>>>> #include "hw/virtio/vhost.h"
>>>> +#include "sysemu/replay.h"
>>>> #define VIRTIO_NET_VM_VERSION 11
>>>> @@ -394,7 +395,7 @@ static void virtio_net_set_status(struct
>>>> VirtIODevice *vdev, uint8_t status)
>>>> timer_mod(q->tx_timer,
>>>> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + n->tx_timeout);
>>>> } else {
>>>> - qemu_bh_schedule(q->tx_bh);
>>>> + replay_bh_schedule_event(q->tx_bh);
>>>> }
>>>> } else {
>>>> if (q->tx_timer) {
>>>> @@ -2546,7 +2547,7 @@ static void
>>>> virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
>>>> return;
>>>> }
>>>> virtio_queue_set_notification(vq, 0);
>>>> - qemu_bh_schedule(q->tx_bh);
>>>> + replay_bh_schedule_event(q->tx_bh);
>>>
>>>
>>> Not familiar with replay but any chance to change qemu_bh_schedule()
>>> instead?
>>
>> It would be better, but sometimes qemu_bh_schedule is used for the
>> callbacks that are not related to the guest state change.
>>
>>>
>>> Thanks
>>>
>>>
>>>> }
>>>> static void virtio_net_tx_timer(void *opaque)
>>>> @@ -2602,7 +2603,7 @@ static void virtio_net_tx_bh(void *opaque)
>>>> /* If we flush a full burst of packets, assume there are
>>>> * more coming and immediately reschedule */
>>>> if (ret >= n->tx_burst) {
>>>> - qemu_bh_schedule(q->tx_bh);
>>>> + replay_bh_schedule_event(q->tx_bh);
>>>> q->tx_waiting = 1;
>>>> return;
>>>> }
>>>> @@ -2616,7 +2617,7 @@ static void virtio_net_tx_bh(void *opaque)
>>>> return;
>>>> } else if (ret > 0) {
>>>> virtio_queue_set_notification(q->tx_vq, 0);
>>>> - qemu_bh_schedule(q->tx_bh);
>>>> + replay_bh_schedule_event(q->tx_bh);
>>>> q->tx_waiting = 1;
>>>> }
>>>> }
>>>> @@ -3206,6 +3207,10 @@ static void
>>>> virtio_net_device_realize(DeviceState *dev, Error **errp)
>>>> n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
>>>> }
>>>> + if (replay_mode != REPLAY_MODE_NONE) {
>>>> + n->host_features &= ~(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE);
>>>> + }
>>>> +
>>>> if (n->net_conf.duplex_str) {
>>>> if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
>>>> n->net_conf.duplex = DUPLEX_HALF;
>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] replay: improve determinism of virtio-net
2021-05-31 6:35 ` Pavel Dovgalyuk
2021-05-31 6:39 ` Jason Wang
@ 2021-10-20 13:40 ` Alex Bennée
2021-10-20 14:50 ` Michael S. Tsirkin
1 sibling, 1 reply; 11+ messages in thread
From: Alex Bennée @ 2021-10-20 13:40 UTC (permalink / raw)
To: Pavel Dovgalyuk; +Cc: pbonzini, Jason Wang, qemu-devel, mst
Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> writes:
> On 31.05.2021 07:55, Jason Wang wrote:
>> 在 2021/5/17 下午9:04, Pavel Dovgalyuk 写道:
<snip>
>>> @@ -2546,7 +2547,7 @@ static void
>>> virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
>>> return;
>>> }
>>> virtio_queue_set_notification(vq, 0);
>>> - qemu_bh_schedule(q->tx_bh);
>>> + replay_bh_schedule_event(q->tx_bh);
>> Not familiar with replay but any chance to change qemu_bh_schedule()
>> instead?
>
> It would be better, but sometimes qemu_bh_schedule is used for the
> callbacks that are not related to the guest state change.
Maybe that indicates we should expand the API:
void qemu_bh_schedule(QEMUBH *bh, bool guest_state);
or maybe explicit functions:
void qemu_bh_schedule(QEMUBH *bh);
void qemu_bh_schedule_guest_update(QEMUBH *bh);
And document the cases with proper prototypes in main-loop.h.
While I was poking around I also saw qemu_bh_schedule_idle which made me
wonder what happens if a guest change is triggered by this. Would this
be impossible to make deterministic because we don't know when the bh
actually get activated?
My concern with just adding replay_bh_schedule_event in the appropriate
places is we end up with a patchwork of support for different devices
and make it very easy to break things.
--
Alex Bennée
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] replay: improve determinism of virtio-net
2021-10-20 13:40 ` Alex Bennée
@ 2021-10-20 14:50 ` Michael S. Tsirkin
0 siblings, 0 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2021-10-20 14:50 UTC (permalink / raw)
To: Alex Bennée; +Cc: pbonzini, Jason Wang, Pavel Dovgalyuk, qemu-devel
On Wed, Oct 20, 2021 at 02:40:18PM +0100, Alex Bennée wrote:
>
> Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> writes:
>
> > On 31.05.2021 07:55, Jason Wang wrote:
> >> 在 2021/5/17 下午9:04, Pavel Dovgalyuk 写道:
> <snip>
> >>> @@ -2546,7 +2547,7 @@ static void
> >>> virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
> >>> return;
> >>> }
> >>> virtio_queue_set_notification(vq, 0);
> >>> - qemu_bh_schedule(q->tx_bh);
> >>> + replay_bh_schedule_event(q->tx_bh);
> >> Not familiar with replay but any chance to change qemu_bh_schedule()
> >> instead?
> >
> > It would be better, but sometimes qemu_bh_schedule is used for the
> > callbacks that are not related to the guest state change.
>
> Maybe that indicates we should expand the API:
>
> void qemu_bh_schedule(QEMUBH *bh, bool guest_state);
>
> or maybe explicit functions:
>
> void qemu_bh_schedule(QEMUBH *bh);
> void qemu_bh_schedule_guest_update(QEMUBH *bh);
>
> And document the cases with proper prototypes in main-loop.h.
>
> While I was poking around I also saw qemu_bh_schedule_idle which made me
> wonder what happens if a guest change is triggered by this. Would this
> be impossible to make deterministic because we don't know when the bh
> actually get activated?
>
> My concern with just adding replay_bh_schedule_event in the appropriate
> places is we end up with a patchwork of support for different devices
> and make it very easy to break things.
Right. In fact I think the default should be guest update,
and some kind of new API to be used for things that are
not guest visible.
We really need static analysis to enforce this kind of
constraint, too.
> --
> Alex Bennée
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] replay: improve determinism of virtio-net
2021-05-17 13:04 [PATCH] replay: improve determinism of virtio-net Pavel Dovgalyuk
2021-05-31 4:46 ` Pavel Dovgalyuk
2021-05-31 4:55 ` Jason Wang
@ 2021-07-02 15:11 ` Michael S. Tsirkin
2021-07-02 15:22 ` Philippe Mathieu-Daudé
3 siblings, 0 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2021-07-02 15:11 UTC (permalink / raw)
To: Pavel Dovgalyuk; +Cc: pbonzini, jasowang, alex.bennee, qemu-devel
On Mon, May 17, 2021 at 04:04:20PM +0300, Pavel Dovgalyuk wrote:
> virtio-net device uses bottom halves for callbacks.
> These callbacks should be deterministic, because they affect VM state.
> This patch replaces BH invocations with corresponding replay functions,
> making them deterministic in record/replay mode.
> This patch also disables guest announce timers for record/replay,
> because they break correct loadvm in deterministic mode.
>
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
Seems to make sense but clearly Jason's area.
Jason?
> ---
> hw/net/virtio-net.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 6b7e8dd04e..e876363236 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -44,6 +44,7 @@
> #include "hw/pci/pci.h"
> #include "net_rx_pkt.h"
> #include "hw/virtio/vhost.h"
> +#include "sysemu/replay.h"
>
> #define VIRTIO_NET_VM_VERSION 11
>
> @@ -394,7 +395,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> timer_mod(q->tx_timer,
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + n->tx_timeout);
> } else {
> - qemu_bh_schedule(q->tx_bh);
> + replay_bh_schedule_event(q->tx_bh);
> }
> } else {
> if (q->tx_timer) {
> @@ -2546,7 +2547,7 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
> return;
> }
> virtio_queue_set_notification(vq, 0);
> - qemu_bh_schedule(q->tx_bh);
> + replay_bh_schedule_event(q->tx_bh);
> }
>
> static void virtio_net_tx_timer(void *opaque)
> @@ -2602,7 +2603,7 @@ static void virtio_net_tx_bh(void *opaque)
> /* If we flush a full burst of packets, assume there are
> * more coming and immediately reschedule */
> if (ret >= n->tx_burst) {
> - qemu_bh_schedule(q->tx_bh);
> + replay_bh_schedule_event(q->tx_bh);
> q->tx_waiting = 1;
> return;
> }
> @@ -2616,7 +2617,7 @@ static void virtio_net_tx_bh(void *opaque)
> return;
> } else if (ret > 0) {
> virtio_queue_set_notification(q->tx_vq, 0);
> - qemu_bh_schedule(q->tx_bh);
> + replay_bh_schedule_event(q->tx_bh);
> q->tx_waiting = 1;
> }
> }
> @@ -3206,6 +3207,10 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
> n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
> }
>
> + if (replay_mode != REPLAY_MODE_NONE) {
> + n->host_features &= ~(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE);
> + }
> +
> if (n->net_conf.duplex_str) {
> if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
> n->net_conf.duplex = DUPLEX_HALF;
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] replay: improve determinism of virtio-net
2021-05-17 13:04 [PATCH] replay: improve determinism of virtio-net Pavel Dovgalyuk
` (2 preceding siblings ...)
2021-07-02 15:11 ` Michael S. Tsirkin
@ 2021-07-02 15:22 ` Philippe Mathieu-Daudé
3 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-02 15:22 UTC (permalink / raw)
To: Pavel Dovgalyuk, qemu-devel; +Cc: pbonzini, jasowang, alex.bennee, mst
On 5/17/21 3:04 PM, Pavel Dovgalyuk wrote:
> virtio-net device uses bottom halves for callbacks.
> These callbacks should be deterministic, because they affect VM state.
> This patch replaces BH invocations with corresponding replay functions,
> making them deterministic in record/replay mode.
^ This is one change which I'm OK to give a R-b tag.
> This patch also disables guest announce timers for record/replay,
> because they break correct loadvm in deterministic mode.
^ This is another change, can you keep it separately? This
would help in case something need to be reverted.
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> ---
> hw/net/virtio-net.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
^ permalink raw reply [flat|nested] 11+ messages in thread