| Message ID | 1290665220-26478-6-git-send-email-tamura.yoshiaki@lab.ntt.co.jp |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 25, 2010 at 03:06:44PM +0900, Yoshiaki Tamura wrote: > Modify inuse type to uint16_t, let save/load to handle, and revert > last_avail_idx with inuse if there are outstanding emulation. > > Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> This changes migration format, so it will break compatibility with existing drivers. More generally, I think migrating internal state that is not guest visible is always a mistake as it ties migration format to an internal implementation (yes, I know we do this sometimes, but we should at least try not to add such cases). I think the right thing to do in this case is to flush outstanding work when vm is stopped. Then, we are guaranteed that inuse is 0. I sent patches that do this for virtio net and block. > --- > hw/virtio.c | 8 +++++++- > 1 files changed, 7 insertions(+), 1 deletions(-) > > diff --git a/hw/virtio.c b/hw/virtio.c > index 849a60f..5509644 100644 > --- a/hw/virtio.c > +++ b/hw/virtio.c > @@ -72,7 +72,7 @@ struct VirtQueue > VRing vring; > target_phys_addr_t pa; > uint16_t last_avail_idx; > - int inuse; > + uint16_t inuse; > uint16_t vector; > void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq); > VirtIODevice *vdev; > @@ -671,6 +671,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) > qemu_put_be32(f, vdev->vq[i].vring.num); > qemu_put_be64(f, vdev->vq[i].pa); > qemu_put_be16s(f, &vdev->vq[i].last_avail_idx); > + qemu_put_be16s(f, &vdev->vq[i].inuse); > if (vdev->binding->save_queue) > vdev->binding->save_queue(vdev->binding_opaque, i, f); > } > @@ -711,6 +712,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) > vdev->vq[i].vring.num = qemu_get_be32(f); > vdev->vq[i].pa = qemu_get_be64(f); > qemu_get_be16s(f, &vdev->vq[i].last_avail_idx); > + qemu_get_be16s(f, &vdev->vq[i].inuse); > + > + /* revert last_avail_idx if there are outstanding emulation. */ > + vdev->vq[i].last_avail_idx -= vdev->vq[i].inuse; > + vdev->vq[i].inuse = 0; > > if (vdev->vq[i].pa) { > virtqueue_init(&vdev->vq[i]); > -- > 1.7.1.2 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
2010/11/28 Michael S. Tsirkin <mst@redhat.com>: > On Thu, Nov 25, 2010 at 03:06:44PM +0900, Yoshiaki Tamura wrote: >> Modify inuse type to uint16_t, let save/load to handle, and revert >> last_avail_idx with inuse if there are outstanding emulation. >> >> Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> > > This changes migration format, so it will break compatibility with > existing drivers. More generally, I think migrating internal > state that is not guest visible is always a mistake > as it ties migration format to an internal implementation > (yes, I know we do this sometimes, but we should at least > try not to add such cases). I think the right thing to do in this case > is to flush outstanding > work when vm is stopped. Then, we are guaranteed that inuse is 0. > I sent patches that do this for virtio net and block. Could you give me the link of your patches? I'd like to test whether they work with Kemari upon failover. If they do, I'm happy to drop this patch. Yoshi > >> --- >> hw/virtio.c | 8 +++++++- >> 1 files changed, 7 insertions(+), 1 deletions(-) >> >> diff --git a/hw/virtio.c b/hw/virtio.c >> index 849a60f..5509644 100644 >> --- a/hw/virtio.c >> +++ b/hw/virtio.c >> @@ -72,7 +72,7 @@ struct VirtQueue >> VRing vring; >> target_phys_addr_t pa; >> uint16_t last_avail_idx; >> - int inuse; >> + uint16_t inuse; >> uint16_t vector; >> void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq); >> VirtIODevice *vdev; >> @@ -671,6 +671,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) >> qemu_put_be32(f, vdev->vq[i].vring.num); >> qemu_put_be64(f, vdev->vq[i].pa); >> qemu_put_be16s(f, &vdev->vq[i].last_avail_idx); >> + qemu_put_be16s(f, &vdev->vq[i].inuse); >> if (vdev->binding->save_queue) >> vdev->binding->save_queue(vdev->binding_opaque, i, f); >> } >> @@ -711,6 +712,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) >> vdev->vq[i].vring.num = qemu_get_be32(f); >> vdev->vq[i].pa = qemu_get_be64(f); >> qemu_get_be16s(f, &vdev->vq[i].last_avail_idx); >> + qemu_get_be16s(f, &vdev->vq[i].inuse); >> + >> + /* revert last_avail_idx if there are outstanding emulation. */ >> + vdev->vq[i].last_avail_idx -= vdev->vq[i].inuse; >> + vdev->vq[i].inuse = 0; >> >> if (vdev->vq[i].pa) { >> virtqueue_init(&vdev->vq[i]); >> -- >> 1.7.1.2 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe kvm" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Sun, Nov 28, 2010 at 08:27:58PM +0900, Yoshiaki Tamura wrote: > 2010/11/28 Michael S. Tsirkin <mst@redhat.com>: > > On Thu, Nov 25, 2010 at 03:06:44PM +0900, Yoshiaki Tamura wrote: > >> Modify inuse type to uint16_t, let save/load to handle, and revert > >> last_avail_idx with inuse if there are outstanding emulation. > >> > >> Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> > > > > This changes migration format, so it will break compatibility with > > existing drivers. More generally, I think migrating internal > > state that is not guest visible is always a mistake > > as it ties migration format to an internal implementation > > (yes, I know we do this sometimes, but we should at least > > try not to add such cases). I think the right thing to do in this case > > is to flush outstanding > > work when vm is stopped. Then, we are guaranteed that inuse is 0. > > I sent patches that do this for virtio net and block. > > Could you give me the link of your patches? I'd like to test > whether they work with Kemari upon failover. If they do, I'm > happy to drop this patch. > > Yoshi Look for this: stable migration image on a stopped vm sent on: Wed, 24 Nov 2010 17:52:49 +0200 > > > >> --- > >> hw/virtio.c | 8 +++++++- > >> 1 files changed, 7 insertions(+), 1 deletions(-) > >> > >> diff --git a/hw/virtio.c b/hw/virtio.c > >> index 849a60f..5509644 100644 > >> --- a/hw/virtio.c > >> +++ b/hw/virtio.c > >> @@ -72,7 +72,7 @@ struct VirtQueue > >> VRing vring; > >> target_phys_addr_t pa; > >> uint16_t last_avail_idx; > >> - int inuse; > >> + uint16_t inuse; > >> uint16_t vector; > >> void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq); > >> VirtIODevice *vdev; > >> @@ -671,6 +671,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) > >> qemu_put_be32(f, vdev->vq[i].vring.num); > >> qemu_put_be64(f, vdev->vq[i].pa); > >> qemu_put_be16s(f, &vdev->vq[i].last_avail_idx); > >> + qemu_put_be16s(f, &vdev->vq[i].inuse); > >> if (vdev->binding->save_queue) > >> vdev->binding->save_queue(vdev->binding_opaque, i, f); > >> } > >> @@ -711,6 +712,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) > >> vdev->vq[i].vring.num = qemu_get_be32(f); > >> vdev->vq[i].pa = qemu_get_be64(f); > >> qemu_get_be16s(f, &vdev->vq[i].last_avail_idx); > >> + qemu_get_be16s(f, &vdev->vq[i].inuse); > >> + > >> + /* revert last_avail_idx if there are outstanding emulation. */ > >> + vdev->vq[i].last_avail_idx -= vdev->vq[i].inuse; > >> + vdev->vq[i].inuse = 0; > >> > >> if (vdev->vq[i].pa) { > >> virtqueue_init(&vdev->vq[i]); > >> -- > >> 1.7.1.2 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe kvm" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > > To unsubscribe from this list: send the line "unsubscribe kvm" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
diff --git a/hw/virtio.c b/hw/virtio.c index 849a60f..5509644 100644 --- a/hw/virtio.c +++ b/hw/virtio.c @@ -72,7 +72,7 @@ struct VirtQueue VRing vring; target_phys_addr_t pa; uint16_t last_avail_idx; - int inuse; + uint16_t inuse; uint16_t vector; void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq); VirtIODevice *vdev; @@ -671,6 +671,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) qemu_put_be32(f, vdev->vq[i].vring.num); qemu_put_be64(f, vdev->vq[i].pa); qemu_put_be16s(f, &vdev->vq[i].last_avail_idx); + qemu_put_be16s(f, &vdev->vq[i].inuse); if (vdev->binding->save_queue) vdev->binding->save_queue(vdev->binding_opaque, i, f); } @@ -711,6 +712,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) vdev->vq[i].vring.num = qemu_get_be32(f); vdev->vq[i].pa = qemu_get_be64(f); qemu_get_be16s(f, &vdev->vq[i].last_avail_idx); + qemu_get_be16s(f, &vdev->vq[i].inuse); + + /* revert last_avail_idx if there are outstanding emulation. */ + vdev->vq[i].last_avail_idx -= vdev->vq[i].inuse; + vdev->vq[i].inuse = 0; if (vdev->vq[i].pa) { virtqueue_init(&vdev->vq[i]);
Modify inuse type to uint16_t, let save/load to handle, and revert last_avail_idx with inuse if there are outstanding emulation. Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> --- hw/virtio.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)