Re: [PATCH] replay: improve determinism of virtio-net

[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?

> > 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;