Re: [Qemu-devel] [PATCH v16 5/5] virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
On 10/13/2017 09:38 PM, Michael S. Tsirkin wrote: On Thu, Oct 12, 2017 at 11:54:56AM +0800, Wei Wang wrote: But I think flushing is very fragile. You will easily run into races if one of the actors gets out of sync and keeps adding data. I think adding an ID in the free vq stream is a more robust approach. Adding ID to the free vq would need the device to distinguish whether it receives an ID or a free page hint, Not really. It's pretty simple: a 64 bit buffer is an ID. A 4K and bigger one is a page. I think we can also use the previous method, free page via in_buf, and id via out_buf. Best, Wei
Re: [Qemu-devel] [PATCH v16 5/5] virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
On Thu, Oct 12, 2017 at 11:54:56AM +0800, Wei Wang wrote: > > But I think flushing is very fragile. You will easily run into races > > if one of the actors gets out of sync and keeps adding data. > > I think adding an ID in the free vq stream is a more robust > > approach. > > > > Adding ID to the free vq would need the device to distinguish whether it > receives an ID or a free page hint, Not really. It's pretty simple: a 64 bit buffer is an ID. A 4K and bigger one is a page. > so an extra protocol is needed for the two sides to talk. Currently, we > directly assign the free page > address to desc->addr. With ID support, we would need to first allocate > buffer for the protocol header, > and add the free page address to the header, then desc->addr = I do not think you should add ID on each page. What would be the point? Add it each time you detect a new start command. > How about putting the ID to the command path? This would avoid the above > trouble. > > For example, using the 32-bit config registers: > first 16-bit: Command field > send 16-bit: ID field > > Then, the working flow would look like this: > > 1) Host writes "Start, 1" to the Host2Guest register and notify; > > 2) Guest reads Host2Guest register, and ACKs by writing "Start, 1" to > Guest2Host register; > > 3) Guest starts report free pages; > > 4) Each time when the host receives a free page hint from the free_page_vq, > it compares the ID fields of > the Host2Guest and Guest2Host register. If matching, then filter out the > free page from the migration dirty bitmap, > otherwise, simply push back without doing the filtering. > > > Best, > Wei All fine but config and vq ops are asynchronous. Host has no idea when were entries added to vq. So the ID sent to host needs to be through vq. And I would make it a 64 or at least 32 bit ID, not a 16 bit one, to avoid wrap-around. -- MST
Re: [Qemu-devel] [PATCH v16 5/5] virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
On 10/11/2017 09:49 PM, Michael S. Tsirkin wrote:
On Wed, Oct 11, 2017 at 02:03:20PM +0800, Wei Wang wrote:
On 10/10/2017 11:15 PM, Michael S. Tsirkin wrote:
On Mon, Oct 02, 2017 at 04:38:01PM +, Wang, Wei W wrote:
On Sunday, October 1, 2017 11:19 AM, Michael S. Tsirkin wrote:
On Sat, Sep 30, 2017 at 12:05:54PM +0800, Wei Wang wrote:
+static void ctrlq_send_cmd(struct virtio_balloon *vb,
+ struct virtio_balloon_ctrlq_cmd *cmd,
+ bool inbuf)
+{
+ struct virtqueue *vq = vb->ctrl_vq;
+
+ ctrlq_add_cmd(vq, cmd, inbuf);
+ if (!inbuf) {
+ /*
+* All the input cmd buffers are replenished here.
+* This is necessary because the input cmd buffers are lost
+* after live migration. The device needs to rewind all of
+* them from the ctrl_vq.
Confused. Live migration somehow loses state? Why is that and why is it a good
idea? And how do you know this is migration even?
Looks like all you know is you got free page end. Could be any reason for this.
I think this would be something that the current live migration lacks - what the
device read from the vq is not transferred during live migration, an example is
the
stat_vq_elem:
Line 476 at https://github.com/qemu/qemu/blob/master/hw/virtio/virtio-balloon.c
This does not touch guest memory though it just manipulates
internal state to make it easier to migrate.
It's transparent to guest as migration should be.
For all the things that are added to the vq and need to be held by the device
to use later need to consider the situation that live migration might happen at
any
time and they need to be re-taken from the vq by the device on the destination
machine.
So, even without this live migration optimization feature, I think all the
things that are
added to the vq for the device to hold, need a way for the device to rewind
back from
the vq - re-adding all the elements to the vq is a trick to keep a record of
all of them
on the vq so that the device side rewinding can work.
Please let me know if anything is missed or if you have other suggestions.
IMO migration should pass enough data source to destination for
destination to continue where source left off without guest help.
I'm afraid it would be difficult to pass the entire VirtQueueElement to the
destination. I think
that would also be the reason that stats_vq_elem chose to rewind from the
guest vq, which re-do the
virtqueue_pop() --> virtqueue_map_desc() steps (the QEMU virtual address to
the guest physical
address relationship may be changed on the destination).
Yes but note how that rewind does not involve modifying the ring.
It just rolls back some indices.
Yes, it rolls back the indices, then the following
virtio_balloon_receive_stats()
can re-pop out the previous entry given by the guest.
Recall how stats_vq_elem works: there is only one stats buffer, which is
used by the
guest to report stats, and also used by the host to ask the guest for
stats report.
So the host can roll back one previous entry and what it gets will
always be stat_vq_elem.
Our case is a little more complex than that - we have both free_page_cmd_in
(for host to guest command) and free_page_cmd_out (for guest to host
command) buffer
passed via ctrl_vq. When the host rolls back one entry, it may get the
free_page_cmd_out
buffer which can't be used as the host to guest buffer (i.e.
free_page_elem held by the device).
So a trick in the driver is to refill the free_page_cmd_in buffer every
time after the free_page_cmd_out
was sent to the host, so that when the host rewind one previous entry,
it can always get the
free_page_cmd_in buffer (may be not a very nice method).
How about another direction which would be easier - using two 32-bit device
specific configuration registers,
Host2Guest and Guest2Host command registers, to replace the ctrlq for
command exchange:
The flow can be as follows:
1) Before Host sending a StartCMD, it flushes the free_page_vq in case any
old free page hint is left there;
2) Host writes StartCMD to the Host2Guest register, and notifies the guest;
3) Upon receiving a configuration notification, Guest reads the Host2Guest
register, and detaches all the used buffers from free_page_vq;
(then for each StartCMD, the free_page_vq will always have no obsolete free
page hints, right? )
4) Guest start report free pages:
4.1) Host may actively write StopCMD to the Host2Guest register before
the guest finishes; or
4.2) Guest finishes reporting, write StopCMD the Guest2HOST register,
which traps to QEMU, to stop.
Best,
Wei
I am not sure it matters whether a VQ or the config are used to start/stop.
Not matters, in terms of the flushing issue. The config method could
avoid the above rewind issue.
But I think flushing is very fragile. You will easily run into races
if one of the actors gets out of sync and keeps adding data.
I
Re: [Qemu-devel] [PATCH v16 5/5] virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
On Wed, Oct 11, 2017 at 02:03:20PM +0800, Wei Wang wrote:
> On 10/10/2017 11:15 PM, Michael S. Tsirkin wrote:
> > On Mon, Oct 02, 2017 at 04:38:01PM +, Wang, Wei W wrote:
> > > On Sunday, October 1, 2017 11:19 AM, Michael S. Tsirkin wrote:
> > > > On Sat, Sep 30, 2017 at 12:05:54PM +0800, Wei Wang wrote:
> > > > > +static void ctrlq_send_cmd(struct virtio_balloon *vb,
> > > > > + struct virtio_balloon_ctrlq_cmd *cmd,
> > > > > + bool inbuf)
> > > > > +{
> > > > > + struct virtqueue *vq = vb->ctrl_vq;
> > > > > +
> > > > > + ctrlq_add_cmd(vq, cmd, inbuf);
> > > > > + if (!inbuf) {
> > > > > + /*
> > > > > + * All the input cmd buffers are replenished here.
> > > > > + * This is necessary because the input cmd buffers are
> > > > > lost
> > > > > + * after live migration. The device needs to rewind all
> > > > > of
> > > > > + * them from the ctrl_vq.
> > > > Confused. Live migration somehow loses state? Why is that and why is it
> > > > a good
> > > > idea? And how do you know this is migration even?
> > > > Looks like all you know is you got free page end. Could be any reason
> > > > for this.
> > >
> > > I think this would be something that the current live migration lacks -
> > > what the
> > > device read from the vq is not transferred during live migration, an
> > > example is the
> > > stat_vq_elem:
> > > Line 476 at
> > > https://github.com/qemu/qemu/blob/master/hw/virtio/virtio-balloon.c
> > This does not touch guest memory though it just manipulates
> > internal state to make it easier to migrate.
> > It's transparent to guest as migration should be.
> >
> > > For all the things that are added to the vq and need to be held by the
> > > device
> > > to use later need to consider the situation that live migration might
> > > happen at any
> > > time and they need to be re-taken from the vq by the device on the
> > > destination
> > > machine.
> > >
> > > So, even without this live migration optimization feature, I think all
> > > the things that are
> > > added to the vq for the device to hold, need a way for the device to
> > > rewind back from
> > > the vq - re-adding all the elements to the vq is a trick to keep a record
> > > of all of them
> > > on the vq so that the device side rewinding can work.
> > >
> > > Please let me know if anything is missed or if you have other suggestions.
> > IMO migration should pass enough data source to destination for
> > destination to continue where source left off without guest help.
> >
>
> I'm afraid it would be difficult to pass the entire VirtQueueElement to the
> destination. I think
> that would also be the reason that stats_vq_elem chose to rewind from the
> guest vq, which re-do the
> virtqueue_pop() --> virtqueue_map_desc() steps (the QEMU virtual address to
> the guest physical
> address relationship may be changed on the destination).
Yes but note how that rewind does not involve modifying the ring.
It just rolls back some indices.
>
> How about another direction which would be easier - using two 32-bit device
> specific configuration registers,
> Host2Guest and Guest2Host command registers, to replace the ctrlq for
> command exchange:
>
> The flow can be as follows:
>
> 1) Before Host sending a StartCMD, it flushes the free_page_vq in case any
> old free page hint is left there;
> 2) Host writes StartCMD to the Host2Guest register, and notifies the guest;
>
> 3) Upon receiving a configuration notification, Guest reads the Host2Guest
> register, and detaches all the used buffers from free_page_vq;
> (then for each StartCMD, the free_page_vq will always have no obsolete free
> page hints, right? )
>
> 4) Guest start report free pages:
> 4.1) Host may actively write StopCMD to the Host2Guest register before
> the guest finishes; or
> 4.2) Guest finishes reporting, write StopCMD the Guest2HOST register,
> which traps to QEMU, to stop.
>
>
> Best,
> Wei
I am not sure it matters whether a VQ or the config are used to start/stop.
But I think flushing is very fragile. You will easily run into races
if one of the actors gets out of sync and keeps adding data.
I think adding an ID in the free vq stream is a more robust
approach.
Re: [Qemu-devel] [PATCH v16 5/5] virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
On 10/10/2017 11:15 PM, Michael S. Tsirkin wrote:
On Mon, Oct 02, 2017 at 04:38:01PM +, Wang, Wei W wrote:
On Sunday, October 1, 2017 11:19 AM, Michael S. Tsirkin wrote:
On Sat, Sep 30, 2017 at 12:05:54PM +0800, Wei Wang wrote:
+static void ctrlq_send_cmd(struct virtio_balloon *vb,
+ struct virtio_balloon_ctrlq_cmd *cmd,
+ bool inbuf)
+{
+ struct virtqueue *vq = vb->ctrl_vq;
+
+ ctrlq_add_cmd(vq, cmd, inbuf);
+ if (!inbuf) {
+ /*
+* All the input cmd buffers are replenished here.
+* This is necessary because the input cmd buffers are lost
+* after live migration. The device needs to rewind all of
+* them from the ctrl_vq.
Confused. Live migration somehow loses state? Why is that and why is it a good
idea? And how do you know this is migration even?
Looks like all you know is you got free page end. Could be any reason for this.
I think this would be something that the current live migration lacks - what the
device read from the vq is not transferred during live migration, an example is
the
stat_vq_elem:
Line 476 at https://github.com/qemu/qemu/blob/master/hw/virtio/virtio-balloon.c
This does not touch guest memory though it just manipulates
internal state to make it easier to migrate.
It's transparent to guest as migration should be.
For all the things that are added to the vq and need to be held by the device
to use later need to consider the situation that live migration might happen at
any
time and they need to be re-taken from the vq by the device on the destination
machine.
So, even without this live migration optimization feature, I think all the
things that are
added to the vq for the device to hold, need a way for the device to rewind
back from
the vq - re-adding all the elements to the vq is a trick to keep a record of
all of them
on the vq so that the device side rewinding can work.
Please let me know if anything is missed or if you have other suggestions.
IMO migration should pass enough data source to destination for
destination to continue where source left off without guest help.
I'm afraid it would be difficult to pass the entire VirtQueueElement to
the destination. I think
that would also be the reason that stats_vq_elem chose to rewind from
the guest vq, which re-do the
virtqueue_pop() --> virtqueue_map_desc() steps (the QEMU virtual address
to the guest physical
address relationship may be changed on the destination).
How about another direction which would be easier - using two 32-bit
device specific configuration registers,
Host2Guest and Guest2Host command registers, to replace the ctrlq for
command exchange:
The flow can be as follows:
1) Before Host sending a StartCMD, it flushes the free_page_vq in case
any old free page hint is left there;
2) Host writes StartCMD to the Host2Guest register, and notifies the guest;
3) Upon receiving a configuration notification, Guest reads the
Host2Guest register, and detaches all the used buffers from free_page_vq;
(then for each StartCMD, the free_page_vq will always have no obsolete
free page hints, right? )
4) Guest start report free pages:
4.1) Host may actively write StopCMD to the Host2Guest register
before the guest finishes; or
4.2) Guest finishes reporting, write StopCMD the Guest2HOST
register, which traps to QEMU, to stop.
Best,
Wei
Re: [Qemu-devel] [PATCH v16 5/5] virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
On Mon, Oct 02, 2017 at 04:38:01PM +, Wang, Wei W wrote:
> On Sunday, October 1, 2017 11:19 AM, Michael S. Tsirkin wrote:
> > On Sat, Sep 30, 2017 at 12:05:54PM +0800, Wei Wang wrote:
> > > +static void ctrlq_send_cmd(struct virtio_balloon *vb,
> > > + struct virtio_balloon_ctrlq_cmd *cmd,
> > > + bool inbuf)
> > > +{
> > > + struct virtqueue *vq = vb->ctrl_vq;
> > > +
> > > + ctrlq_add_cmd(vq, cmd, inbuf);
> > > + if (!inbuf) {
> > > + /*
> > > + * All the input cmd buffers are replenished here.
> > > + * This is necessary because the input cmd buffers are lost
> > > + * after live migration. The device needs to rewind all of
> > > + * them from the ctrl_vq.
> >
> > Confused. Live migration somehow loses state? Why is that and why is it a
> > good
> > idea? And how do you know this is migration even?
> > Looks like all you know is you got free page end. Could be any reason for
> > this.
>
>
> I think this would be something that the current live migration lacks - what
> the
> device read from the vq is not transferred during live migration, an example
> is the
> stat_vq_elem:
> Line 476 at
> https://github.com/qemu/qemu/blob/master/hw/virtio/virtio-balloon.c
This does not touch guest memory though it just manipulates
internal state to make it easier to migrate.
It's transparent to guest as migration should be.
> For all the things that are added to the vq and need to be held by the device
> to use later need to consider the situation that live migration might happen
> at any
> time and they need to be re-taken from the vq by the device on the destination
> machine.
>
> So, even without this live migration optimization feature, I think all the
> things that are
> added to the vq for the device to hold, need a way for the device to rewind
> back from
> the vq - re-adding all the elements to the vq is a trick to keep a record of
> all of them
> on the vq so that the device side rewinding can work.
>
> Please let me know if anything is missed or if you have other suggestions.
IMO migration should pass enough data source to destination for
destination to continue where source left off without guest help.
>
> > > +static void ctrlq_handle(struct virtqueue *vq) {
> > > + struct virtio_balloon *vb = vq->vdev->priv;
> > > + struct virtio_balloon_ctrlq_cmd *msg;
> > > + unsigned int class, cmd, len;
> > > +
> > > + msg = (struct virtio_balloon_ctrlq_cmd *)virtqueue_get_buf(vq, );
> > > + if (unlikely(!msg))
> > > + return;
> > > +
> > > + /* The outbuf is sent by the host for recycling, so just return. */
> > > + if (msg == >free_page_cmd_out)
> > > + return;
> > > +
> > > + class = virtio32_to_cpu(vb->vdev, msg->class);
> > > + cmd = virtio32_to_cpu(vb->vdev, msg->cmd);
> > > +
> > > + switch (class) {
> > > + case VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE:
> > > + if (cmd == VIRTIO_BALLOON_FREE_PAGE_F_STOP) {
> > > + vb->report_free_page_stop = true;
> > > + } else if (cmd == VIRTIO_BALLOON_FREE_PAGE_F_START) {
> > > + vb->report_free_page_stop = false;
> > > + queue_work(vb->balloon_wq,
> > >report_free_page_work);
> > > + }
> > > + vb->free_page_cmd_in.class =
> > > +
> > VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE;
> > > + ctrlq_send_cmd(vb, >free_page_cmd_in, true);
> > > + break;
> > > + default:
> > > + dev_warn(>vdev->dev, "%s: cmd class not supported\n",
> > > + __func__);
> > > + }
> >
> > Manipulating report_free_page_stop without any locks looks very suspicious.
>
> > Also, what if we get two start commands? we should restart from beginning,
> > should we not?
> >
>
>
> Yes, it will start to report free pages from the beginning.
> walk_free_mem_block() doesn't maintain any internal status, so the invoking of
> it will always start from the beginning.
Well yes but it will first complete the previous walk.
>
> > > +/* Ctrlq commands related to VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE
> > */
> > > +#define VIRTIO_BALLOON_FREE_PAGE_F_STOP 0
> > > +#define VIRTIO_BALLOON_FREE_PAGE_F_START 1
> > > +
> > > #endif /* _LINUX_VIRTIO_BALLOON_H */
> >
> > The stop command does not appear to be thought through.
> >
> > Let's assume e.g. you started migration. You ask guest for free pages.
> > Then you cancel it. There are a bunch of pages in free vq and you are
> > getting
> > more. You now want to start migration again. What to do?
> >
> > A bunch of vq flushing and waiting will maybe do the trick, but waiting on
> > guest
> > is never a great idea.
> >
>
>
> I think the device can flush (pop out what's left in the vq and push them
> back) the
> vq right after the Stop command is sent to the guest, rather than doing the
> flush
> when the 2nd initiation of live migration begins. The entries pushed back to
> the vq
> will be in the used ring, what would the
