OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]


Subject: Re: [PATCH v5 4/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT


On 03/16/2018 11:16 PM, Michael S. Tsirkin wrote:
On Fri, Mar 16, 2018 at 06:48:28PM +0800, Wei Wang wrote:
The new feature enables the virtio-balloon device to receive hints of
guest free pages from the free page vq.

balloon_free_page_start - start guest free page hint reporting.
balloon_free_page_stop - stop guest free page hint reporting.

Note: balloon will report pages which were free at the time
of this call. As the reporting happens asynchronously, dirty bit logging
must be enabled before this call is made. Guest reporting must be
disabled before the migration dirty bitmap is synchronized.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Signed-off-by: Liang Li <liang.z.li@intel.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
---
  balloon.c                                       |  58 +++++--
  hw/virtio/virtio-balloon.c                      | 217 ++++++++++++++++++++++--
  include/hw/virtio/virtio-balloon.h              |  20 ++-
  include/standard-headers/linux/virtio_balloon.h |   7 +
  include/sysemu/balloon.h                        |  15 +-
  5 files changed, 288 insertions(+), 29 deletions(-)

diff --git a/balloon.c b/balloon.c
index 6bf0a96..87a0410 100644
--- a/balloon.c
+++ b/balloon.c
@@ -36,6 +36,9 @@
+static void *virtio_balloon_poll_free_page_hints(void *opaque)
+{
+    VirtQueueElement *elem;
+    VirtIOBalloon *dev = opaque;
+    VirtQueue *vq = dev->free_page_vq;
+    uint32_t id;
+    size_t size;
+
+    /* The optimization thread runs only when the guest is running. */
+    while (runstate_is_running()) {
Note that this check is not guaranteed to be correct
when checked like this outside BQL.

I think you are better off relying on status
callback to synchronise with the backend thread.


It's actually OK here, I think we don't need the guarantee. The device is just the consumer of the vq, essentially it doesn't have a dependency (i.e. won't block or cause errors) on the guest state.
For example:
1) when the device executes "while (runstate_is_running())" and finds that the guest is running, it proceeds; 2) the guest is stopped immediately right after the "while (runstate_is_running())" check; 3) the device side execution reaches virtqueue_pop(), and finds "elem==NULL", since the driver (provider) stops adding hints. Then it continues by going back to "while (runstate_is_running())", and now it finds the guest is stopped, and then exits.

Essentially, this runstate check is just an optimization to the case that the driver is stopped to provide hints while the device side optimization thread is still polling the empty vq (i.e. effort in vain). Probably, it would be better to check the runstate under "elem==NULL":

while (1) {
    ...
    elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
    if (!elem) {
            qemu_spin_unlock(&dev->free_page_lock);
            if (runstate_is_running())
                continue;
            else
                break;
    }
    ...
}


+                dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
+            } else if (dev->free_page_report_status ==
+                       FREE_PAGE_REPORT_S_START) {
+                /*
+                 * Stop the optimization only when it has started. This avoids
+                 * a stale stop sign for the previous command.
+                 */
+                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+                qemu_spin_unlock(&dev->free_page_lock);
+                break;
+            }
And else? What if it does not match and status is not start?
Don't you need to skip in elem decoding?

No, actually we don't need "else". Please see the code inside if (elem->in_num) below. If the status isn't set to START, qemu_guest_free_page_hint will not be called to decode the elem.



+        }
+
+        if (elem->in_num) {
+            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START &&
+                !dev->poison_val) {
poison generally disables everything? Add a TODO to handle
it in he future pls.


OK, will add TODO in the commit.



@@ -368,6 +499,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
                         ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT),
                         &error_abort);
     }
+    dev->poison_val = le32_to_cpu(config.poison_val);

We probably should not assume this value is correct if guest didn't
ack the appropriate feature.


OK, I'll add a comment to explain that.


@@ -475,11 +632,37 @@ static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
- if (!s->stats_vq_elem && vdev->vm_running &&
-        (status & VIRTIO_CONFIG_S_DRIVER_OK) && virtqueue_rewind(s->svq, 1)) {
-        /* poll stats queue for the element we have discarded when the VM
-         * was stopped */
-        virtio_balloon_receive_stats(vdev, s->svq);
+    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+        if (!s->stats_vq_elem && vdev->vm_running &&
+            virtqueue_rewind(s->svq, 1)) {
+            /*
+             * Poll stats queue for the element we have discarded when the VM
+             * was stopped.
+             */
+            virtio_balloon_receive_stats(vdev, s->svq);
+        }
+
+        if (virtio_balloon_free_page_support(s)) {
+            qemu_add_balloon_handler(virtio_balloon_to_target,
+                                     virtio_balloon_stat,
+                                     virtio_balloon_free_page_support,
+                                     virtio_balloon_free_page_start,
+                                     virtio_balloon_free_page_stop,
+                                     s);
+            /*
+             * This handles the case that the guest is being stopped (e.g. by
+             * qmp commands) while the driver is still reporting hints. When
+             * the guest is woken up, it will continue to report hints, which
+             * are not needed. So when the wakeup notifier invokes the
+             * set_status callback here, we get the chance to make sure that
+             * the free page optimization thread is exited via
+             * virtio_balloon_free_page_stop.
+             */
+            virtio_balloon_free_page_stop(s);

I don't understand the logic here at all.
So if status is set to DRIVER_OK, then we stop reporting?


I thought about this more. It's not necessary to call the stop() function here, because we have already made the optimization thread exit when the guest is stopped. When the guest is woken up, it will continue to report, and there is no consumer of the vq (the optimization thread has exited). There is no functional issue here, since the driver will just drop the hint when the vq is full. From the optimization point of view, I think we can consider to replace the above sop() function with virtio_notify_config(vdev), which will stop the guest's unnecessary reporting.


Best,
Wei


[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]