[PATCH v21 QEMU 0/5] virtio-balloon: add support for free page reporting

[PATCH v21 QEMU 0/5] virtio-balloon: add support for free page reporting

[Alexander Duyck]

This series provides an asynchronous means of reporting free guest pages
to QEMU through virtio-balloon so that the memory associated with those
pages can be dropped and reused by other processes and/or guests on the
host. Using this it is possible to avoid unnecessary I/O to disk and
greatly improve performance in the case of memory overcommit on the host.

As of April 7th this functionality has been enabled in Linus's kernel
tree[1] and so I am submitting the QEMU pieces for inclusion.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b0c504f154718904ae49349147e3b7e6ae91ffdc

Changes from v17:
Fixed typo in patch 1 title
Addressed white-space issues reported via checkpatch
Added braces {} for two if statements to match expected coding style

Changes from v18:
Updated patches 2 and 3 based on input from dhildenb
Added comment to patch 2 describing what keeps us from reporting a bad page
Added patch to address issue with ROM devices being directly writable

Changes from v19:
Added std-headers change to match changes pushed for linux kernel headers
Added patch to remove "report" from page hinting code paths
Updated comment to better explain why we disable hints w/ page poisoning
Removed code that was modifying config size for poison vs hinting
Dropped x-page-poison property
Added code to bounds check the reported region vs the RAM block
Dropped patch for ROM devices as that was already pulled in by Paolo

Changes from v20:
Rearranged patches to push Linux header sync patches to front
Removed association between free page hinting and VIRTIO_BALLOON_F_PAGE_POISON
Added code to enable VIRTIO_BALLOON_F_PAGE_POISON if page reporting is enabled
Fixed possible resource leak if poison or qemu_balloon_is_inhibited return true
Updated cover page comments

---

Alexander Duyck (5):
      linux-headers: Update to allow renaming of free_page_report_cmd_id
      linux-headers: update to contain virito-balloon free page reporting
      virtio-balloon: Replace free page hinting references to 'report' with 'hint'
      virtio-balloon: Implement support for page poison tracking feature
      virtio-balloon: Provide an interface for free page reporting


 hw/virtio/virtio-balloon.c                      |  151 +++++++++++++++++------
 include/hw/virtio/virtio-balloon.h              |   23 ++--
 include/standard-headers/linux/virtio_balloon.h |   12 ++
 3 files changed, 136 insertions(+), 50 deletions(-)

--

[virtio-dev] [PATCH v21 QEMU 0/5] virtio-balloon: add support for free page reporting

[Alexander Duyck]

This series provides an asynchronous means of reporting free guest pages
to QEMU through virtio-balloon so that the memory associated with those
pages can be dropped and reused by other processes and/or guests on the
host. Using this it is possible to avoid unnecessary I/O to disk and
greatly improve performance in the case of memory overcommit on the host.

As of April 7th this functionality has been enabled in Linus's kernel
tree[1] and so I am submitting the QEMU pieces for inclusion.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b0c504f154718904ae49349147e3b7e6ae91ffdc

Changes from v17:
Fixed typo in patch 1 title
Addressed white-space issues reported via checkpatch
Added braces {} for two if statements to match expected coding style

Changes from v18:
Updated patches 2 and 3 based on input from dhildenb
Added comment to patch 2 describing what keeps us from reporting a bad page
Added patch to address issue with ROM devices being directly writable

Changes from v19:
Added std-headers change to match changes pushed for linux kernel headers
Added patch to remove "report" from page hinting code paths
Updated comment to better explain why we disable hints w/ page poisoning
Removed code that was modifying config size for poison vs hinting
Dropped x-page-poison property
Added code to bounds check the reported region vs the RAM block
Dropped patch for ROM devices as that was already pulled in by Paolo

Changes from v20:
Rearranged patches to push Linux header sync patches to front
Removed association between free page hinting and VIRTIO_BALLOON_F_PAGE_POISON
Added code to enable VIRTIO_BALLOON_F_PAGE_POISON if page reporting is enabled
Fixed possible resource leak if poison or qemu_balloon_is_inhibited return true
Updated cover page comments

---

Alexander Duyck (5):
      linux-headers: Update to allow renaming of free_page_report_cmd_id
      linux-headers: update to contain virito-balloon free page reporting
      virtio-balloon: Replace free page hinting references to 'report' with 'hint'
      virtio-balloon: Implement support for page poison tracking feature
      virtio-balloon: Provide an interface for free page reporting


 hw/virtio/virtio-balloon.c                      |  151 +++++++++++++++++------
 include/hw/virtio/virtio-balloon.h              |   23 ++--
 include/standard-headers/linux/virtio_balloon.h |   12 ++
 3 files changed, 136 insertions(+), 50 deletions(-)

--

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[PATCH v21 QEMU 1/5] linux-headers: Update to allow renaming of free_page_report_cmd_id

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Sync to the latest upstream changes for free page hinting. To be
replaced by a full linux header sync.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 include/standard-headers/linux/virtio_balloon.h |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h
index 9375ca2a70de..af0a6b59dab2 100644
--- a/include/standard-headers/linux/virtio_balloon.h
+++ b/include/standard-headers/linux/virtio_balloon.h
@@ -47,8 +47,15 @@ struct virtio_balloon_config {
 	uint32_t num_pages;
 	/* Number of pages we've actually got in balloon. */
 	uint32_t actual;
-	/* Free page report command id, readonly by guest */
-	uint32_t free_page_report_cmd_id;
+	/*
+	 * Free page hint command id, readonly by guest.
+	 * Was previously name free_page_report_cmd_id so we
+	 * need to carry that name for legacy support.
+	 */
+	union {
+		uint32_t free_page_hint_cmd_id;
+		uint32_t free_page_report_cmd_id;	/* deprecated */
+	};
 	/* Stores PAGE_POISON if page poisoning is in use */
 	uint32_t poison_val;
 };

[virtio-dev] [PATCH v21 QEMU 1/5] linux-headers: Update to allow renaming of free_page_report_cmd_id

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Sync to the latest upstream changes for free page hinting. To be
replaced by a full linux header sync.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 include/standard-headers/linux/virtio_balloon.h |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h
index 9375ca2a70de..af0a6b59dab2 100644
--- a/include/standard-headers/linux/virtio_balloon.h
+++ b/include/standard-headers/linux/virtio_balloon.h
@@ -47,8 +47,15 @@ struct virtio_balloon_config {
 	uint32_t num_pages;
 	/* Number of pages we've actually got in balloon. */
 	uint32_t actual;
-	/* Free page report command id, readonly by guest */
-	uint32_t free_page_report_cmd_id;
+	/*
+	 * Free page hint command id, readonly by guest.
+	 * Was previously name free_page_report_cmd_id so we
+	 * need to carry that name for legacy support.
+	 */
+	union {
+		uint32_t free_page_hint_cmd_id;
+		uint32_t free_page_report_cmd_id;	/* deprecated */
+	};
 	/* Stores PAGE_POISON if page poisoning is in use */
 	uint32_t poison_val;
 };


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[PATCH v21 QEMU 2/5] linux-headers: update to contain virito-balloon free page reporting

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Sync the latest upstream changes for free page reporting. To be
replaced by a full linux header sync.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 include/standard-headers/linux/virtio_balloon.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h
index af0a6b59dab2..af3b7a1fa263 100644
--- a/include/standard-headers/linux/virtio_balloon.h
+++ b/include/standard-headers/linux/virtio_balloon.h
@@ -36,6 +36,7 @@
 #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM	2 /* Deflate balloon on OOM */
 #define VIRTIO_BALLOON_F_FREE_PAGE_HINT	3 /* VQ to report free pages */
 #define VIRTIO_BALLOON_F_PAGE_POISON	4 /* Guest is using page poisoning */
+#define VIRTIO_BALLOON_F_REPORTING	5 /* Page reporting virtqueue */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12

[virtio-dev] [PATCH v21 QEMU 2/5] linux-headers: update to contain virito-balloon free page reporting

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Sync the latest upstream changes for free page reporting. To be
replaced by a full linux header sync.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 include/standard-headers/linux/virtio_balloon.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h
index af0a6b59dab2..af3b7a1fa263 100644
--- a/include/standard-headers/linux/virtio_balloon.h
+++ b/include/standard-headers/linux/virtio_balloon.h
@@ -36,6 +36,7 @@
 #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM	2 /* Deflate balloon on OOM */
 #define VIRTIO_BALLOON_F_FREE_PAGE_HINT	3 /* VQ to report free pages */
 #define VIRTIO_BALLOON_F_PAGE_POISON	4 /* Guest is using page poisoning */
+#define VIRTIO_BALLOON_F_REPORTING	5 /* Page reporting virtqueue */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[PATCH v21 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

In an upcoming patch a feature named Free Page Reporting is about to be
added. In order to avoid any confusion we should drop the use of the word
'report' when referring to Free Page Hinting. So what this patch does is go
through and replace all instances of 'report' with 'hint" when we are
referring to free page hinting.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 hw/virtio/virtio-balloon.c         |   74 ++++++++++++++++++------------------
 include/hw/virtio/virtio-balloon.h |   20 +++++-----
 2 files changed, 47 insertions(+), 47 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a4729f7fc930..a1d6fb52c876 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -466,21 +466,21 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
             ret = false;
             goto out;
         }
-        if (id == dev->free_page_report_cmd_id) {
-            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
+        if (id == dev->free_page_hint_cmd_id) {
+            dev->free_page_hint_status = FREE_PAGE_HINT_S_START;
         } else {
             /*
              * Stop the optimization only when it has started. This
              * avoids a stale stop sign for the previous command.
              */
-            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
-                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+            if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
+                dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
             }
         }
     }
 
     if (elem->in_num) {
-        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
+        if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
             qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
                                       elem->in_sg[0].iov_len);
         }
@@ -506,11 +506,11 @@ static void virtio_ballloon_get_free_page_hints(void *opaque)
         qemu_mutex_unlock(&dev->free_page_lock);
         virtio_notify(vdev, vq);
       /*
-       * Start to poll the vq once the reporting started. Otherwise, continue
+       * Start to poll the vq once the hinting started. Otherwise, continue
        * only when there are entries on the vq, which need to be given back.
        */
     } while (continue_to_get_hints ||
-             dev->free_page_report_status == FREE_PAGE_REPORT_S_START);
+             dev->free_page_hint_status == FREE_PAGE_HINT_S_START);
     virtio_queue_set_notification(vq, 1);
 }
 
@@ -531,14 +531,14 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
         return;
     }
 
-    if (s->free_page_report_cmd_id == UINT_MAX) {
-        s->free_page_report_cmd_id =
-                       VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
+    if (s->free_page_hint_cmd_id == UINT_MAX) {
+        s->free_page_hint_cmd_id =
+                       VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
     } else {
-        s->free_page_report_cmd_id++;
+        s->free_page_hint_cmd_id++;
     }
 
-    s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
+    s->free_page_hint_status = FREE_PAGE_HINT_S_REQUESTED;
     virtio_notify_config(vdev);
 }
 
@@ -546,18 +546,18 @@ static void virtio_balloon_free_page_stop(VirtIOBalloon *s)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-    if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
+    if (s->free_page_hint_status != FREE_PAGE_HINT_S_STOP) {
         /*
          * The lock also guarantees us that the
          * virtio_ballloon_get_free_page_hints exits after the
-         * free_page_report_status is set to S_STOP.
+         * free_page_hint_status is set to S_STOP.
          */
         qemu_mutex_lock(&s->free_page_lock);
         /*
          * The guest hasn't done the reporting, so host sends a notification
          * to the guest to actively stop the reporting.
          */
-        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
         qemu_mutex_unlock(&s->free_page_lock);
         virtio_notify_config(vdev);
     }
@@ -567,15 +567,15 @@ static void virtio_balloon_free_page_done(VirtIOBalloon *s)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-    s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
+    s->free_page_hint_status = FREE_PAGE_HINT_S_DONE;
     virtio_notify_config(vdev);
 }
 
 static int
-virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
+virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
 {
     VirtIOBalloon *dev = container_of(n, VirtIOBalloon,
-                                      free_page_report_notify);
+                                      free_page_hint_notify);
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     PrecopyNotifyData *pnd = data;
 
@@ -624,7 +624,7 @@ static size_t virtio_balloon_config_size(VirtIOBalloon *s)
     if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
         return offsetof(struct virtio_balloon_config, poison_val);
     }
-    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
+    return offsetof(struct virtio_balloon_config, free_page_hint_cmd_id);
 }
 
 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
@@ -635,14 +635,14 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
     config.num_pages = cpu_to_le32(dev->num_pages);
     config.actual = cpu_to_le32(dev->actual);
 
-    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
-        config.free_page_report_cmd_id =
-                       cpu_to_le32(dev->free_page_report_cmd_id);
-    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
-        config.free_page_report_cmd_id =
+    if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
+        config.free_page_hint_cmd_id =
+                       cpu_to_le32(dev->free_page_hint_cmd_id);
+    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_STOP) {
+        config.free_page_hint_cmd_id =
                        cpu_to_le32(VIRTIO_BALLOON_CMD_ID_STOP);
-    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE) {
-        config.free_page_report_cmd_id =
+    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_DONE) {
+        config.free_page_hint_cmd_id =
                        cpu_to_le32(VIRTIO_BALLOON_CMD_ID_DONE);
     }
 
@@ -743,14 +743,14 @@ static int virtio_balloon_post_load_device(void *opaque, int version_id)
     return 0;
 }
 
-static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
+static const VMStateDescription vmstate_virtio_balloon_free_page_hint = {
     .name = "virtio-balloon-device/free-page-report",
     .version_id = 1,
     .minimum_version_id = 1,
     .needed = virtio_balloon_free_page_support,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
-        VMSTATE_UINT32(free_page_report_status, VirtIOBalloon),
+        VMSTATE_UINT32(free_page_hint_cmd_id, VirtIOBalloon),
+        VMSTATE_UINT32(free_page_hint_status, VirtIOBalloon),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -766,7 +766,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription * []) {
-        &vmstate_virtio_balloon_free_page_report,
+        &vmstate_virtio_balloon_free_page_hint,
         NULL
     }
 };
@@ -797,12 +797,12 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
                            VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
         s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
                                            virtio_balloon_handle_free_page_vq);
-        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
-        s->free_page_report_cmd_id =
-                           VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
-        s->free_page_report_notify.notify =
-                                       virtio_balloon_free_page_report_notify;
-        precopy_add_notifier(&s->free_page_report_notify);
+        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
+        s->free_page_hint_cmd_id =
+                           VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
+        s->free_page_hint_notify.notify =
+                                       virtio_balloon_free_page_hint_notify;
+        precopy_add_notifier(&s->free_page_hint_notify);
         if (s->iothread) {
             object_ref(OBJECT(s->iothread));
             s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
@@ -827,7 +827,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
     if (virtio_balloon_free_page_support(s)) {
         qemu_bh_delete(s->free_page_bh);
         virtio_balloon_free_page_stop(s);
-        precopy_remove_notifier(&s->free_page_report_notify);
+        precopy_remove_notifier(&s->free_page_hint_notify);
     }
     balloon_stats_destroy_timer(s);
     qemu_remove_balloon_handler(s);
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index d1c968d2376e..108cff97e71a 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -23,7 +23,7 @@
 #define VIRTIO_BALLOON(obj) \
         OBJECT_CHECK(VirtIOBalloon, (obj), TYPE_VIRTIO_BALLOON)
 
-#define VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN 0x80000000
+#define VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN 0x80000000
 
 typedef struct virtio_balloon_stat VirtIOBalloonStat;
 
@@ -33,20 +33,20 @@ typedef struct virtio_balloon_stat_modern {
        uint64_t val;
 } VirtIOBalloonStatModern;
 
-enum virtio_balloon_free_page_report_status {
-    FREE_PAGE_REPORT_S_STOP = 0,
-    FREE_PAGE_REPORT_S_REQUESTED = 1,
-    FREE_PAGE_REPORT_S_START = 2,
-    FREE_PAGE_REPORT_S_DONE = 3,
+enum virtio_balloon_free_page_hint_status {
+    FREE_PAGE_HINT_S_STOP = 0,
+    FREE_PAGE_HINT_S_REQUESTED = 1,
+    FREE_PAGE_HINT_S_START = 2,
+    FREE_PAGE_HINT_S_DONE = 3,
 };
 
 typedef struct VirtIOBalloon {
     VirtIODevice parent_obj;
     VirtQueue *ivq, *dvq, *svq, *free_page_vq;
-    uint32_t free_page_report_status;
+    uint32_t free_page_hint_status;
     uint32_t num_pages;
     uint32_t actual;
-    uint32_t free_page_report_cmd_id;
+    uint32_t free_page_hint_cmd_id;
     uint64_t stats[VIRTIO_BALLOON_S_NR];
     VirtQueueElement *stats_vq_elem;
     size_t stats_vq_offset;
@@ -55,7 +55,7 @@ typedef struct VirtIOBalloon {
     QEMUBH *free_page_bh;
     /*
      * Lock to synchronize threads to access the free page reporting related
-     * fields (e.g. free_page_report_status).
+     * fields (e.g. free_page_hint_status).
      */
     QemuMutex free_page_lock;
     QemuCond  free_page_cond;
@@ -64,7 +64,7 @@ typedef struct VirtIOBalloon {
      * stopped.
      */
     bool block_iothread;
-    NotifierWithReturn free_page_report_notify;
+    NotifierWithReturn free_page_hint_notify;
     int64_t stats_last_update;
     int64_t stats_poll_interval;
     uint32_t host_features;

[virtio-dev] [PATCH v21 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

In an upcoming patch a feature named Free Page Reporting is about to be
added. In order to avoid any confusion we should drop the use of the word
'report' when referring to Free Page Hinting. So what this patch does is go
through and replace all instances of 'report' with 'hint" when we are
referring to free page hinting.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 hw/virtio/virtio-balloon.c         |   74 ++++++++++++++++++------------------
 include/hw/virtio/virtio-balloon.h |   20 +++++-----
 2 files changed, 47 insertions(+), 47 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a4729f7fc930..a1d6fb52c876 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -466,21 +466,21 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
             ret = false;
             goto out;
         }
-        if (id == dev->free_page_report_cmd_id) {
-            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
+        if (id == dev->free_page_hint_cmd_id) {
+            dev->free_page_hint_status = FREE_PAGE_HINT_S_START;
         } else {
             /*
              * Stop the optimization only when it has started. This
              * avoids a stale stop sign for the previous command.
              */
-            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
-                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+            if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
+                dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
             }
         }
     }
 
     if (elem->in_num) {
-        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
+        if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
             qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
                                       elem->in_sg[0].iov_len);
         }
@@ -506,11 +506,11 @@ static void virtio_ballloon_get_free_page_hints(void *opaque)
         qemu_mutex_unlock(&dev->free_page_lock);
         virtio_notify(vdev, vq);
       /*
-       * Start to poll the vq once the reporting started. Otherwise, continue
+       * Start to poll the vq once the hinting started. Otherwise, continue
        * only when there are entries on the vq, which need to be given back.
        */
     } while (continue_to_get_hints ||
-             dev->free_page_report_status == FREE_PAGE_REPORT_S_START);
+             dev->free_page_hint_status == FREE_PAGE_HINT_S_START);
     virtio_queue_set_notification(vq, 1);
 }
 
@@ -531,14 +531,14 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
         return;
     }
 
-    if (s->free_page_report_cmd_id == UINT_MAX) {
-        s->free_page_report_cmd_id =
-                       VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
+    if (s->free_page_hint_cmd_id == UINT_MAX) {
+        s->free_page_hint_cmd_id =
+                       VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
     } else {
-        s->free_page_report_cmd_id++;
+        s->free_page_hint_cmd_id++;
     }
 
-    s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
+    s->free_page_hint_status = FREE_PAGE_HINT_S_REQUESTED;
     virtio_notify_config(vdev);
 }
 
@@ -546,18 +546,18 @@ static void virtio_balloon_free_page_stop(VirtIOBalloon *s)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-    if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
+    if (s->free_page_hint_status != FREE_PAGE_HINT_S_STOP) {
         /*
          * The lock also guarantees us that the
          * virtio_ballloon_get_free_page_hints exits after the
-         * free_page_report_status is set to S_STOP.
+         * free_page_hint_status is set to S_STOP.
          */
         qemu_mutex_lock(&s->free_page_lock);
         /*
          * The guest hasn't done the reporting, so host sends a notification
          * to the guest to actively stop the reporting.
          */
-        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
         qemu_mutex_unlock(&s->free_page_lock);
         virtio_notify_config(vdev);
     }
@@ -567,15 +567,15 @@ static void virtio_balloon_free_page_done(VirtIOBalloon *s)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-    s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
+    s->free_page_hint_status = FREE_PAGE_HINT_S_DONE;
     virtio_notify_config(vdev);
 }
 
 static int
-virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
+virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
 {
     VirtIOBalloon *dev = container_of(n, VirtIOBalloon,
-                                      free_page_report_notify);
+                                      free_page_hint_notify);
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     PrecopyNotifyData *pnd = data;
 
@@ -624,7 +624,7 @@ static size_t virtio_balloon_config_size(VirtIOBalloon *s)
     if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
         return offsetof(struct virtio_balloon_config, poison_val);
     }
-    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
+    return offsetof(struct virtio_balloon_config, free_page_hint_cmd_id);
 }
 
 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
@@ -635,14 +635,14 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
     config.num_pages = cpu_to_le32(dev->num_pages);
     config.actual = cpu_to_le32(dev->actual);
 
-    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
-        config.free_page_report_cmd_id =
-                       cpu_to_le32(dev->free_page_report_cmd_id);
-    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
-        config.free_page_report_cmd_id =
+    if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
+        config.free_page_hint_cmd_id =
+                       cpu_to_le32(dev->free_page_hint_cmd_id);
+    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_STOP) {
+        config.free_page_hint_cmd_id =
                        cpu_to_le32(VIRTIO_BALLOON_CMD_ID_STOP);
-    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE) {
-        config.free_page_report_cmd_id =
+    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_DONE) {
+        config.free_page_hint_cmd_id =
                        cpu_to_le32(VIRTIO_BALLOON_CMD_ID_DONE);
     }
 
@@ -743,14 +743,14 @@ static int virtio_balloon_post_load_device(void *opaque, int version_id)
     return 0;
 }
 
-static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
+static const VMStateDescription vmstate_virtio_balloon_free_page_hint = {
     .name = "virtio-balloon-device/free-page-report",
     .version_id = 1,
     .minimum_version_id = 1,
     .needed = virtio_balloon_free_page_support,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
-        VMSTATE_UINT32(free_page_report_status, VirtIOBalloon),
+        VMSTATE_UINT32(free_page_hint_cmd_id, VirtIOBalloon),
+        VMSTATE_UINT32(free_page_hint_status, VirtIOBalloon),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -766,7 +766,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription * []) {
-        &vmstate_virtio_balloon_free_page_report,
+        &vmstate_virtio_balloon_free_page_hint,
         NULL
     }
 };
@@ -797,12 +797,12 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
                            VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
         s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
                                            virtio_balloon_handle_free_page_vq);
-        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
-        s->free_page_report_cmd_id =
-                           VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
-        s->free_page_report_notify.notify =
-                                       virtio_balloon_free_page_report_notify;
-        precopy_add_notifier(&s->free_page_report_notify);
+        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
+        s->free_page_hint_cmd_id =
+                           VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
+        s->free_page_hint_notify.notify =
+                                       virtio_balloon_free_page_hint_notify;
+        precopy_add_notifier(&s->free_page_hint_notify);
         if (s->iothread) {
             object_ref(OBJECT(s->iothread));
             s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
@@ -827,7 +827,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
     if (virtio_balloon_free_page_support(s)) {
         qemu_bh_delete(s->free_page_bh);
         virtio_balloon_free_page_stop(s);
-        precopy_remove_notifier(&s->free_page_report_notify);
+        precopy_remove_notifier(&s->free_page_hint_notify);
     }
     balloon_stats_destroy_timer(s);
     qemu_remove_balloon_handler(s);
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index d1c968d2376e..108cff97e71a 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -23,7 +23,7 @@
 #define VIRTIO_BALLOON(obj) \
         OBJECT_CHECK(VirtIOBalloon, (obj), TYPE_VIRTIO_BALLOON)
 
-#define VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN 0x80000000
+#define VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN 0x80000000
 
 typedef struct virtio_balloon_stat VirtIOBalloonStat;
 
@@ -33,20 +33,20 @@ typedef struct virtio_balloon_stat_modern {
        uint64_t val;
 } VirtIOBalloonStatModern;
 
-enum virtio_balloon_free_page_report_status {
-    FREE_PAGE_REPORT_S_STOP = 0,
-    FREE_PAGE_REPORT_S_REQUESTED = 1,
-    FREE_PAGE_REPORT_S_START = 2,
-    FREE_PAGE_REPORT_S_DONE = 3,
+enum virtio_balloon_free_page_hint_status {
+    FREE_PAGE_HINT_S_STOP = 0,
+    FREE_PAGE_HINT_S_REQUESTED = 1,
+    FREE_PAGE_HINT_S_START = 2,
+    FREE_PAGE_HINT_S_DONE = 3,
 };
 
 typedef struct VirtIOBalloon {
     VirtIODevice parent_obj;
     VirtQueue *ivq, *dvq, *svq, *free_page_vq;
-    uint32_t free_page_report_status;
+    uint32_t free_page_hint_status;
     uint32_t num_pages;
     uint32_t actual;
-    uint32_t free_page_report_cmd_id;
+    uint32_t free_page_hint_cmd_id;
     uint64_t stats[VIRTIO_BALLOON_S_NR];
     VirtQueueElement *stats_vq_elem;
     size_t stats_vq_offset;
@@ -55,7 +55,7 @@ typedef struct VirtIOBalloon {
     QEMUBH *free_page_bh;
     /*
      * Lock to synchronize threads to access the free page reporting related
-     * fields (e.g. free_page_report_status).
+     * fields (e.g. free_page_hint_status).
      */
     QemuMutex free_page_lock;
     QemuCond  free_page_cond;
@@ -64,7 +64,7 @@ typedef struct VirtIOBalloon {
      * stopped.
      */
     bool block_iothread;
-    NotifierWithReturn free_page_report_notify;
+    NotifierWithReturn free_page_hint_notify;
     int64_t stats_last_update;
     int64_t stats_poll_interval;
     uint32_t host_features;


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

We need to make certain to advertise support for page poison tracking if
we want to actually get data on if the guest will be poisoning pages.

Add a value for tracking the poison value being used if page poisoning is
enabled. With this we can determine if we will need to skip page reporting
when it is enabled in the future.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 hw/virtio/virtio-balloon.c         |    7 +++++++
 include/hw/virtio/virtio-balloon.h |    1 +
 2 files changed, 8 insertions(+)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a1d6fb52c876..5effc8b4653b 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
 
     config.num_pages = cpu_to_le32(dev->num_pages);
     config.actual = cpu_to_le32(dev->actual);
+    config.poison_val = cpu_to_le32(dev->poison_val);
 
     if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
         config.free_page_hint_cmd_id =
@@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
         qapi_event_send_balloon_change(vm_ram_size -
                         ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
     }
+    dev->poison_val = 0;
+    if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
+        dev->poison_val = le32_to_cpu(config.poison_val);
+    }
     trace_virtio_balloon_set_config(dev->actual, oldactual);
 }
 
@@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
         g_free(s->stats_vq_elem);
         s->stats_vq_elem = NULL;
     }
+
+    s->poison_val = 0;
 }
 
 static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index 108cff97e71a..3ca2a78e1aca 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -70,6 +70,7 @@ typedef struct VirtIOBalloon {
     uint32_t host_features;
 
     bool qemu_4_0_config_size;
+    uint32_t poison_val;
 } VirtIOBalloon;
 
 #endif

[virtio-dev] [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

We need to make certain to advertise support for page poison tracking if
we want to actually get data on if the guest will be poisoning pages.

Add a value for tracking the poison value being used if page poisoning is
enabled. With this we can determine if we will need to skip page reporting
when it is enabled in the future.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 hw/virtio/virtio-balloon.c         |    7 +++++++
 include/hw/virtio/virtio-balloon.h |    1 +
 2 files changed, 8 insertions(+)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a1d6fb52c876..5effc8b4653b 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
 
     config.num_pages = cpu_to_le32(dev->num_pages);
     config.actual = cpu_to_le32(dev->actual);
+    config.poison_val = cpu_to_le32(dev->poison_val);
 
     if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
         config.free_page_hint_cmd_id =
@@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
         qapi_event_send_balloon_change(vm_ram_size -
                         ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
     }
+    dev->poison_val = 0;
+    if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
+        dev->poison_val = le32_to_cpu(config.poison_val);
+    }
     trace_virtio_balloon_set_config(dev->actual, oldactual);
 }
 
@@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
         g_free(s->stats_vq_elem);
         s->stats_vq_elem = NULL;
     }
+
+    s->poison_val = 0;
 }
 
 static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index 108cff97e71a..3ca2a78e1aca 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -70,6 +70,7 @@ typedef struct VirtIOBalloon {
     uint32_t host_features;
 
     bool qemu_4_0_config_size;
+    uint32_t poison_val;
 } VirtIOBalloon;
 
 #endif


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[PATCH v21 QEMU 5/5] virtio-balloon: Provide an interface for free page reporting

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Add support for free page reporting. The idea is to function very similar
to how the balloon works in that we basically end up madvising the page as
not being used. However we don't really need to bother with any deflate
type logic since the page will be faulted back into the guest when it is
read or written to.

This provides a new way of letting the guest proactively report free
pages to the hypervisor, so the hypervisor can reuse them. In contrast to
inflate/deflate that is triggered via the hypervisor explicitly.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 hw/virtio/virtio-balloon.c         |   70 ++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-balloon.h |    2 +
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 5effc8b4653b..b473ff7f4b88 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -321,6 +321,60 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
     balloon_stats_change_timer(s, 0);
 }
 
+static void virtio_balloon_handle_report(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
+    VirtQueueElement *elem;
+
+    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
+        unsigned int i;
+
+        if (qemu_balloon_is_inhibited() || dev->poison_val) {
+            goto skip_element;
+        }
+
+        for (i = 0; i < elem->in_num; i++) {
+            void *addr = elem->in_sg[i].iov_base;
+            size_t size = elem->in_sg[i].iov_len;
+            ram_addr_t ram_offset;
+            RAMBlock *rb;
+
+            /*
+             * There is no need to check the memory section to see if
+             * it is ram/readonly/romd like there is for handle_output
+             * below. If the region is not meant to be written to then
+             * address_space_map will have allocated a bounce buffer
+             * and it will be freed in address_space_unmap and trigger
+             * and unassigned_mem_write before failing to copy over the
+             * buffer. If more than one bad descriptor is provided it
+             * will return NULL after the first bounce buffer and fail
+             * to map any resources.
+             */
+            rb = qemu_ram_block_from_host(addr, false, &ram_offset);
+            if (!rb) {
+                trace_virtio_balloon_bad_addr(elem->in_addr[i]);
+                continue;
+            }
+
+            /*
+             * For now we will simply ignore unaligned memory regions, or
+             * regions that overrun the end of the RAMBlock.
+             */
+            if (!QEMU_IS_ALIGNED(ram_offset | size, qemu_ram_pagesize(rb)) ||
+                (ram_offset + size) > qemu_ram_get_used_length(rb)) {
+                continue;
+            }
+
+            ram_block_discard_range(rb, ram_offset, size);
+        }
+
+skip_element:
+        virtqueue_push(vq, elem, 0);
+        virtio_notify(vdev, vq);
+        g_free(elem);
+    }
+}
+
 static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
@@ -782,6 +836,16 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
     VirtIOBalloon *s = VIRTIO_BALLOON(dev);
     int ret;
 
+    /*
+     * Page reporting is dependant on page poison to make sure we can
+     * report a page without changing the state of the internal data.
+     * We need to set the flag before we call virtio_init as it will
+     * affect the config size of the vdev.
+     */
+    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
+        s->host_features |= 1 << VIRTIO_BALLOON_F_PAGE_POISON;
+    }
+
     virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
                 virtio_balloon_config_size(s));
 
@@ -798,6 +862,10 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
     s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
     s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
 
+    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
+        s->rvq = virtio_add_queue(vdev, 32, virtio_balloon_handle_report);
+    }
+
     if (virtio_has_feature(s->host_features,
                            VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
         s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
@@ -923,6 +991,8 @@ static Property virtio_balloon_properties[] = {
                     VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
     DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
                     VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
+    DEFINE_PROP_BIT("free-page-reporting", VirtIOBalloon, host_features,
+                    VIRTIO_BALLOON_F_REPORTING, true),
     /* QEMU 4.0 accidentally changed the config size even when free-page-hint
      * is disabled, resulting in QEMU 3.1 migration incompatibility.  This
      * property retains this quirk for QEMU 4.1 machine types.
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index 3ca2a78e1aca..ac4013d51010 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -42,7 +42,7 @@ enum virtio_balloon_free_page_hint_status {
 
 typedef struct VirtIOBalloon {
     VirtIODevice parent_obj;
-    VirtQueue *ivq, *dvq, *svq, *free_page_vq;
+    VirtQueue *ivq, *dvq, *svq, *free_page_vq, *rvq;
     uint32_t free_page_hint_status;
     uint32_t num_pages;
     uint32_t actual;

[virtio-dev] [PATCH v21 QEMU 5/5] virtio-balloon: Provide an interface for free page reporting

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Add support for free page reporting. The idea is to function very similar
to how the balloon works in that we basically end up madvising the page as
not being used. However we don't really need to bother with any deflate
type logic since the page will be faulted back into the guest when it is
read or written to.

This provides a new way of letting the guest proactively report free
pages to the hypervisor, so the hypervisor can reuse them. In contrast to
inflate/deflate that is triggered via the hypervisor explicitly.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 hw/virtio/virtio-balloon.c         |   70 ++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-balloon.h |    2 +
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 5effc8b4653b..b473ff7f4b88 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -321,6 +321,60 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
     balloon_stats_change_timer(s, 0);
 }
 
+static void virtio_balloon_handle_report(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
+    VirtQueueElement *elem;
+
+    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
+        unsigned int i;
+
+        if (qemu_balloon_is_inhibited() || dev->poison_val) {
+            goto skip_element;
+        }
+
+        for (i = 0; i < elem->in_num; i++) {
+            void *addr = elem->in_sg[i].iov_base;
+            size_t size = elem->in_sg[i].iov_len;
+            ram_addr_t ram_offset;
+            RAMBlock *rb;
+
+            /*
+             * There is no need to check the memory section to see if
+             * it is ram/readonly/romd like there is for handle_output
+             * below. If the region is not meant to be written to then
+             * address_space_map will have allocated a bounce buffer
+             * and it will be freed in address_space_unmap and trigger
+             * and unassigned_mem_write before failing to copy over the
+             * buffer. If more than one bad descriptor is provided it
+             * will return NULL after the first bounce buffer and fail
+             * to map any resources.
+             */
+            rb = qemu_ram_block_from_host(addr, false, &ram_offset);
+            if (!rb) {
+                trace_virtio_balloon_bad_addr(elem->in_addr[i]);
+                continue;
+            }
+
+            /*
+             * For now we will simply ignore unaligned memory regions, or
+             * regions that overrun the end of the RAMBlock.
+             */
+            if (!QEMU_IS_ALIGNED(ram_offset | size, qemu_ram_pagesize(rb)) ||
+                (ram_offset + size) > qemu_ram_get_used_length(rb)) {
+                continue;
+            }
+
+            ram_block_discard_range(rb, ram_offset, size);
+        }
+
+skip_element:
+        virtqueue_push(vq, elem, 0);
+        virtio_notify(vdev, vq);
+        g_free(elem);
+    }
+}
+
 static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
@@ -782,6 +836,16 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
     VirtIOBalloon *s = VIRTIO_BALLOON(dev);
     int ret;
 
+    /*
+     * Page reporting is dependant on page poison to make sure we can
+     * report a page without changing the state of the internal data.
+     * We need to set the flag before we call virtio_init as it will
+     * affect the config size of the vdev.
+     */
+    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
+        s->host_features |= 1 << VIRTIO_BALLOON_F_PAGE_POISON;
+    }
+
     virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
                 virtio_balloon_config_size(s));
 
@@ -798,6 +862,10 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
     s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
     s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
 
+    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
+        s->rvq = virtio_add_queue(vdev, 32, virtio_balloon_handle_report);
+    }
+
     if (virtio_has_feature(s->host_features,
                            VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
         s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
@@ -923,6 +991,8 @@ static Property virtio_balloon_properties[] = {
                     VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
     DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
                     VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
+    DEFINE_PROP_BIT("free-page-reporting", VirtIOBalloon, host_features,
+                    VIRTIO_BALLOON_F_REPORTING, true),
     /* QEMU 4.0 accidentally changed the config size even when free-page-hint
      * is disabled, resulting in QEMU 3.1 migration incompatibility.  This
      * property retains this quirk for QEMU 4.1 machine types.
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index 3ca2a78e1aca..ac4013d51010 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -42,7 +42,7 @@ enum virtio_balloon_free_page_hint_status {
 
 typedef struct VirtIOBalloon {
     VirtIODevice parent_obj;
-    VirtQueue *ivq, *dvq, *svq, *free_page_vq;
+    VirtQueue *ivq, *dvq, *svq, *free_page_vq, *rvq;
     uint32_t free_page_hint_status;
     uint32_t num_pages;
     uint32_t actual;


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[David Hildenbrand]

On 22.04.20 20:21, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> We need to make certain to advertise support for page poison tracking if
> we want to actually get data on if the guest will be poisoning pages.
> 
> Add a value for tracking the poison value being used if page poisoning is
> enabled. With this we can determine if we will need to skip page reporting
> when it is enabled in the future.

Maybe add something about the semantics

"VIRTIO_BALLOON_F_PAGE_POISON will not change the behavior of free page
hinting or ordinary balloon inflation/deflation."

I do wonder if we should just unconditionally enable
VIRTIO_BALLOON_F_PAGE_POISON here, gluing it to the QEMU compat machine
(via a property that is default-enabled, and disabled from compat machines).

Because, as Michael said, knowing that the guest is using page poisoning
might be interesting even if free page reporting is not around.

> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  hw/virtio/virtio-balloon.c         |    7 +++++++
>  include/hw/virtio/virtio-balloon.h |    1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a1d6fb52c876..5effc8b4653b 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>  
>      config.num_pages = cpu_to_le32(dev->num_pages);
>      config.actual = cpu_to_le32(dev->actual);
> +    config.poison_val = cpu_to_le32(dev->poison_val);
>  
>      if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
>          config.free_page_hint_cmd_id =
> @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>          qapi_event_send_balloon_change(vm_ram_size -
>                          ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
>      }
> +    dev->poison_val = 0;
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> +        dev->poison_val = le32_to_cpu(config.poison_val);
> +    }
>      trace_virtio_balloon_set_config(dev->actual, oldactual);
>  }
>  
> @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
>          g_free(s->stats_vq_elem);
>          s->stats_vq_elem = NULL;
>      }
> +
> +    s->poison_val = 0;
>  }
>  
>  static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index 108cff97e71a..3ca2a78e1aca 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon {
>      uint32_t host_features;
>  
>      bool qemu_4_0_config_size;
> +    uint32_t poison_val;
>  } VirtIOBalloon;
>  
>  #endif
> 

You still have to migrate poison_val if I am not wrong, otherwise you
would lose it during migration if I am not mistaking.

-- 
Thanks,

David / dhildenb

[virtio-dev] Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[David Hildenbrand]

On 22.04.20 20:21, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> We need to make certain to advertise support for page poison tracking if
> we want to actually get data on if the guest will be poisoning pages.
> 
> Add a value for tracking the poison value being used if page poisoning is
> enabled. With this we can determine if we will need to skip page reporting
> when it is enabled in the future.

Maybe add something about the semantics

"VIRTIO_BALLOON_F_PAGE_POISON will not change the behavior of free page
hinting or ordinary balloon inflation/deflation."

I do wonder if we should just unconditionally enable
VIRTIO_BALLOON_F_PAGE_POISON here, gluing it to the QEMU compat machine
(via a property that is default-enabled, and disabled from compat machines).

Because, as Michael said, knowing that the guest is using page poisoning
might be interesting even if free page reporting is not around.

> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  hw/virtio/virtio-balloon.c         |    7 +++++++
>  include/hw/virtio/virtio-balloon.h |    1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a1d6fb52c876..5effc8b4653b 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>  
>      config.num_pages = cpu_to_le32(dev->num_pages);
>      config.actual = cpu_to_le32(dev->actual);
> +    config.poison_val = cpu_to_le32(dev->poison_val);
>  
>      if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
>          config.free_page_hint_cmd_id =
> @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>          qapi_event_send_balloon_change(vm_ram_size -
>                          ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
>      }
> +    dev->poison_val = 0;
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> +        dev->poison_val = le32_to_cpu(config.poison_val);
> +    }
>      trace_virtio_balloon_set_config(dev->actual, oldactual);
>  }
>  
> @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
>          g_free(s->stats_vq_elem);
>          s->stats_vq_elem = NULL;
>      }
> +
> +    s->poison_val = 0;
>  }
>  
>  static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index 108cff97e71a..3ca2a78e1aca 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon {
>      uint32_t host_features;
>  
>      bool qemu_4_0_config_size;
> +    uint32_t poison_val;
>  } VirtIOBalloon;
>  
>  #endif
> 

You still have to migrate poison_val if I am not wrong, otherwise you
would lose it during migration if I am not mistaking.

-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[Alexander Duyck]

On Thu, Apr 23, 2020 at 1:11 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 22.04.20 20:21, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > We need to make certain to advertise support for page poison tracking if
> > we want to actually get data on if the guest will be poisoning pages.
> >
> > Add a value for tracking the poison value being used if page poisoning is
> > enabled. With this we can determine if we will need to skip page reporting
> > when it is enabled in the future.
>
> Maybe add something about the semantics
>
> "VIRTIO_BALLOON_F_PAGE_POISON will not change the behavior of free page
> hinting or ordinary balloon inflation/deflation."
>
> I do wonder if we should just unconditionally enable
> VIRTIO_BALLOON_F_PAGE_POISON here, gluing it to the QEMU compat machine
> (via a property that is default-enabled, and disabled from compat machines).
>
> Because, as Michael said, knowing that the guest is using page poisoning
> might be interesting even if free page reporting is not around.

I could do that. So if that is the case though would I disable page
reporting if it isn't enabled, or would I be enabling page poison if
page reporting is enabled?

> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  hw/virtio/virtio-balloon.c         |    7 +++++++
> >  include/hw/virtio/virtio-balloon.h |    1 +
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index a1d6fb52c876..5effc8b4653b 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> >
> >      config.num_pages = cpu_to_le32(dev->num_pages);
> >      config.actual = cpu_to_le32(dev->actual);
> > +    config.poison_val = cpu_to_le32(dev->poison_val);
> >
> >      if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
> >          config.free_page_hint_cmd_id =
> > @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
> >          qapi_event_send_balloon_change(vm_ram_size -
> >                          ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
> >      }
> > +    dev->poison_val = 0;
> > +    if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> > +        dev->poison_val = le32_to_cpu(config.poison_val);
> > +    }
> >      trace_virtio_balloon_set_config(dev->actual, oldactual);
> >  }
> >
> > @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
> >          g_free(s->stats_vq_elem);
> >          s->stats_vq_elem = NULL;
> >      }
> > +
> > +    s->poison_val = 0;
> >  }
> >
> >  static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> > index 108cff97e71a..3ca2a78e1aca 100644
> > --- a/include/hw/virtio/virtio-balloon.h
> > +++ b/include/hw/virtio/virtio-balloon.h
> > @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon {
> >      uint32_t host_features;
> >
> >      bool qemu_4_0_config_size;
> > +    uint32_t poison_val;
> >  } VirtIOBalloon;
> >
> >  #endif
> >
>
> You still have to migrate poison_val if I am not wrong, otherwise you
> would lose it during migration if I am not mistaking.

So what are the requirements to migrate a value? Would I need to add a
property so it can be retrieved/set?

[virtio-dev] Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[Alexander Duyck]

On Thu, Apr 23, 2020 at 1:11 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 22.04.20 20:21, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > We need to make certain to advertise support for page poison tracking if
> > we want to actually get data on if the guest will be poisoning pages.
> >
> > Add a value for tracking the poison value being used if page poisoning is
> > enabled. With this we can determine if we will need to skip page reporting
> > when it is enabled in the future.
>
> Maybe add something about the semantics
>
> "VIRTIO_BALLOON_F_PAGE_POISON will not change the behavior of free page
> hinting or ordinary balloon inflation/deflation."
>
> I do wonder if we should just unconditionally enable
> VIRTIO_BALLOON_F_PAGE_POISON here, gluing it to the QEMU compat machine
> (via a property that is default-enabled, and disabled from compat machines).
>
> Because, as Michael said, knowing that the guest is using page poisoning
> might be interesting even if free page reporting is not around.

I could do that. So if that is the case though would I disable page
reporting if it isn't enabled, or would I be enabling page poison if
page reporting is enabled?

> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  hw/virtio/virtio-balloon.c         |    7 +++++++
> >  include/hw/virtio/virtio-balloon.h |    1 +
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index a1d6fb52c876..5effc8b4653b 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> >
> >      config.num_pages = cpu_to_le32(dev->num_pages);
> >      config.actual = cpu_to_le32(dev->actual);
> > +    config.poison_val = cpu_to_le32(dev->poison_val);
> >
> >      if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
> >          config.free_page_hint_cmd_id =
> > @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
> >          qapi_event_send_balloon_change(vm_ram_size -
> >                          ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
> >      }
> > +    dev->poison_val = 0;
> > +    if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> > +        dev->poison_val = le32_to_cpu(config.poison_val);
> > +    }
> >      trace_virtio_balloon_set_config(dev->actual, oldactual);
> >  }
> >
> > @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
> >          g_free(s->stats_vq_elem);
> >          s->stats_vq_elem = NULL;
> >      }
> > +
> > +    s->poison_val = 0;
> >  }
> >
> >  static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> > index 108cff97e71a..3ca2a78e1aca 100644
> > --- a/include/hw/virtio/virtio-balloon.h
> > +++ b/include/hw/virtio/virtio-balloon.h
> > @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon {
> >      uint32_t host_features;
> >
> >      bool qemu_4_0_config_size;
> > +    uint32_t poison_val;
> >  } VirtIOBalloon;
> >
> >  #endif
> >
>
> You still have to migrate poison_val if I am not wrong, otherwise you
> would lose it during migration if I am not mistaking.

So what are the requirements to migrate a value? Would I need to add a
property so it can be retrieved/set?

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[David Hildenbrand]

On 23.04.20 16:46, Alexander Duyck wrote:
> On Thu, Apr 23, 2020 at 1:11 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 22.04.20 20:21, Alexander Duyck wrote:
>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>
>>> We need to make certain to advertise support for page poison tracking if
>>> we want to actually get data on if the guest will be poisoning pages.
>>>
>>> Add a value for tracking the poison value being used if page poisoning is
>>> enabled. With this we can determine if we will need to skip page reporting
>>> when it is enabled in the future.
>>
>> Maybe add something about the semantics
>>
>> "VIRTIO_BALLOON_F_PAGE_POISON will not change the behavior of free page
>> hinting or ordinary balloon inflation/deflation."
>>
>> I do wonder if we should just unconditionally enable
>> VIRTIO_BALLOON_F_PAGE_POISON here, gluing it to the QEMU compat machine
>> (via a property that is default-enabled, and disabled from compat machines).
>>
>> Because, as Michael said, knowing that the guest is using page poisoning
>> might be interesting even if free page reporting is not around.
> 
> I could do that. So if that is the case though would I disable page
> reporting if it isn't enabled, or would I be enabling page poison if
> page reporting is enabled?


So, I would suggest this the following as a diff to this patch (the TODO part as a
separate patch - we will have these compat properties briefly after the 5.0
release)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index c1a444cb75..2e96cba4ff 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -28,6 +28,12 @@
 #include "hw/mem/nvdimm.h"
 #include "migration/vmstate.h"
 
+// TODO: After 5.0 release
+GlobalProperty hw_compat_5_0[] = {
+    { "virtio-balloon-device", "page-hint", "false"},
+};
+const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
+
 GlobalProperty hw_compat_4_2[] = {
     { "virtio-blk-device", "queue-size", "128"},
     { "virtio-scsi-device", "virtqueue_size", "128"},
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a4729f7fc9..5ff8a669cf 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -924,6 +924,8 @@ static Property virtio_balloon_properties[] = {
                      qemu_4_0_config_size, false),
     DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
                      IOThread *),
+    DEFINE_PROP_BIT("page-poison", VirtIOBalloon, host_features,
+                    VIRTIO_BALLOON_F_PAGE_POISON, true),
     DEFINE_PROP_END_OF_LIST(),
 };


What to do with page reporting depends: I would not implicitly enable features.
I think we are talking about

-device virtio-balloon-pci,...,page-poison=false,free-page-reporting=true

a) Valid scenario. Fix Linux guests as we discussed to not use reporting if they rely on page poisoning.
b) Invalid scenario. E.g., bail out when trying to realize that device ("'free-page-reporting' requires 'page-poison'").

With new QEMU machines, this should not happen with

-device virtio-balloon-pci,...,free-page-reporting=true

as page-poison=true is the new default.

What's your opinion?

> 
>>>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>> ---
>>>  hw/virtio/virtio-balloon.c         |    7 +++++++
>>>  include/hw/virtio/virtio-balloon.h |    1 +
>>>  2 files changed, 8 insertions(+)
>>>
>>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>>> index a1d6fb52c876..5effc8b4653b 100644
>>> --- a/hw/virtio/virtio-balloon.c
>>> +++ b/hw/virtio/virtio-balloon.c
>>> @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>>>
>>>      config.num_pages = cpu_to_le32(dev->num_pages);
>>>      config.actual = cpu_to_le32(dev->actual);
>>> +    config.poison_val = cpu_to_le32(dev->poison_val);
>>>
>>>      if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
>>>          config.free_page_hint_cmd_id =
>>> @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>>>          qapi_event_send_balloon_change(vm_ram_size -
>>>                          ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
>>>      }
>>> +    dev->poison_val = 0;
>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
>>> +        dev->poison_val = le32_to_cpu(config.poison_val);
>>> +    }
>>>      trace_virtio_balloon_set_config(dev->actual, oldactual);
>>>  }
>>>
>>> @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
>>>          g_free(s->stats_vq_elem);
>>>          s->stats_vq_elem = NULL;
>>>      }
>>> +
>>> +    s->poison_val = 0;
>>>  }
>>>
>>>  static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
>>> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
>>> index 108cff97e71a..3ca2a78e1aca 100644
>>> --- a/include/hw/virtio/virtio-balloon.h
>>> +++ b/include/hw/virtio/virtio-balloon.h
>>> @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon {
>>>      uint32_t host_features;
>>>
>>>      bool qemu_4_0_config_size;
>>> +    uint32_t poison_val;
>>>  } VirtIOBalloon;
>>>
>>>  #endif
>>>
>>
>> You still have to migrate poison_val if I am not wrong, otherwise you
>> would lose it during migration if I am not mistaking.
> 
> So what are the requirements to migrate a value? Would I need to add a
> property so it can be retrieved/set?
> 

Something like this would do the trick:

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a4729f7fc9..ea0afec5f6 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -522,6 +522,13 @@ static bool virtio_balloon_free_page_support(void *opaque)
     return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
 }
 
+static bool virtio_balloon_page_poison_support(void *opaque)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
+
+    return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
+}
+
 static void virtio_balloon_free_page_start(VirtIOBalloon *s)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
@@ -755,6 +762,17 @@ static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
     }
 };
 
+static const VMStateDescription vmstate_virtio_balloon_page_poison = {
+    .name = "vitio-balloon-device/page-poison",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = virtio_balloon_page_poison_support,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(poison_val, VirtIOBalloon),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_virtio_balloon_device = {
     .name = "virtio-balloon-device",
     .version_id = 1,
@@ -767,6 +785,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
     },
     .subsections = (const VMStateDescription * []) {
         &vmstate_virtio_balloon_free_page_report,
+        &vmstate_virtio_balloon_page_poison,
         NULL
     }
 };




But I *think* the following should work as well IIRC (could be that migrating new QEMU
-> old QEMU would be an issue, I don't recall if both directions are safe):


diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a4729f7fc9..01bccf26fb 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -757,12 +757,13 @@ static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
 
 static const VMStateDescription vmstate_virtio_balloon_device = {
     .name = "virtio-balloon-device",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .post_load = virtio_balloon_post_load_device,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(num_pages, VirtIOBalloon),
         VMSTATE_UINT32(actual, VirtIOBalloon),
+        VMSTATE_UINT32_V(poison_val, VirtIOBalloon, 2),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription * []) {


-- 
Thanks,

David / dhildenb

[virtio-dev] Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[David Hildenbrand]

On 23.04.20 16:46, Alexander Duyck wrote:
> On Thu, Apr 23, 2020 at 1:11 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 22.04.20 20:21, Alexander Duyck wrote:
>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>
>>> We need to make certain to advertise support for page poison tracking if
>>> we want to actually get data on if the guest will be poisoning pages.
>>>
>>> Add a value for tracking the poison value being used if page poisoning is
>>> enabled. With this we can determine if we will need to skip page reporting
>>> when it is enabled in the future.
>>
>> Maybe add something about the semantics
>>
>> "VIRTIO_BALLOON_F_PAGE_POISON will not change the behavior of free page
>> hinting or ordinary balloon inflation/deflation."
>>
>> I do wonder if we should just unconditionally enable
>> VIRTIO_BALLOON_F_PAGE_POISON here, gluing it to the QEMU compat machine
>> (via a property that is default-enabled, and disabled from compat machines).
>>
>> Because, as Michael said, knowing that the guest is using page poisoning
>> might be interesting even if free page reporting is not around.
> 
> I could do that. So if that is the case though would I disable page
> reporting if it isn't enabled, or would I be enabling page poison if
> page reporting is enabled?


So, I would suggest this the following as a diff to this patch (the TODO part as a
separate patch - we will have these compat properties briefly after the 5.0
release)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index c1a444cb75..2e96cba4ff 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -28,6 +28,12 @@
 #include "hw/mem/nvdimm.h"
 #include "migration/vmstate.h"
 
+// TODO: After 5.0 release
+GlobalProperty hw_compat_5_0[] = {
+    { "virtio-balloon-device", "page-hint", "false"},
+};
+const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
+
 GlobalProperty hw_compat_4_2[] = {
     { "virtio-blk-device", "queue-size", "128"},
     { "virtio-scsi-device", "virtqueue_size", "128"},
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a4729f7fc9..5ff8a669cf 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -924,6 +924,8 @@ static Property virtio_balloon_properties[] = {
                      qemu_4_0_config_size, false),
     DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
                      IOThread *),
+    DEFINE_PROP_BIT("page-poison", VirtIOBalloon, host_features,
+                    VIRTIO_BALLOON_F_PAGE_POISON, true),
     DEFINE_PROP_END_OF_LIST(),
 };


What to do with page reporting depends: I would not implicitly enable features.
I think we are talking about

-device virtio-balloon-pci,...,page-poison=false,free-page-reporting=true

a) Valid scenario. Fix Linux guests as we discussed to not use reporting if they rely on page poisoning.
b) Invalid scenario. E.g., bail out when trying to realize that device ("'free-page-reporting' requires 'page-poison'").

With new QEMU machines, this should not happen with

-device virtio-balloon-pci,...,free-page-reporting=true

as page-poison=true is the new default.

What's your opinion?

> 
>>>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>> ---
>>>  hw/virtio/virtio-balloon.c         |    7 +++++++
>>>  include/hw/virtio/virtio-balloon.h |    1 +
>>>  2 files changed, 8 insertions(+)
>>>
>>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>>> index a1d6fb52c876..5effc8b4653b 100644
>>> --- a/hw/virtio/virtio-balloon.c
>>> +++ b/hw/virtio/virtio-balloon.c
>>> @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>>>
>>>      config.num_pages = cpu_to_le32(dev->num_pages);
>>>      config.actual = cpu_to_le32(dev->actual);
>>> +    config.poison_val = cpu_to_le32(dev->poison_val);
>>>
>>>      if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
>>>          config.free_page_hint_cmd_id =
>>> @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>>>          qapi_event_send_balloon_change(vm_ram_size -
>>>                          ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
>>>      }
>>> +    dev->poison_val = 0;
>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
>>> +        dev->poison_val = le32_to_cpu(config.poison_val);
>>> +    }
>>>      trace_virtio_balloon_set_config(dev->actual, oldactual);
>>>  }
>>>
>>> @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
>>>          g_free(s->stats_vq_elem);
>>>          s->stats_vq_elem = NULL;
>>>      }
>>> +
>>> +    s->poison_val = 0;
>>>  }
>>>
>>>  static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
>>> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
>>> index 108cff97e71a..3ca2a78e1aca 100644
>>> --- a/include/hw/virtio/virtio-balloon.h
>>> +++ b/include/hw/virtio/virtio-balloon.h
>>> @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon {
>>>      uint32_t host_features;
>>>
>>>      bool qemu_4_0_config_size;
>>> +    uint32_t poison_val;
>>>  } VirtIOBalloon;
>>>
>>>  #endif
>>>
>>
>> You still have to migrate poison_val if I am not wrong, otherwise you
>> would lose it during migration if I am not mistaking.
> 
> So what are the requirements to migrate a value? Would I need to add a
> property so it can be retrieved/set?
> 

Something like this would do the trick:

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a4729f7fc9..ea0afec5f6 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -522,6 +522,13 @@ static bool virtio_balloon_free_page_support(void *opaque)
     return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
 }
 
+static bool virtio_balloon_page_poison_support(void *opaque)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
+
+    return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
+}
+
 static void virtio_balloon_free_page_start(VirtIOBalloon *s)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
@@ -755,6 +762,17 @@ static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
     }
 };
 
+static const VMStateDescription vmstate_virtio_balloon_page_poison = {
+    .name = "vitio-balloon-device/page-poison",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = virtio_balloon_page_poison_support,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(poison_val, VirtIOBalloon),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_virtio_balloon_device = {
     .name = "virtio-balloon-device",
     .version_id = 1,
@@ -767,6 +785,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
     },
     .subsections = (const VMStateDescription * []) {
         &vmstate_virtio_balloon_free_page_report,
+        &vmstate_virtio_balloon_page_poison,
         NULL
     }
 };




But I *think* the following should work as well IIRC (could be that migrating new QEMU
-> old QEMU would be an issue, I don't recall if both directions are safe):


diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a4729f7fc9..01bccf26fb 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -757,12 +757,13 @@ static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
 
 static const VMStateDescription vmstate_virtio_balloon_device = {
     .name = "virtio-balloon-device",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .post_load = virtio_balloon_post_load_device,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(num_pages, VirtIOBalloon),
         VMSTATE_UINT32(actual, VirtIOBalloon),
+        VMSTATE_UINT32_V(poison_val, VirtIOBalloon, 2),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription * []) {


-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[Alexander Duyck]

On Thu, Apr 23, 2020 at 9:02 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 23.04.20 16:46, Alexander Duyck wrote:
> > On Thu, Apr 23, 2020 at 1:11 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 22.04.20 20:21, Alexander Duyck wrote:
> >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>
> >>> We need to make certain to advertise support for page poison tracking if
> >>> we want to actually get data on if the guest will be poisoning pages.
> >>>
> >>> Add a value for tracking the poison value being used if page poisoning is
> >>> enabled. With this we can determine if we will need to skip page reporting
> >>> when it is enabled in the future.
> >>
> >> Maybe add something about the semantics
> >>
> >> "VIRTIO_BALLOON_F_PAGE_POISON will not change the behavior of free page
> >> hinting or ordinary balloon inflation/deflation."
> >>
> >> I do wonder if we should just unconditionally enable
> >> VIRTIO_BALLOON_F_PAGE_POISON here, gluing it to the QEMU compat machine
> >> (via a property that is default-enabled, and disabled from compat machines).
> >>
> >> Because, as Michael said, knowing that the guest is using page poisoning
> >> might be interesting even if free page reporting is not around.
> >
> > I could do that. So if that is the case though would I disable page
> > reporting if it isn't enabled, or would I be enabling page poison if
> > page reporting is enabled?
>
>
> So, I would suggest this the following as a diff to this patch (the TODO part as a
> separate patch - we will have these compat properties briefly after the 5.0
> release)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c1a444cb75..2e96cba4ff 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -28,6 +28,12 @@
>  #include "hw/mem/nvdimm.h"
>  #include "migration/vmstate.h"
>
> +// TODO: After 5.0 release
> +GlobalProperty hw_compat_5_0[] = {
> +    { "virtio-balloon-device", "page-hint", "false"},
> +};
> +const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> +
>  GlobalProperty hw_compat_4_2[] = {
>      { "virtio-blk-device", "queue-size", "128"},
>      { "virtio-scsi-device", "virtqueue_size", "128"},

Okay, so the bit above is for after 5_0 is released then? Is there a
way to queue up a reminder or something so we get to it when the time
comes, or I just need to watch for 5.0 to come out and submit a patch
then?

> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a4729f7fc9..5ff8a669cf 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -924,6 +924,8 @@ static Property virtio_balloon_properties[] = {
>                       qemu_4_0_config_size, false),
>      DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
>                       IOThread *),
> +    DEFINE_PROP_BIT("page-poison", VirtIOBalloon, host_features,
> +                    VIRTIO_BALLOON_F_PAGE_POISON, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
>
> What to do with page reporting depends: I would not implicitly enable features.
> I think we are talking about
>
> -device virtio-balloon-pci,...,page-poison=false,free-page-reporting=true
>
> a) Valid scenario. Fix Linux guests as we discussed to not use reporting if they rely on page poisoning.

Okay I will probably go this route and resubmit the patch I had
submitted before that only allows us to do page reporting if poisoning
is disabled on the guest kernel, or the page-poison is enabled on the
hypervisor.

> b) Invalid scenario. E.g., bail out when trying to realize that device ("'free-page-reporting' requires 'page-poison'").
>
> With new QEMU machines, this should not happen with
>
> -device virtio-balloon-pci,...,free-page-reporting=true
>
> as page-poison=true is the new default.
>
> What's your opinion?

I will just clean up and resubmit my earlier kernel patch, this time
without it messing with free page hinting.

> >
> >>>
> >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>> ---
> >>>  hw/virtio/virtio-balloon.c         |    7 +++++++
> >>>  include/hw/virtio/virtio-balloon.h |    1 +
> >>>  2 files changed, 8 insertions(+)
> >>>
> >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> >>> index a1d6fb52c876..5effc8b4653b 100644
> >>> --- a/hw/virtio/virtio-balloon.c
> >>> +++ b/hw/virtio/virtio-balloon.c
> >>> @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> >>>
> >>>      config.num_pages = cpu_to_le32(dev->num_pages);
> >>>      config.actual = cpu_to_le32(dev->actual);
> >>> +    config.poison_val = cpu_to_le32(dev->poison_val);
> >>>
> >>>      if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
> >>>          config.free_page_hint_cmd_id =
> >>> @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
> >>>          qapi_event_send_balloon_change(vm_ram_size -
> >>>                          ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
> >>>      }
> >>> +    dev->poison_val = 0;
> >>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> >>> +        dev->poison_val = le32_to_cpu(config.poison_val);
> >>> +    }
> >>>      trace_virtio_balloon_set_config(dev->actual, oldactual);
> >>>  }
> >>>
> >>> @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
> >>>          g_free(s->stats_vq_elem);
> >>>          s->stats_vq_elem = NULL;
> >>>      }
> >>> +
> >>> +    s->poison_val = 0;
> >>>  }
> >>>
> >>>  static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> >>> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> >>> index 108cff97e71a..3ca2a78e1aca 100644
> >>> --- a/include/hw/virtio/virtio-balloon.h
> >>> +++ b/include/hw/virtio/virtio-balloon.h
> >>> @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon {
> >>>      uint32_t host_features;
> >>>
> >>>      bool qemu_4_0_config_size;
> >>> +    uint32_t poison_val;
> >>>  } VirtIOBalloon;
> >>>
> >>>  #endif
> >>>
> >>
> >> You still have to migrate poison_val if I am not wrong, otherwise you
> >> would lose it during migration if I am not mistaking.
> >
> > So what are the requirements to migrate a value? Would I need to add a
> > property so it can be retrieved/set?
> >
>
> Something like this would do the trick:
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a4729f7fc9..ea0afec5f6 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -522,6 +522,13 @@ static bool virtio_balloon_free_page_support(void *opaque)
>      return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
>  }
>
> +static bool virtio_balloon_page_poison_support(void *opaque)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
> +
> +    return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> +}
> +
>  static void virtio_balloon_free_page_start(VirtIOBalloon *s)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> @@ -755,6 +762,17 @@ static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
>      }
>  };
>
> +static const VMStateDescription vmstate_virtio_balloon_page_poison = {
> +    .name = "vitio-balloon-device/page-poison",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = virtio_balloon_page_poison_support,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(poison_val, VirtIOBalloon),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_virtio_balloon_device = {
>      .name = "virtio-balloon-device",
>      .version_id = 1,
> @@ -767,6 +785,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
>      },
>      .subsections = (const VMStateDescription * []) {
>          &vmstate_virtio_balloon_free_page_report,
> +        &vmstate_virtio_balloon_page_poison,
>          NULL
>      }
>  };

So I will probably go this route. It looks like that is the way we
went for free page reporting so it is easy enough to just do some
cut/paste/replace and have something ready to go later today without
having to second guess things.

[virtio-dev] Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[Alexander Duyck]

On Thu, Apr 23, 2020 at 9:02 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 23.04.20 16:46, Alexander Duyck wrote:
> > On Thu, Apr 23, 2020 at 1:11 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 22.04.20 20:21, Alexander Duyck wrote:
> >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>
> >>> We need to make certain to advertise support for page poison tracking if
> >>> we want to actually get data on if the guest will be poisoning pages.
> >>>
> >>> Add a value for tracking the poison value being used if page poisoning is
> >>> enabled. With this we can determine if we will need to skip page reporting
> >>> when it is enabled in the future.
> >>
> >> Maybe add something about the semantics
> >>
> >> "VIRTIO_BALLOON_F_PAGE_POISON will not change the behavior of free page
> >> hinting or ordinary balloon inflation/deflation."
> >>
> >> I do wonder if we should just unconditionally enable
> >> VIRTIO_BALLOON_F_PAGE_POISON here, gluing it to the QEMU compat machine
> >> (via a property that is default-enabled, and disabled from compat machines).
> >>
> >> Because, as Michael said, knowing that the guest is using page poisoning
> >> might be interesting even if free page reporting is not around.
> >
> > I could do that. So if that is the case though would I disable page
> > reporting if it isn't enabled, or would I be enabling page poison if
> > page reporting is enabled?
>
>
> So, I would suggest this the following as a diff to this patch (the TODO part as a
> separate patch - we will have these compat properties briefly after the 5.0
> release)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c1a444cb75..2e96cba4ff 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -28,6 +28,12 @@
>  #include "hw/mem/nvdimm.h"
>  #include "migration/vmstate.h"
>
> +// TODO: After 5.0 release
> +GlobalProperty hw_compat_5_0[] = {
> +    { "virtio-balloon-device", "page-hint", "false"},
> +};
> +const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> +
>  GlobalProperty hw_compat_4_2[] = {
>      { "virtio-blk-device", "queue-size", "128"},
>      { "virtio-scsi-device", "virtqueue_size", "128"},

Okay, so the bit above is for after 5_0 is released then? Is there a
way to queue up a reminder or something so we get to it when the time
comes, or I just need to watch for 5.0 to come out and submit a patch
then?

> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a4729f7fc9..5ff8a669cf 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -924,6 +924,8 @@ static Property virtio_balloon_properties[] = {
>                       qemu_4_0_config_size, false),
>      DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
>                       IOThread *),
> +    DEFINE_PROP_BIT("page-poison", VirtIOBalloon, host_features,
> +                    VIRTIO_BALLOON_F_PAGE_POISON, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
>
> What to do with page reporting depends: I would not implicitly enable features.
> I think we are talking about
>
> -device virtio-balloon-pci,...,page-poison=false,free-page-reporting=true
>
> a) Valid scenario. Fix Linux guests as we discussed to not use reporting if they rely on page poisoning.

Okay I will probably go this route and resubmit the patch I had
submitted before that only allows us to do page reporting if poisoning
is disabled on the guest kernel, or the page-poison is enabled on the
hypervisor.

> b) Invalid scenario. E.g., bail out when trying to realize that device ("'free-page-reporting' requires 'page-poison'").
>
> With new QEMU machines, this should not happen with
>
> -device virtio-balloon-pci,...,free-page-reporting=true
>
> as page-poison=true is the new default.
>
> What's your opinion?

I will just clean up and resubmit my earlier kernel patch, this time
without it messing with free page hinting.

> >
> >>>
> >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>> ---
> >>>  hw/virtio/virtio-balloon.c         |    7 +++++++
> >>>  include/hw/virtio/virtio-balloon.h |    1 +
> >>>  2 files changed, 8 insertions(+)
> >>>
> >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> >>> index a1d6fb52c876..5effc8b4653b 100644
> >>> --- a/hw/virtio/virtio-balloon.c
> >>> +++ b/hw/virtio/virtio-balloon.c
> >>> @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> >>>
> >>>      config.num_pages = cpu_to_le32(dev->num_pages);
> >>>      config.actual = cpu_to_le32(dev->actual);
> >>> +    config.poison_val = cpu_to_le32(dev->poison_val);
> >>>
> >>>      if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
> >>>          config.free_page_hint_cmd_id =
> >>> @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
> >>>          qapi_event_send_balloon_change(vm_ram_size -
> >>>                          ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
> >>>      }
> >>> +    dev->poison_val = 0;
> >>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> >>> +        dev->poison_val = le32_to_cpu(config.poison_val);
> >>> +    }
> >>>      trace_virtio_balloon_set_config(dev->actual, oldactual);
> >>>  }
> >>>
> >>> @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
> >>>          g_free(s->stats_vq_elem);
> >>>          s->stats_vq_elem = NULL;
> >>>      }
> >>> +
> >>> +    s->poison_val = 0;
> >>>  }
> >>>
> >>>  static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> >>> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> >>> index 108cff97e71a..3ca2a78e1aca 100644
> >>> --- a/include/hw/virtio/virtio-balloon.h
> >>> +++ b/include/hw/virtio/virtio-balloon.h
> >>> @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon {
> >>>      uint32_t host_features;
> >>>
> >>>      bool qemu_4_0_config_size;
> >>> +    uint32_t poison_val;
> >>>  } VirtIOBalloon;
> >>>
> >>>  #endif
> >>>
> >>
> >> You still have to migrate poison_val if I am not wrong, otherwise you
> >> would lose it during migration if I am not mistaking.
> >
> > So what are the requirements to migrate a value? Would I need to add a
> > property so it can be retrieved/set?
> >
>
> Something like this would do the trick:
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a4729f7fc9..ea0afec5f6 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -522,6 +522,13 @@ static bool virtio_balloon_free_page_support(void *opaque)
>      return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
>  }
>
> +static bool virtio_balloon_page_poison_support(void *opaque)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
> +
> +    return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> +}
> +
>  static void virtio_balloon_free_page_start(VirtIOBalloon *s)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> @@ -755,6 +762,17 @@ static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
>      }
>  };
>
> +static const VMStateDescription vmstate_virtio_balloon_page_poison = {
> +    .name = "vitio-balloon-device/page-poison",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = virtio_balloon_page_poison_support,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(poison_val, VirtIOBalloon),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_virtio_balloon_device = {
>      .name = "virtio-balloon-device",
>      .version_id = 1,
> @@ -767,6 +785,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
>      },
>      .subsections = (const VMStateDescription * []) {
>          &vmstate_virtio_balloon_free_page_report,
> +        &vmstate_virtio_balloon_page_poison,
>          NULL
>      }
>  };

So I will probably go this route. It looks like that is the way we
went for free page reporting so it is easy enough to just do some
cut/paste/replace and have something ready to go later today without
having to second guess things.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[David Hildenbrand]

>>  GlobalProperty hw_compat_4_2[] = {
>>      { "virtio-blk-device", "queue-size", "128"},
>>      { "virtio-scsi-device", "virtqueue_size", "128"},
> 
> Okay, so the bit above is for after 5_0 is released then? Is there a

Yes.

> way to queue up a reminder or something so we get to it when the time
> comes, or I just need to watch for 5.0 to come out and submit a patch
> then?

I think what happened usually happens is that someone introduces all the
compat machines, sometimes directly with empty hw_compat.

E.g., see

commit 3eb74d2087d3bd6cb51c06a49ba94222248d2de4
Author: Cornelia Huck <cohuck@redhat.com>
Date:   Tue Nov 12 11:48:11 2019 +0100

    hw: add compat machines for 5.0

    Add 5.0 machine types for arm/i440fx/q35/s390x/spapr.

and

commit 9aec2e52ce9d9632a86be2d1d0dd493722d2e7be
Author: Cornelia Huck <cohuck@redhat.com>
Date:   Wed Jul 24 12:35:24 2019 +0200

    hw: add compat machines for 4.2

    Add 4.2 machine types for arm/i440fx/q35/s390x/spapr.


The latter already introduced hw_compat_4_1, for examnple.

@Conny, do you already have a patch for 5.1 compat patch lying around
somewhere?

[...]

> So I will probably go this route. It looks like that is the way we
> went for free page reporting so it is easy enough to just do some
> cut/paste/replace and have something ready to go later today without
> having to second guess things.


I remember that a v1->v2 vmstate migration works (e.g., old QEMU to new
QEMU). But I can't tell from the top of my head what would happen when
we migrate v2->v1 (e.g., new QEMU to old QEMU). My guess is that the
latter won't work, but I might be wrong.

Looking at migration/vmstate.c:vmstate_load_state()

"incoming version_id ... is too new for local version_id"

One would have to tell the new QEMU to send via vmstate v1 when running
under the compat machine. I don't recall if and how that would be
possible. So the other approach is a save bet.

-- 
Thanks,

David / dhildenb

[virtio-dev] Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[David Hildenbrand]

>>  GlobalProperty hw_compat_4_2[] = {
>>      { "virtio-blk-device", "queue-size", "128"},
>>      { "virtio-scsi-device", "virtqueue_size", "128"},
> 
> Okay, so the bit above is for after 5_0 is released then? Is there a

Yes.

> way to queue up a reminder or something so we get to it when the time
> comes, or I just need to watch for 5.0 to come out and submit a patch
> then?

I think what happened usually happens is that someone introduces all the
compat machines, sometimes directly with empty hw_compat.

E.g., see

commit 3eb74d2087d3bd6cb51c06a49ba94222248d2de4
Author: Cornelia Huck <cohuck@redhat.com>
Date:   Tue Nov 12 11:48:11 2019 +0100

    hw: add compat machines for 5.0

    Add 5.0 machine types for arm/i440fx/q35/s390x/spapr.

and

commit 9aec2e52ce9d9632a86be2d1d0dd493722d2e7be
Author: Cornelia Huck <cohuck@redhat.com>
Date:   Wed Jul 24 12:35:24 2019 +0200

    hw: add compat machines for 4.2

    Add 4.2 machine types for arm/i440fx/q35/s390x/spapr.


The latter already introduced hw_compat_4_1, for examnple.

@Conny, do you already have a patch for 5.1 compat patch lying around
somewhere?

[...]

> So I will probably go this route. It looks like that is the way we
> went for free page reporting so it is easy enough to just do some
> cut/paste/replace and have something ready to go later today without
> having to second guess things.


I remember that a v1->v2 vmstate migration works (e.g., old QEMU to new
QEMU). But I can't tell from the top of my head what would happen when
we migrate v2->v1 (e.g., new QEMU to old QEMU). My guess is that the
latter won't work, but I might be wrong.

Looking at migration/vmstate.c:vmstate_load_state()

"incoming version_id ... is too new for local version_id"

One would have to tell the new QEMU to send via vmstate v1 when running
under the compat machine. I don't recall if and how that would be
possible. So the other approach is a save bet.

-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[Cornelia Huck]

On Fri, 24 Apr 2020 09:07:25 +0200
David Hildenbrand <david@redhat.com> wrote:

> >>  GlobalProperty hw_compat_4_2[] = {
> >>      { "virtio-blk-device", "queue-size", "128"},
> >>      { "virtio-scsi-device", "virtqueue_size", "128"},  
> > 
> > Okay, so the bit above is for after 5_0 is released then? Is there a  
> 
> Yes.
> 
> > way to queue up a reminder or something so we get to it when the time
> > comes, or I just need to watch for 5.0 to come out and submit a patch
> > then?  
> 
> I think what happened usually happens is that someone introduces all the
> compat machines, sometimes directly with empty hw_compat.
> 
> E.g., see
> 
> commit 3eb74d2087d3bd6cb51c06a49ba94222248d2de4
> Author: Cornelia Huck <cohuck@redhat.com>
> Date:   Tue Nov 12 11:48:11 2019 +0100
> 
>     hw: add compat machines for 5.0
> 
>     Add 5.0 machine types for arm/i440fx/q35/s390x/spapr.
> 
> and
> 
> commit 9aec2e52ce9d9632a86be2d1d0dd493722d2e7be
> Author: Cornelia Huck <cohuck@redhat.com>
> Date:   Wed Jul 24 12:35:24 2019 +0200
> 
>     hw: add compat machines for 4.2
> 
>     Add 4.2 machine types for arm/i440fx/q35/s390x/spapr.
> 
> 
> The latter already introduced hw_compat_4_1, for examnple.
> 
> @Conny, do you already have a patch for 5.1 compat patch lying around
> somewhere?

Not yet, will do so now. Thanks for the reminder, nearly forgot about
that :)

[virtio-dev] Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[Cornelia Huck]

On Fri, 24 Apr 2020 09:07:25 +0200
David Hildenbrand <david@redhat.com> wrote:

> >>  GlobalProperty hw_compat_4_2[] = {
> >>      { "virtio-blk-device", "queue-size", "128"},
> >>      { "virtio-scsi-device", "virtqueue_size", "128"},  
> > 
> > Okay, so the bit above is for after 5_0 is released then? Is there a  
> 
> Yes.
> 
> > way to queue up a reminder or something so we get to it when the time
> > comes, or I just need to watch for 5.0 to come out and submit a patch
> > then?  
> 
> I think what happened usually happens is that someone introduces all the
> compat machines, sometimes directly with empty hw_compat.
> 
> E.g., see
> 
> commit 3eb74d2087d3bd6cb51c06a49ba94222248d2de4
> Author: Cornelia Huck <cohuck@redhat.com>
> Date:   Tue Nov 12 11:48:11 2019 +0100
> 
>     hw: add compat machines for 5.0
> 
>     Add 5.0 machine types for arm/i440fx/q35/s390x/spapr.
> 
> and
> 
> commit 9aec2e52ce9d9632a86be2d1d0dd493722d2e7be
> Author: Cornelia Huck <cohuck@redhat.com>
> Date:   Wed Jul 24 12:35:24 2019 +0200
> 
>     hw: add compat machines for 4.2
> 
>     Add 4.2 machine types for arm/i440fx/q35/s390x/spapr.
> 
> 
> The latter already introduced hw_compat_4_1, for examnple.
> 
> @Conny, do you already have a patch for 5.1 compat patch lying around
> somewhere?

Not yet, will do so now. Thanks for the reminder, nearly forgot about
that :)


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[David Hildenbrand]

On 24.04.20 09:53, Cornelia Huck wrote:
> On Fri, 24 Apr 2020 09:07:25 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>>>>  GlobalProperty hw_compat_4_2[] = {
>>>>      { "virtio-blk-device", "queue-size", "128"},
>>>>      { "virtio-scsi-device", "virtqueue_size", "128"},  
>>>
>>> Okay, so the bit above is for after 5_0 is released then? Is there a  
>>
>> Yes.
>>
>>> way to queue up a reminder or something so we get to it when the time
>>> comes, or I just need to watch for 5.0 to come out and submit a patch
>>> then?  
>>
>> I think what happened usually happens is that someone introduces all the
>> compat machines, sometimes directly with empty hw_compat.
>>
>> E.g., see
>>
>> commit 3eb74d2087d3bd6cb51c06a49ba94222248d2de4
>> Author: Cornelia Huck <cohuck@redhat.com>
>> Date:   Tue Nov 12 11:48:11 2019 +0100
>>
>>     hw: add compat machines for 5.0
>>
>>     Add 5.0 machine types for arm/i440fx/q35/s390x/spapr.
>>
>> and
>>
>> commit 9aec2e52ce9d9632a86be2d1d0dd493722d2e7be
>> Author: Cornelia Huck <cohuck@redhat.com>
>> Date:   Wed Jul 24 12:35:24 2019 +0200
>>
>>     hw: add compat machines for 4.2
>>
>>     Add 4.2 machine types for arm/i440fx/q35/s390x/spapr.
>>
>>
>> The latter already introduced hw_compat_4_1, for examnple.
>>
>> @Conny, do you already have a patch for 5.1 compat patch lying around
>> somewhere?
> 
> Not yet, will do so now. Thanks for the reminder, nearly forgot about
> that :)
> 

Awesome!

I just re-read my sentences and decided to consume more coffee before
starting to write emails in the morning. :)

-- 
Thanks,

David / dhildenb

[virtio-dev] Re: [PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[David Hildenbrand]

On 24.04.20 09:53, Cornelia Huck wrote:
> On Fri, 24 Apr 2020 09:07:25 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>>>>  GlobalProperty hw_compat_4_2[] = {
>>>>      { "virtio-blk-device", "queue-size", "128"},
>>>>      { "virtio-scsi-device", "virtqueue_size", "128"},  
>>>
>>> Okay, so the bit above is for after 5_0 is released then? Is there a  
>>
>> Yes.
>>
>>> way to queue up a reminder or something so we get to it when the time
>>> comes, or I just need to watch for 5.0 to come out and submit a patch
>>> then?  
>>
>> I think what happened usually happens is that someone introduces all the
>> compat machines, sometimes directly with empty hw_compat.
>>
>> E.g., see
>>
>> commit 3eb74d2087d3bd6cb51c06a49ba94222248d2de4
>> Author: Cornelia Huck <cohuck@redhat.com>
>> Date:   Tue Nov 12 11:48:11 2019 +0100
>>
>>     hw: add compat machines for 5.0
>>
>>     Add 5.0 machine types for arm/i440fx/q35/s390x/spapr.
>>
>> and
>>
>> commit 9aec2e52ce9d9632a86be2d1d0dd493722d2e7be
>> Author: Cornelia Huck <cohuck@redhat.com>
>> Date:   Wed Jul 24 12:35:24 2019 +0200
>>
>>     hw: add compat machines for 4.2
>>
>>     Add 4.2 machine types for arm/i440fx/q35/s390x/spapr.
>>
>>
>> The latter already introduced hw_compat_4_1, for examnple.
>>
>> @Conny, do you already have a patch for 5.1 compat patch lying around
>> somewhere?
> 
> Not yet, will do so now. Thanks for the reminder, nearly forgot about
> that :)
> 

Awesome!

I just re-read my sentences and decided to consume more coffee before
starting to write emails in the morning. :)

-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 5/5] virtio-balloon: Provide an interface for free page reporting

[David Hildenbrand]

On 22.04.20 20:21, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> Add support for free page reporting. The idea is to function very similar
> to how the balloon works in that we basically end up madvising the page as
> not being used. However we don't really need to bother with any deflate
> type logic since the page will be faulted back into the guest when it is
> read or written to.
> 
> This provides a new way of letting the guest proactively report free
> pages to the hypervisor, so the hypervisor can reuse them. In contrast to
> inflate/deflate that is triggered via the hypervisor explicitly.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  hw/virtio/virtio-balloon.c         |   70 ++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-balloon.h |    2 +
>  2 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5effc8b4653b..b473ff7f4b88 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -321,6 +321,60 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
>      balloon_stats_change_timer(s, 0);
>  }
>  
> +static void virtio_balloon_handle_report(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
> +    VirtQueueElement *elem;
> +
> +    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
> +        unsigned int i;
> +

Maybe add a comment like

/*
 * As discarded pages will be zero when re-accessed, all pages either
 * have the old value, or were zeroed out. In case the guest expects
 * another value, make sure to never discard.
 */

Whatever you think is best.

> +        if (qemu_balloon_is_inhibited() || dev->poison_val) {
> +            goto skip_element;
> +        }
> +
> +        for (i = 0; i < elem->in_num; i++) {
> +            void *addr = elem->in_sg[i].iov_base;
> +            size_t size = elem->in_sg[i].iov_len;
> +            ram_addr_t ram_offset;
> +            RAMBlock *rb;
> +
> +            /*
> +             * There is no need to check the memory section to see if
> +             * it is ram/readonly/romd like there is for handle_output
> +             * below. If the region is not meant to be written to then
> +             * address_space_map will have allocated a bounce buffer
> +             * and it will be freed in address_space_unmap and trigger
> +             * and unassigned_mem_write before failing to copy over the
> +             * buffer. If more than one bad descriptor is provided it
> +             * will return NULL after the first bounce buffer and fail
> +             * to map any resources.
> +             */
> +            rb = qemu_ram_block_from_host(addr, false, &ram_offset);
> +            if (!rb) {
> +                trace_virtio_balloon_bad_addr(elem->in_addr[i]);
> +                continue;
> +            }
> +
> +            /*
> +             * For now we will simply ignore unaligned memory regions, or
> +             * regions that overrun the end of the RAMBlock.
> +             */
> +            if (!QEMU_IS_ALIGNED(ram_offset | size, qemu_ram_pagesize(rb)) ||
> +                (ram_offset + size) > qemu_ram_get_used_length(rb)) {
> +                continue;
> +            }
> +
> +            ram_block_discard_range(rb, ram_offset, size);
> +        }
> +
> +skip_element:
> +        virtqueue_push(vq, elem, 0);
> +        virtio_notify(vdev, vq);
> +        g_free(elem);
> +    }
> +}
> +
>  static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>  {
>      VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> @@ -782,6 +836,16 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>      int ret;
>  
> +    /*
> +     * Page reporting is dependant on page poison to make sure we can
> +     * report a page without changing the state of the internal data.
> +     * We need to set the flag before we call virtio_init as it will
> +     * affect the config size of the vdev.
> +     */
> +    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
> +        s->host_features |= 1 << VIRTIO_BALLOON_F_PAGE_POISON;
> +    }
> +

As discussed, this hunk would go away. With that, this patch is really
minimal, which is good :)

>      virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
>                  virtio_balloon_config_size(s));
>  
> @@ -798,6 +862,10 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>      s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
>  
> +    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
> +        s->rvq = virtio_add_queue(vdev, 32, virtio_balloon_handle_report);
> +    }
> +
>      if (virtio_has_feature(s->host_features,
>                             VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
> @@ -923,6 +991,8 @@ static Property virtio_balloon_properties[] = {
>                      VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
>      DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
>                      VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
> +    DEFINE_PROP_BIT("free-page-reporting", VirtIOBalloon, host_features,
> +                    VIRTIO_BALLOON_F_REPORTING, true),

I think you'll have to similarly disable it via compat machines if you
want to default enable. Otherwise, backward migration would be broken.

Also, I do wonder if we want to default-enable it. It can still have a
negative performance impact and some people might not want that.

Apart from that, looks good to me. Nothing else we have to migrate AFAIKs.

-- 
Thanks,

David / dhildenb

[virtio-dev] Re: [PATCH v21 QEMU 5/5] virtio-balloon: Provide an interface for free page reporting

[David Hildenbrand]

On 22.04.20 20:21, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> Add support for free page reporting. The idea is to function very similar
> to how the balloon works in that we basically end up madvising the page as
> not being used. However we don't really need to bother with any deflate
> type logic since the page will be faulted back into the guest when it is
> read or written to.
> 
> This provides a new way of letting the guest proactively report free
> pages to the hypervisor, so the hypervisor can reuse them. In contrast to
> inflate/deflate that is triggered via the hypervisor explicitly.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  hw/virtio/virtio-balloon.c         |   70 ++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-balloon.h |    2 +
>  2 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5effc8b4653b..b473ff7f4b88 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -321,6 +321,60 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
>      balloon_stats_change_timer(s, 0);
>  }
>  
> +static void virtio_balloon_handle_report(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
> +    VirtQueueElement *elem;
> +
> +    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
> +        unsigned int i;
> +

Maybe add a comment like

/*
 * As discarded pages will be zero when re-accessed, all pages either
 * have the old value, or were zeroed out. In case the guest expects
 * another value, make sure to never discard.
 */

Whatever you think is best.

> +        if (qemu_balloon_is_inhibited() || dev->poison_val) {
> +            goto skip_element;
> +        }
> +
> +        for (i = 0; i < elem->in_num; i++) {
> +            void *addr = elem->in_sg[i].iov_base;
> +            size_t size = elem->in_sg[i].iov_len;
> +            ram_addr_t ram_offset;
> +            RAMBlock *rb;
> +
> +            /*
> +             * There is no need to check the memory section to see if
> +             * it is ram/readonly/romd like there is for handle_output
> +             * below. If the region is not meant to be written to then
> +             * address_space_map will have allocated a bounce buffer
> +             * and it will be freed in address_space_unmap and trigger
> +             * and unassigned_mem_write before failing to copy over the
> +             * buffer. If more than one bad descriptor is provided it
> +             * will return NULL after the first bounce buffer and fail
> +             * to map any resources.
> +             */
> +            rb = qemu_ram_block_from_host(addr, false, &ram_offset);
> +            if (!rb) {
> +                trace_virtio_balloon_bad_addr(elem->in_addr[i]);
> +                continue;
> +            }
> +
> +            /*
> +             * For now we will simply ignore unaligned memory regions, or
> +             * regions that overrun the end of the RAMBlock.
> +             */
> +            if (!QEMU_IS_ALIGNED(ram_offset | size, qemu_ram_pagesize(rb)) ||
> +                (ram_offset + size) > qemu_ram_get_used_length(rb)) {
> +                continue;
> +            }
> +
> +            ram_block_discard_range(rb, ram_offset, size);
> +        }
> +
> +skip_element:
> +        virtqueue_push(vq, elem, 0);
> +        virtio_notify(vdev, vq);
> +        g_free(elem);
> +    }
> +}
> +
>  static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>  {
>      VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> @@ -782,6 +836,16 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>      int ret;
>  
> +    /*
> +     * Page reporting is dependant on page poison to make sure we can
> +     * report a page without changing the state of the internal data.
> +     * We need to set the flag before we call virtio_init as it will
> +     * affect the config size of the vdev.
> +     */
> +    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
> +        s->host_features |= 1 << VIRTIO_BALLOON_F_PAGE_POISON;
> +    }
> +

As discussed, this hunk would go away. With that, this patch is really
minimal, which is good :)

>      virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
>                  virtio_balloon_config_size(s));
>  
> @@ -798,6 +862,10 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>      s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
>  
> +    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
> +        s->rvq = virtio_add_queue(vdev, 32, virtio_balloon_handle_report);
> +    }
> +
>      if (virtio_has_feature(s->host_features,
>                             VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
> @@ -923,6 +991,8 @@ static Property virtio_balloon_properties[] = {
>                      VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
>      DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
>                      VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
> +    DEFINE_PROP_BIT("free-page-reporting", VirtIOBalloon, host_features,
> +                    VIRTIO_BALLOON_F_REPORTING, true),

I think you'll have to similarly disable it via compat machines if you
want to default enable. Otherwise, backward migration would be broken.

Also, I do wonder if we want to default-enable it. It can still have a
negative performance impact and some people might not want that.

Apart from that, looks good to me. Nothing else we have to migrate AFAIKs.

-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'

[David Hildenbrand]

On 22.04.20 20:21, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> In an upcoming patch a feature named Free Page Reporting is about to be
> added. In order to avoid any confusion we should drop the use of the word
> 'report' when referring to Free Page Hinting. So what this patch does is go
> through and replace all instances of 'report' with 'hint" when we are
> referring to free page hinting.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  hw/virtio/virtio-balloon.c         |   74 ++++++++++++++++++------------------
>  include/hw/virtio/virtio-balloon.h |   20 +++++-----
>  2 files changed, 47 insertions(+), 47 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a4729f7fc930..a1d6fb52c876 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -466,21 +466,21 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>              ret = false;
>              goto out;
>          }
> -        if (id == dev->free_page_report_cmd_id) {
> -            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
> +        if (id == dev->free_page_hint_cmd_id) {
> +            dev->free_page_hint_status = FREE_PAGE_HINT_S_START;
>          } else {
>              /*
>               * Stop the optimization only when it has started. This
>               * avoids a stale stop sign for the previous command.
>               */
> -            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
> -                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> +            if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
> +                dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
>              }
>          }
>      }
>  
>      if (elem->in_num) {
> -        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
> +        if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
>              qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
>                                        elem->in_sg[0].iov_len);
>          }
> @@ -506,11 +506,11 @@ static void virtio_ballloon_get_free_page_hints(void *opaque)
>          qemu_mutex_unlock(&dev->free_page_lock);
>          virtio_notify(vdev, vq);
>        /*
> -       * Start to poll the vq once the reporting started. Otherwise, continue
> +       * Start to poll the vq once the hinting started. Otherwise, continue
>         * only when there are entries on the vq, which need to be given back.
>         */
>      } while (continue_to_get_hints ||
> -             dev->free_page_report_status == FREE_PAGE_REPORT_S_START);
> +             dev->free_page_hint_status == FREE_PAGE_HINT_S_START);
>      virtio_queue_set_notification(vq, 1);
>  }
>  
> @@ -531,14 +531,14 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
>          return;
>      }
>  
> -    if (s->free_page_report_cmd_id == UINT_MAX) {
> -        s->free_page_report_cmd_id =
> -                       VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> +    if (s->free_page_hint_cmd_id == UINT_MAX) {
> +        s->free_page_hint_cmd_id =
> +                       VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
>      } else {
> -        s->free_page_report_cmd_id++;
> +        s->free_page_hint_cmd_id++;
>      }
>  
> -    s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
> +    s->free_page_hint_status = FREE_PAGE_HINT_S_REQUESTED;
>      virtio_notify_config(vdev);
>  }
>  
> @@ -546,18 +546,18 @@ static void virtio_balloon_free_page_stop(VirtIOBalloon *s)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
>  
> -    if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
> +    if (s->free_page_hint_status != FREE_PAGE_HINT_S_STOP) {
>          /*
>           * The lock also guarantees us that the
>           * virtio_ballloon_get_free_page_hints exits after the
> -         * free_page_report_status is set to S_STOP.
> +         * free_page_hint_status is set to S_STOP.
>           */
>          qemu_mutex_lock(&s->free_page_lock);
>          /*
>           * The guest hasn't done the reporting, so host sends a notification
>           * to the guest to actively stop the reporting.
>           */
> -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
>          qemu_mutex_unlock(&s->free_page_lock);
>          virtio_notify_config(vdev);
>      }
> @@ -567,15 +567,15 @@ static void virtio_balloon_free_page_done(VirtIOBalloon *s)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
>  
> -    s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
> +    s->free_page_hint_status = FREE_PAGE_HINT_S_DONE;
>      virtio_notify_config(vdev);
>  }
>  
>  static int
> -virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
> +virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
>  {
>      VirtIOBalloon *dev = container_of(n, VirtIOBalloon,
> -                                      free_page_report_notify);
> +                                      free_page_hint_notify);
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      PrecopyNotifyData *pnd = data;
>  
> @@ -624,7 +624,7 @@ static size_t virtio_balloon_config_size(VirtIOBalloon *s)
>      if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>          return offsetof(struct virtio_balloon_config, poison_val);
>      }
> -    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
> +    return offsetof(struct virtio_balloon_config, free_page_hint_cmd_id);
>  }
>  
>  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> @@ -635,14 +635,14 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>      config.num_pages = cpu_to_le32(dev->num_pages);
>      config.actual = cpu_to_le32(dev->actual);
>  
> -    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
> -        config.free_page_report_cmd_id =
> -                       cpu_to_le32(dev->free_page_report_cmd_id);
> -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
> -        config.free_page_report_cmd_id =
> +    if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
> +        config.free_page_hint_cmd_id =
> +                       cpu_to_le32(dev->free_page_hint_cmd_id);
> +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_STOP) {
> +        config.free_page_hint_cmd_id =
>                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_STOP);
> -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE) {
> -        config.free_page_report_cmd_id =
> +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_DONE) {
> +        config.free_page_hint_cmd_id =
>                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_DONE);
>      }
>  
> @@ -743,14 +743,14 @@ static int virtio_balloon_post_load_device(void *opaque, int version_id)
>      return 0;
>  }
>  
> -static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
> +static const VMStateDescription vmstate_virtio_balloon_free_page_hint = {
>      .name = "virtio-balloon-device/free-page-report",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .needed = virtio_balloon_free_page_support,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
> -        VMSTATE_UINT32(free_page_report_status, VirtIOBalloon),
> +        VMSTATE_UINT32(free_page_hint_cmd_id, VirtIOBalloon),
> +        VMSTATE_UINT32(free_page_hint_status, VirtIOBalloon),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> @@ -766,7 +766,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription * []) {
> -        &vmstate_virtio_balloon_free_page_report,
> +        &vmstate_virtio_balloon_free_page_hint,
>          NULL
>      }
>  };
> @@ -797,12 +797,12 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>                             VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
>                                             virtio_balloon_handle_free_page_vq);
> -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> -        s->free_page_report_cmd_id =
> -                           VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> -        s->free_page_report_notify.notify =
> -                                       virtio_balloon_free_page_report_notify;
> -        precopy_add_notifier(&s->free_page_report_notify);
> +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
> +        s->free_page_hint_cmd_id =
> +                           VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
> +        s->free_page_hint_notify.notify =
> +                                       virtio_balloon_free_page_hint_notify;
> +        precopy_add_notifier(&s->free_page_hint_notify);
>          if (s->iothread) {
>              object_ref(OBJECT(s->iothread));
>              s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
> @@ -827,7 +827,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
>      if (virtio_balloon_free_page_support(s)) {
>          qemu_bh_delete(s->free_page_bh);
>          virtio_balloon_free_page_stop(s);
> -        precopy_remove_notifier(&s->free_page_report_notify);
> +        precopy_remove_notifier(&s->free_page_hint_notify);
>      }
>      balloon_stats_destroy_timer(s);
>      qemu_remove_balloon_handler(s);
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index d1c968d2376e..108cff97e71a 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -23,7 +23,7 @@
>  #define VIRTIO_BALLOON(obj) \
>          OBJECT_CHECK(VirtIOBalloon, (obj), TYPE_VIRTIO_BALLOON)
>  
> -#define VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN 0x80000000
> +#define VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN 0x80000000
>  
>  typedef struct virtio_balloon_stat VirtIOBalloonStat;
>  
> @@ -33,20 +33,20 @@ typedef struct virtio_balloon_stat_modern {
>         uint64_t val;
>  } VirtIOBalloonStatModern;
>  
> -enum virtio_balloon_free_page_report_status {
> -    FREE_PAGE_REPORT_S_STOP = 0,
> -    FREE_PAGE_REPORT_S_REQUESTED = 1,
> -    FREE_PAGE_REPORT_S_START = 2,
> -    FREE_PAGE_REPORT_S_DONE = 3,
> +enum virtio_balloon_free_page_hint_status {
> +    FREE_PAGE_HINT_S_STOP = 0,
> +    FREE_PAGE_HINT_S_REQUESTED = 1,
> +    FREE_PAGE_HINT_S_START = 2,
> +    FREE_PAGE_HINT_S_DONE = 3,
>  };
>  
>  typedef struct VirtIOBalloon {
>      VirtIODevice parent_obj;
>      VirtQueue *ivq, *dvq, *svq, *free_page_vq;
> -    uint32_t free_page_report_status;
> +    uint32_t free_page_hint_status;
>      uint32_t num_pages;
>      uint32_t actual;
> -    uint32_t free_page_report_cmd_id;
> +    uint32_t free_page_hint_cmd_id;
>      uint64_t stats[VIRTIO_BALLOON_S_NR];
>      VirtQueueElement *stats_vq_elem;
>      size_t stats_vq_offset;
> @@ -55,7 +55,7 @@ typedef struct VirtIOBalloon {
>      QEMUBH *free_page_bh;
>      /*
>       * Lock to synchronize threads to access the free page reporting related
> -     * fields (e.g. free_page_report_status).
> +     * fields (e.g. free_page_hint_status).
>       */
>      QemuMutex free_page_lock;
>      QemuCond  free_page_cond;
> @@ -64,7 +64,7 @@ typedef struct VirtIOBalloon {
>       * stopped.
>       */
>      bool block_iothread;
> -    NotifierWithReturn free_page_report_notify;
> +    NotifierWithReturn free_page_hint_notify;
>      int64_t stats_last_update;
>      int64_t stats_poll_interval;
>      uint32_t host_features;
> 

Maybe split out the change to "free_page_report_cmd_id" in the uapi
(meaning: move that to a separate patch). So you can move forward with
most of this series (moving patch #1 and the free_page_report_cmd_id
change to the very last of this series) without depending on that change
to go upstream.

-- 
Thanks,

David / dhildenb

[virtio-dev] Re: [PATCH v21 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'

[David Hildenbrand]

On 22.04.20 20:21, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> In an upcoming patch a feature named Free Page Reporting is about to be
> added. In order to avoid any confusion we should drop the use of the word
> 'report' when referring to Free Page Hinting. So what this patch does is go
> through and replace all instances of 'report' with 'hint" when we are
> referring to free page hinting.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  hw/virtio/virtio-balloon.c         |   74 ++++++++++++++++++------------------
>  include/hw/virtio/virtio-balloon.h |   20 +++++-----
>  2 files changed, 47 insertions(+), 47 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a4729f7fc930..a1d6fb52c876 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -466,21 +466,21 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>              ret = false;
>              goto out;
>          }
> -        if (id == dev->free_page_report_cmd_id) {
> -            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
> +        if (id == dev->free_page_hint_cmd_id) {
> +            dev->free_page_hint_status = FREE_PAGE_HINT_S_START;
>          } else {
>              /*
>               * Stop the optimization only when it has started. This
>               * avoids a stale stop sign for the previous command.
>               */
> -            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
> -                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> +            if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
> +                dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
>              }
>          }
>      }
>  
>      if (elem->in_num) {
> -        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
> +        if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
>              qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
>                                        elem->in_sg[0].iov_len);
>          }
> @@ -506,11 +506,11 @@ static void virtio_ballloon_get_free_page_hints(void *opaque)
>          qemu_mutex_unlock(&dev->free_page_lock);
>          virtio_notify(vdev, vq);
>        /*
> -       * Start to poll the vq once the reporting started. Otherwise, continue
> +       * Start to poll the vq once the hinting started. Otherwise, continue
>         * only when there are entries on the vq, which need to be given back.
>         */
>      } while (continue_to_get_hints ||
> -             dev->free_page_report_status == FREE_PAGE_REPORT_S_START);
> +             dev->free_page_hint_status == FREE_PAGE_HINT_S_START);
>      virtio_queue_set_notification(vq, 1);
>  }
>  
> @@ -531,14 +531,14 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
>          return;
>      }
>  
> -    if (s->free_page_report_cmd_id == UINT_MAX) {
> -        s->free_page_report_cmd_id =
> -                       VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> +    if (s->free_page_hint_cmd_id == UINT_MAX) {
> +        s->free_page_hint_cmd_id =
> +                       VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
>      } else {
> -        s->free_page_report_cmd_id++;
> +        s->free_page_hint_cmd_id++;
>      }
>  
> -    s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
> +    s->free_page_hint_status = FREE_PAGE_HINT_S_REQUESTED;
>      virtio_notify_config(vdev);
>  }
>  
> @@ -546,18 +546,18 @@ static void virtio_balloon_free_page_stop(VirtIOBalloon *s)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
>  
> -    if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
> +    if (s->free_page_hint_status != FREE_PAGE_HINT_S_STOP) {
>          /*
>           * The lock also guarantees us that the
>           * virtio_ballloon_get_free_page_hints exits after the
> -         * free_page_report_status is set to S_STOP.
> +         * free_page_hint_status is set to S_STOP.
>           */
>          qemu_mutex_lock(&s->free_page_lock);
>          /*
>           * The guest hasn't done the reporting, so host sends a notification
>           * to the guest to actively stop the reporting.
>           */
> -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
>          qemu_mutex_unlock(&s->free_page_lock);
>          virtio_notify_config(vdev);
>      }
> @@ -567,15 +567,15 @@ static void virtio_balloon_free_page_done(VirtIOBalloon *s)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
>  
> -    s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
> +    s->free_page_hint_status = FREE_PAGE_HINT_S_DONE;
>      virtio_notify_config(vdev);
>  }
>  
>  static int
> -virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
> +virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
>  {
>      VirtIOBalloon *dev = container_of(n, VirtIOBalloon,
> -                                      free_page_report_notify);
> +                                      free_page_hint_notify);
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      PrecopyNotifyData *pnd = data;
>  
> @@ -624,7 +624,7 @@ static size_t virtio_balloon_config_size(VirtIOBalloon *s)
>      if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>          return offsetof(struct virtio_balloon_config, poison_val);
>      }
> -    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
> +    return offsetof(struct virtio_balloon_config, free_page_hint_cmd_id);
>  }
>  
>  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> @@ -635,14 +635,14 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>      config.num_pages = cpu_to_le32(dev->num_pages);
>      config.actual = cpu_to_le32(dev->actual);
>  
> -    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
> -        config.free_page_report_cmd_id =
> -                       cpu_to_le32(dev->free_page_report_cmd_id);
> -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
> -        config.free_page_report_cmd_id =
> +    if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
> +        config.free_page_hint_cmd_id =
> +                       cpu_to_le32(dev->free_page_hint_cmd_id);
> +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_STOP) {
> +        config.free_page_hint_cmd_id =
>                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_STOP);
> -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE) {
> -        config.free_page_report_cmd_id =
> +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_DONE) {
> +        config.free_page_hint_cmd_id =
>                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_DONE);
>      }
>  
> @@ -743,14 +743,14 @@ static int virtio_balloon_post_load_device(void *opaque, int version_id)
>      return 0;
>  }
>  
> -static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
> +static const VMStateDescription vmstate_virtio_balloon_free_page_hint = {
>      .name = "virtio-balloon-device/free-page-report",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .needed = virtio_balloon_free_page_support,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
> -        VMSTATE_UINT32(free_page_report_status, VirtIOBalloon),
> +        VMSTATE_UINT32(free_page_hint_cmd_id, VirtIOBalloon),
> +        VMSTATE_UINT32(free_page_hint_status, VirtIOBalloon),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> @@ -766,7 +766,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription * []) {
> -        &vmstate_virtio_balloon_free_page_report,
> +        &vmstate_virtio_balloon_free_page_hint,
>          NULL
>      }
>  };
> @@ -797,12 +797,12 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>                             VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
>                                             virtio_balloon_handle_free_page_vq);
> -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> -        s->free_page_report_cmd_id =
> -                           VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> -        s->free_page_report_notify.notify =
> -                                       virtio_balloon_free_page_report_notify;
> -        precopy_add_notifier(&s->free_page_report_notify);
> +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
> +        s->free_page_hint_cmd_id =
> +                           VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
> +        s->free_page_hint_notify.notify =
> +                                       virtio_balloon_free_page_hint_notify;
> +        precopy_add_notifier(&s->free_page_hint_notify);
>          if (s->iothread) {
>              object_ref(OBJECT(s->iothread));
>              s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
> @@ -827,7 +827,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
>      if (virtio_balloon_free_page_support(s)) {
>          qemu_bh_delete(s->free_page_bh);
>          virtio_balloon_free_page_stop(s);
> -        precopy_remove_notifier(&s->free_page_report_notify);
> +        precopy_remove_notifier(&s->free_page_hint_notify);
>      }
>      balloon_stats_destroy_timer(s);
>      qemu_remove_balloon_handler(s);
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index d1c968d2376e..108cff97e71a 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -23,7 +23,7 @@
>  #define VIRTIO_BALLOON(obj) \
>          OBJECT_CHECK(VirtIOBalloon, (obj), TYPE_VIRTIO_BALLOON)
>  
> -#define VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN 0x80000000
> +#define VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN 0x80000000
>  
>  typedef struct virtio_balloon_stat VirtIOBalloonStat;
>  
> @@ -33,20 +33,20 @@ typedef struct virtio_balloon_stat_modern {
>         uint64_t val;
>  } VirtIOBalloonStatModern;
>  
> -enum virtio_balloon_free_page_report_status {
> -    FREE_PAGE_REPORT_S_STOP = 0,
> -    FREE_PAGE_REPORT_S_REQUESTED = 1,
> -    FREE_PAGE_REPORT_S_START = 2,
> -    FREE_PAGE_REPORT_S_DONE = 3,
> +enum virtio_balloon_free_page_hint_status {
> +    FREE_PAGE_HINT_S_STOP = 0,
> +    FREE_PAGE_HINT_S_REQUESTED = 1,
> +    FREE_PAGE_HINT_S_START = 2,
> +    FREE_PAGE_HINT_S_DONE = 3,
>  };
>  
>  typedef struct VirtIOBalloon {
>      VirtIODevice parent_obj;
>      VirtQueue *ivq, *dvq, *svq, *free_page_vq;
> -    uint32_t free_page_report_status;
> +    uint32_t free_page_hint_status;
>      uint32_t num_pages;
>      uint32_t actual;
> -    uint32_t free_page_report_cmd_id;
> +    uint32_t free_page_hint_cmd_id;
>      uint64_t stats[VIRTIO_BALLOON_S_NR];
>      VirtQueueElement *stats_vq_elem;
>      size_t stats_vq_offset;
> @@ -55,7 +55,7 @@ typedef struct VirtIOBalloon {
>      QEMUBH *free_page_bh;
>      /*
>       * Lock to synchronize threads to access the free page reporting related
> -     * fields (e.g. free_page_report_status).
> +     * fields (e.g. free_page_hint_status).
>       */
>      QemuMutex free_page_lock;
>      QemuCond  free_page_cond;
> @@ -64,7 +64,7 @@ typedef struct VirtIOBalloon {
>       * stopped.
>       */
>      bool block_iothread;
> -    NotifierWithReturn free_page_report_notify;
> +    NotifierWithReturn free_page_hint_notify;
>      int64_t stats_last_update;
>      int64_t stats_poll_interval;
>      uint32_t host_features;
> 

Maybe split out the change to "free_page_report_cmd_id" in the uapi
(meaning: move that to a separate patch). So you can move forward with
most of this series (moving patch #1 and the free_page_report_cmd_id
change to the very last of this series) without depending on that change
to go upstream.

-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'

[Alexander Duyck]

On Fri, Apr 24, 2020 at 4:23 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 22.04.20 20:21, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > In an upcoming patch a feature named Free Page Reporting is about to be
> > added. In order to avoid any confusion we should drop the use of the word
> > 'report' when referring to Free Page Hinting. So what this patch does is go
> > through and replace all instances of 'report' with 'hint" when we are
> > referring to free page hinting.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  hw/virtio/virtio-balloon.c         |   74 ++++++++++++++++++------------------
> >  include/hw/virtio/virtio-balloon.h |   20 +++++-----
> >  2 files changed, 47 insertions(+), 47 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index a4729f7fc930..a1d6fb52c876 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -466,21 +466,21 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> >              ret = false;
> >              goto out;
> >          }
> > -        if (id == dev->free_page_report_cmd_id) {
> > -            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
> > +        if (id == dev->free_page_hint_cmd_id) {
> > +            dev->free_page_hint_status = FREE_PAGE_HINT_S_START;
> >          } else {
> >              /*
> >               * Stop the optimization only when it has started. This
> >               * avoids a stale stop sign for the previous command.
> >               */
> > -            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
> > -                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > +            if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
> > +                dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
> >              }
> >          }
> >      }
> >
> >      if (elem->in_num) {
> > -        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
> > +        if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
> >              qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
> >                                        elem->in_sg[0].iov_len);
> >          }
> > @@ -506,11 +506,11 @@ static void virtio_ballloon_get_free_page_hints(void *opaque)
> >          qemu_mutex_unlock(&dev->free_page_lock);
> >          virtio_notify(vdev, vq);
> >        /*
> > -       * Start to poll the vq once the reporting started. Otherwise, continue
> > +       * Start to poll the vq once the hinting started. Otherwise, continue
> >         * only when there are entries on the vq, which need to be given back.
> >         */
> >      } while (continue_to_get_hints ||
> > -             dev->free_page_report_status == FREE_PAGE_REPORT_S_START);
> > +             dev->free_page_hint_status == FREE_PAGE_HINT_S_START);
> >      virtio_queue_set_notification(vq, 1);
> >  }
> >
> > @@ -531,14 +531,14 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
> >          return;
> >      }
> >
> > -    if (s->free_page_report_cmd_id == UINT_MAX) {
> > -        s->free_page_report_cmd_id =
> > -                       VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> > +    if (s->free_page_hint_cmd_id == UINT_MAX) {
> > +        s->free_page_hint_cmd_id =
> > +                       VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
> >      } else {
> > -        s->free_page_report_cmd_id++;
> > +        s->free_page_hint_cmd_id++;
> >      }
> >
> > -    s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
> > +    s->free_page_hint_status = FREE_PAGE_HINT_S_REQUESTED;
> >      virtio_notify_config(vdev);
> >  }
> >
> > @@ -546,18 +546,18 @@ static void virtio_balloon_free_page_stop(VirtIOBalloon *s)
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >
> > -    if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
> > +    if (s->free_page_hint_status != FREE_PAGE_HINT_S_STOP) {
> >          /*
> >           * The lock also guarantees us that the
> >           * virtio_ballloon_get_free_page_hints exits after the
> > -         * free_page_report_status is set to S_STOP.
> > +         * free_page_hint_status is set to S_STOP.
> >           */
> >          qemu_mutex_lock(&s->free_page_lock);
> >          /*
> >           * The guest hasn't done the reporting, so host sends a notification
> >           * to the guest to actively stop the reporting.
> >           */
> > -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
> >          qemu_mutex_unlock(&s->free_page_lock);
> >          virtio_notify_config(vdev);
> >      }
> > @@ -567,15 +567,15 @@ static void virtio_balloon_free_page_done(VirtIOBalloon *s)
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >
> > -    s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
> > +    s->free_page_hint_status = FREE_PAGE_HINT_S_DONE;
> >      virtio_notify_config(vdev);
> >  }
> >
> >  static int
> > -virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
> > +virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
> >  {
> >      VirtIOBalloon *dev = container_of(n, VirtIOBalloon,
> > -                                      free_page_report_notify);
> > +                                      free_page_hint_notify);
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      PrecopyNotifyData *pnd = data;
> >
> > @@ -624,7 +624,7 @@ static size_t virtio_balloon_config_size(VirtIOBalloon *s)
> >      if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> >          return offsetof(struct virtio_balloon_config, poison_val);
> >      }
> > -    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
> > +    return offsetof(struct virtio_balloon_config, free_page_hint_cmd_id);
> >  }
> >
> >  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> > @@ -635,14 +635,14 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> >      config.num_pages = cpu_to_le32(dev->num_pages);
> >      config.actual = cpu_to_le32(dev->actual);
> >
> > -    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
> > -        config.free_page_report_cmd_id =
> > -                       cpu_to_le32(dev->free_page_report_cmd_id);
> > -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
> > -        config.free_page_report_cmd_id =
> > +    if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
> > +        config.free_page_hint_cmd_id =
> > +                       cpu_to_le32(dev->free_page_hint_cmd_id);
> > +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_STOP) {
> > +        config.free_page_hint_cmd_id =
> >                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_STOP);
> > -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE) {
> > -        config.free_page_report_cmd_id =
> > +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_DONE) {
> > +        config.free_page_hint_cmd_id =
> >                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_DONE);
> >      }
> >
> > @@ -743,14 +743,14 @@ static int virtio_balloon_post_load_device(void *opaque, int version_id)
> >      return 0;
> >  }
> >
> > -static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
> > +static const VMStateDescription vmstate_virtio_balloon_free_page_hint = {
> >      .name = "virtio-balloon-device/free-page-report",
> >      .version_id = 1,
> >      .minimum_version_id = 1,
> >      .needed = virtio_balloon_free_page_support,
> >      .fields = (VMStateField[]) {
> > -        VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
> > -        VMSTATE_UINT32(free_page_report_status, VirtIOBalloon),
> > +        VMSTATE_UINT32(free_page_hint_cmd_id, VirtIOBalloon),
> > +        VMSTATE_UINT32(free_page_hint_status, VirtIOBalloon),
> >          VMSTATE_END_OF_LIST()
> >      }
> >  };
> > @@ -766,7 +766,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
> >          VMSTATE_END_OF_LIST()
> >      },
> >      .subsections = (const VMStateDescription * []) {
> > -        &vmstate_virtio_balloon_free_page_report,
> > +        &vmstate_virtio_balloon_free_page_hint,
> >          NULL
> >      }
> >  };
> > @@ -797,12 +797,12 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >                             VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> >          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
> >                                             virtio_balloon_handle_free_page_vq);
> > -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > -        s->free_page_report_cmd_id =
> > -                           VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> > -        s->free_page_report_notify.notify =
> > -                                       virtio_balloon_free_page_report_notify;
> > -        precopy_add_notifier(&s->free_page_report_notify);
> > +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
> > +        s->free_page_hint_cmd_id =
> > +                           VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
> > +        s->free_page_hint_notify.notify =
> > +                                       virtio_balloon_free_page_hint_notify;
> > +        precopy_add_notifier(&s->free_page_hint_notify);
> >          if (s->iothread) {
> >              object_ref(OBJECT(s->iothread));
> >              s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
> > @@ -827,7 +827,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
> >      if (virtio_balloon_free_page_support(s)) {
> >          qemu_bh_delete(s->free_page_bh);
> >          virtio_balloon_free_page_stop(s);
> > -        precopy_remove_notifier(&s->free_page_report_notify);
> > +        precopy_remove_notifier(&s->free_page_hint_notify);
> >      }
> >      balloon_stats_destroy_timer(s);
> >      qemu_remove_balloon_handler(s);
> > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> > index d1c968d2376e..108cff97e71a 100644
> > --- a/include/hw/virtio/virtio-balloon.h
> > +++ b/include/hw/virtio/virtio-balloon.h
> > @@ -23,7 +23,7 @@
> >  #define VIRTIO_BALLOON(obj) \
> >          OBJECT_CHECK(VirtIOBalloon, (obj), TYPE_VIRTIO_BALLOON)
> >
> > -#define VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN 0x80000000
> > +#define VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN 0x80000000
> >
> >  typedef struct virtio_balloon_stat VirtIOBalloonStat;
> >
> > @@ -33,20 +33,20 @@ typedef struct virtio_balloon_stat_modern {
> >         uint64_t val;
> >  } VirtIOBalloonStatModern;
> >
> > -enum virtio_balloon_free_page_report_status {
> > -    FREE_PAGE_REPORT_S_STOP = 0,
> > -    FREE_PAGE_REPORT_S_REQUESTED = 1,
> > -    FREE_PAGE_REPORT_S_START = 2,
> > -    FREE_PAGE_REPORT_S_DONE = 3,
> > +enum virtio_balloon_free_page_hint_status {
> > +    FREE_PAGE_HINT_S_STOP = 0,
> > +    FREE_PAGE_HINT_S_REQUESTED = 1,
> > +    FREE_PAGE_HINT_S_START = 2,
> > +    FREE_PAGE_HINT_S_DONE = 3,
> >  };
> >
> >  typedef struct VirtIOBalloon {
> >      VirtIODevice parent_obj;
> >      VirtQueue *ivq, *dvq, *svq, *free_page_vq;
> > -    uint32_t free_page_report_status;
> > +    uint32_t free_page_hint_status;
> >      uint32_t num_pages;
> >      uint32_t actual;
> > -    uint32_t free_page_report_cmd_id;
> > +    uint32_t free_page_hint_cmd_id;
> >      uint64_t stats[VIRTIO_BALLOON_S_NR];
> >      VirtQueueElement *stats_vq_elem;
> >      size_t stats_vq_offset;
> > @@ -55,7 +55,7 @@ typedef struct VirtIOBalloon {
> >      QEMUBH *free_page_bh;
> >      /*
> >       * Lock to synchronize threads to access the free page reporting related
> > -     * fields (e.g. free_page_report_status).
> > +     * fields (e.g. free_page_hint_status).
> >       */
> >      QemuMutex free_page_lock;
> >      QemuCond  free_page_cond;
> > @@ -64,7 +64,7 @@ typedef struct VirtIOBalloon {
> >       * stopped.
> >       */
> >      bool block_iothread;
> > -    NotifierWithReturn free_page_report_notify;
> > +    NotifierWithReturn free_page_hint_notify;
> >      int64_t stats_last_update;
> >      int64_t stats_poll_interval;
> >      uint32_t host_features;
> >
>
> Maybe split out the change to "free_page_report_cmd_id" in the uapi
> (meaning: move that to a separate patch). So you can move forward with
> most of this series (moving patch #1 and the free_page_report_cmd_id
> change to the very last of this series) without depending on that change
> to go upstream.

Okay. I can split it if that is needed. Any specific reason for
splitting it I should cite in the patch? From what I can tell Michael
has already accepted the renamed and pushed it to Linus.

Thanks.

- Alex

[virtio-dev] Re: [PATCH v21 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'

[Alexander Duyck]

On Fri, Apr 24, 2020 at 4:23 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 22.04.20 20:21, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > In an upcoming patch a feature named Free Page Reporting is about to be
> > added. In order to avoid any confusion we should drop the use of the word
> > 'report' when referring to Free Page Hinting. So what this patch does is go
> > through and replace all instances of 'report' with 'hint" when we are
> > referring to free page hinting.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  hw/virtio/virtio-balloon.c         |   74 ++++++++++++++++++------------------
> >  include/hw/virtio/virtio-balloon.h |   20 +++++-----
> >  2 files changed, 47 insertions(+), 47 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index a4729f7fc930..a1d6fb52c876 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -466,21 +466,21 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> >              ret = false;
> >              goto out;
> >          }
> > -        if (id == dev->free_page_report_cmd_id) {
> > -            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
> > +        if (id == dev->free_page_hint_cmd_id) {
> > +            dev->free_page_hint_status = FREE_PAGE_HINT_S_START;
> >          } else {
> >              /*
> >               * Stop the optimization only when it has started. This
> >               * avoids a stale stop sign for the previous command.
> >               */
> > -            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
> > -                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > +            if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
> > +                dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
> >              }
> >          }
> >      }
> >
> >      if (elem->in_num) {
> > -        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
> > +        if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
> >              qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
> >                                        elem->in_sg[0].iov_len);
> >          }
> > @@ -506,11 +506,11 @@ static void virtio_ballloon_get_free_page_hints(void *opaque)
> >          qemu_mutex_unlock(&dev->free_page_lock);
> >          virtio_notify(vdev, vq);
> >        /*
> > -       * Start to poll the vq once the reporting started. Otherwise, continue
> > +       * Start to poll the vq once the hinting started. Otherwise, continue
> >         * only when there are entries on the vq, which need to be given back.
> >         */
> >      } while (continue_to_get_hints ||
> > -             dev->free_page_report_status == FREE_PAGE_REPORT_S_START);
> > +             dev->free_page_hint_status == FREE_PAGE_HINT_S_START);
> >      virtio_queue_set_notification(vq, 1);
> >  }
> >
> > @@ -531,14 +531,14 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
> >          return;
> >      }
> >
> > -    if (s->free_page_report_cmd_id == UINT_MAX) {
> > -        s->free_page_report_cmd_id =
> > -                       VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> > +    if (s->free_page_hint_cmd_id == UINT_MAX) {
> > +        s->free_page_hint_cmd_id =
> > +                       VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
> >      } else {
> > -        s->free_page_report_cmd_id++;
> > +        s->free_page_hint_cmd_id++;
> >      }
> >
> > -    s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
> > +    s->free_page_hint_status = FREE_PAGE_HINT_S_REQUESTED;
> >      virtio_notify_config(vdev);
> >  }
> >
> > @@ -546,18 +546,18 @@ static void virtio_balloon_free_page_stop(VirtIOBalloon *s)
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >
> > -    if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
> > +    if (s->free_page_hint_status != FREE_PAGE_HINT_S_STOP) {
> >          /*
> >           * The lock also guarantees us that the
> >           * virtio_ballloon_get_free_page_hints exits after the
> > -         * free_page_report_status is set to S_STOP.
> > +         * free_page_hint_status is set to S_STOP.
> >           */
> >          qemu_mutex_lock(&s->free_page_lock);
> >          /*
> >           * The guest hasn't done the reporting, so host sends a notification
> >           * to the guest to actively stop the reporting.
> >           */
> > -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
> >          qemu_mutex_unlock(&s->free_page_lock);
> >          virtio_notify_config(vdev);
> >      }
> > @@ -567,15 +567,15 @@ static void virtio_balloon_free_page_done(VirtIOBalloon *s)
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >
> > -    s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
> > +    s->free_page_hint_status = FREE_PAGE_HINT_S_DONE;
> >      virtio_notify_config(vdev);
> >  }
> >
> >  static int
> > -virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
> > +virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
> >  {
> >      VirtIOBalloon *dev = container_of(n, VirtIOBalloon,
> > -                                      free_page_report_notify);
> > +                                      free_page_hint_notify);
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      PrecopyNotifyData *pnd = data;
> >
> > @@ -624,7 +624,7 @@ static size_t virtio_balloon_config_size(VirtIOBalloon *s)
> >      if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> >          return offsetof(struct virtio_balloon_config, poison_val);
> >      }
> > -    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
> > +    return offsetof(struct virtio_balloon_config, free_page_hint_cmd_id);
> >  }
> >
> >  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> > @@ -635,14 +635,14 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> >      config.num_pages = cpu_to_le32(dev->num_pages);
> >      config.actual = cpu_to_le32(dev->actual);
> >
> > -    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
> > -        config.free_page_report_cmd_id =
> > -                       cpu_to_le32(dev->free_page_report_cmd_id);
> > -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
> > -        config.free_page_report_cmd_id =
> > +    if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
> > +        config.free_page_hint_cmd_id =
> > +                       cpu_to_le32(dev->free_page_hint_cmd_id);
> > +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_STOP) {
> > +        config.free_page_hint_cmd_id =
> >                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_STOP);
> > -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE) {
> > -        config.free_page_report_cmd_id =
> > +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_DONE) {
> > +        config.free_page_hint_cmd_id =
> >                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_DONE);
> >      }
> >
> > @@ -743,14 +743,14 @@ static int virtio_balloon_post_load_device(void *opaque, int version_id)
> >      return 0;
> >  }
> >
> > -static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
> > +static const VMStateDescription vmstate_virtio_balloon_free_page_hint = {
> >      .name = "virtio-balloon-device/free-page-report",
> >      .version_id = 1,
> >      .minimum_version_id = 1,
> >      .needed = virtio_balloon_free_page_support,
> >      .fields = (VMStateField[]) {
> > -        VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
> > -        VMSTATE_UINT32(free_page_report_status, VirtIOBalloon),
> > +        VMSTATE_UINT32(free_page_hint_cmd_id, VirtIOBalloon),
> > +        VMSTATE_UINT32(free_page_hint_status, VirtIOBalloon),
> >          VMSTATE_END_OF_LIST()
> >      }
> >  };
> > @@ -766,7 +766,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
> >          VMSTATE_END_OF_LIST()
> >      },
> >      .subsections = (const VMStateDescription * []) {
> > -        &vmstate_virtio_balloon_free_page_report,
> > +        &vmstate_virtio_balloon_free_page_hint,
> >          NULL
> >      }
> >  };
> > @@ -797,12 +797,12 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >                             VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> >          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
> >                                             virtio_balloon_handle_free_page_vq);
> > -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > -        s->free_page_report_cmd_id =
> > -                           VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> > -        s->free_page_report_notify.notify =
> > -                                       virtio_balloon_free_page_report_notify;
> > -        precopy_add_notifier(&s->free_page_report_notify);
> > +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
> > +        s->free_page_hint_cmd_id =
> > +                           VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
> > +        s->free_page_hint_notify.notify =
> > +                                       virtio_balloon_free_page_hint_notify;
> > +        precopy_add_notifier(&s->free_page_hint_notify);
> >          if (s->iothread) {
> >              object_ref(OBJECT(s->iothread));
> >              s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
> > @@ -827,7 +827,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
> >      if (virtio_balloon_free_page_support(s)) {
> >          qemu_bh_delete(s->free_page_bh);
> >          virtio_balloon_free_page_stop(s);
> > -        precopy_remove_notifier(&s->free_page_report_notify);
> > +        precopy_remove_notifier(&s->free_page_hint_notify);
> >      }
> >      balloon_stats_destroy_timer(s);
> >      qemu_remove_balloon_handler(s);
> > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> > index d1c968d2376e..108cff97e71a 100644
> > --- a/include/hw/virtio/virtio-balloon.h
> > +++ b/include/hw/virtio/virtio-balloon.h
> > @@ -23,7 +23,7 @@
> >  #define VIRTIO_BALLOON(obj) \
> >          OBJECT_CHECK(VirtIOBalloon, (obj), TYPE_VIRTIO_BALLOON)
> >
> > -#define VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN 0x80000000
> > +#define VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN 0x80000000
> >
> >  typedef struct virtio_balloon_stat VirtIOBalloonStat;
> >
> > @@ -33,20 +33,20 @@ typedef struct virtio_balloon_stat_modern {
> >         uint64_t val;
> >  } VirtIOBalloonStatModern;
> >
> > -enum virtio_balloon_free_page_report_status {
> > -    FREE_PAGE_REPORT_S_STOP = 0,
> > -    FREE_PAGE_REPORT_S_REQUESTED = 1,
> > -    FREE_PAGE_REPORT_S_START = 2,
> > -    FREE_PAGE_REPORT_S_DONE = 3,
> > +enum virtio_balloon_free_page_hint_status {
> > +    FREE_PAGE_HINT_S_STOP = 0,
> > +    FREE_PAGE_HINT_S_REQUESTED = 1,
> > +    FREE_PAGE_HINT_S_START = 2,
> > +    FREE_PAGE_HINT_S_DONE = 3,
> >  };
> >
> >  typedef struct VirtIOBalloon {
> >      VirtIODevice parent_obj;
> >      VirtQueue *ivq, *dvq, *svq, *free_page_vq;
> > -    uint32_t free_page_report_status;
> > +    uint32_t free_page_hint_status;
> >      uint32_t num_pages;
> >      uint32_t actual;
> > -    uint32_t free_page_report_cmd_id;
> > +    uint32_t free_page_hint_cmd_id;
> >      uint64_t stats[VIRTIO_BALLOON_S_NR];
> >      VirtQueueElement *stats_vq_elem;
> >      size_t stats_vq_offset;
> > @@ -55,7 +55,7 @@ typedef struct VirtIOBalloon {
> >      QEMUBH *free_page_bh;
> >      /*
> >       * Lock to synchronize threads to access the free page reporting related
> > -     * fields (e.g. free_page_report_status).
> > +     * fields (e.g. free_page_hint_status).
> >       */
> >      QemuMutex free_page_lock;
> >      QemuCond  free_page_cond;
> > @@ -64,7 +64,7 @@ typedef struct VirtIOBalloon {
> >       * stopped.
> >       */
> >      bool block_iothread;
> > -    NotifierWithReturn free_page_report_notify;
> > +    NotifierWithReturn free_page_hint_notify;
> >      int64_t stats_last_update;
> >      int64_t stats_poll_interval;
> >      uint32_t host_features;
> >
>
> Maybe split out the change to "free_page_report_cmd_id" in the uapi
> (meaning: move that to a separate patch). So you can move forward with
> most of this series (moving patch #1 and the free_page_report_cmd_id
> change to the very last of this series) without depending on that change
> to go upstream.

Okay. I can split it if that is needed. Any specific reason for
splitting it I should cite in the patch? From what I can tell Michael
has already accepted the renamed and pushed it to Linus.

Thanks.

- Alex

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'

[David Hildenbrand]

On 24.04.20 16:56, Alexander Duyck wrote:
> On Fri, Apr 24, 2020 at 4:23 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 22.04.20 20:21, Alexander Duyck wrote:
>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>
>>> In an upcoming patch a feature named Free Page Reporting is about to be
>>> added. In order to avoid any confusion we should drop the use of the word
>>> 'report' when referring to Free Page Hinting. So what this patch does is go
>>> through and replace all instances of 'report' with 'hint" when we are
>>> referring to free page hinting.
>>>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>> ---
>>>  hw/virtio/virtio-balloon.c         |   74 ++++++++++++++++++------------------
>>>  include/hw/virtio/virtio-balloon.h |   20 +++++-----
>>>  2 files changed, 47 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>>> index a4729f7fc930..a1d6fb52c876 100644
>>> --- a/hw/virtio/virtio-balloon.c
>>> +++ b/hw/virtio/virtio-balloon.c
>>> @@ -466,21 +466,21 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>>>              ret = false;
>>>              goto out;
>>>          }
>>> -        if (id == dev->free_page_report_cmd_id) {
>>> -            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
>>> +        if (id == dev->free_page_hint_cmd_id) {
>>> +            dev->free_page_hint_status = FREE_PAGE_HINT_S_START;
>>>          } else {
>>>              /*
>>>               * Stop the optimization only when it has started. This
>>>               * avoids a stale stop sign for the previous command.
>>>               */
>>> -            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
>>> -                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
>>> +            if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
>>> +                dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
>>>              }
>>>          }
>>>      }
>>>
>>>      if (elem->in_num) {
>>> -        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
>>> +        if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
>>>              qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
>>>                                        elem->in_sg[0].iov_len);
>>>          }
>>> @@ -506,11 +506,11 @@ static void virtio_ballloon_get_free_page_hints(void *opaque)
>>>          qemu_mutex_unlock(&dev->free_page_lock);
>>>          virtio_notify(vdev, vq);
>>>        /*
>>> -       * Start to poll the vq once the reporting started. Otherwise, continue
>>> +       * Start to poll the vq once the hinting started. Otherwise, continue
>>>         * only when there are entries on the vq, which need to be given back.
>>>         */
>>>      } while (continue_to_get_hints ||
>>> -             dev->free_page_report_status == FREE_PAGE_REPORT_S_START);
>>> +             dev->free_page_hint_status == FREE_PAGE_HINT_S_START);
>>>      virtio_queue_set_notification(vq, 1);
>>>  }
>>>
>>> @@ -531,14 +531,14 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
>>>          return;
>>>      }
>>>
>>> -    if (s->free_page_report_cmd_id == UINT_MAX) {
>>> -        s->free_page_report_cmd_id =
>>> -                       VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
>>> +    if (s->free_page_hint_cmd_id == UINT_MAX) {
>>> +        s->free_page_hint_cmd_id =
>>> +                       VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
>>>      } else {
>>> -        s->free_page_report_cmd_id++;
>>> +        s->free_page_hint_cmd_id++;
>>>      }
>>>
>>> -    s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
>>> +    s->free_page_hint_status = FREE_PAGE_HINT_S_REQUESTED;
>>>      virtio_notify_config(vdev);
>>>  }
>>>
>>> @@ -546,18 +546,18 @@ static void virtio_balloon_free_page_stop(VirtIOBalloon *s)
>>>  {
>>>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
>>>
>>> -    if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
>>> +    if (s->free_page_hint_status != FREE_PAGE_HINT_S_STOP) {
>>>          /*
>>>           * The lock also guarantees us that the
>>>           * virtio_ballloon_get_free_page_hints exits after the
>>> -         * free_page_report_status is set to S_STOP.
>>> +         * free_page_hint_status is set to S_STOP.
>>>           */
>>>          qemu_mutex_lock(&s->free_page_lock);
>>>          /*
>>>           * The guest hasn't done the reporting, so host sends a notification
>>>           * to the guest to actively stop the reporting.
>>>           */
>>> -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
>>> +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
>>>          qemu_mutex_unlock(&s->free_page_lock);
>>>          virtio_notify_config(vdev);
>>>      }
>>> @@ -567,15 +567,15 @@ static void virtio_balloon_free_page_done(VirtIOBalloon *s)
>>>  {
>>>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
>>>
>>> -    s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
>>> +    s->free_page_hint_status = FREE_PAGE_HINT_S_DONE;
>>>      virtio_notify_config(vdev);
>>>  }
>>>
>>>  static int
>>> -virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
>>> +virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
>>>  {
>>>      VirtIOBalloon *dev = container_of(n, VirtIOBalloon,
>>> -                                      free_page_report_notify);
>>> +                                      free_page_hint_notify);
>>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>      PrecopyNotifyData *pnd = data;
>>>
>>> @@ -624,7 +624,7 @@ static size_t virtio_balloon_config_size(VirtIOBalloon *s)
>>>      if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>>>          return offsetof(struct virtio_balloon_config, poison_val);
>>>      }
>>> -    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
>>> +    return offsetof(struct virtio_balloon_config, free_page_hint_cmd_id);
>>>  }
>>>
>>>  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>>> @@ -635,14 +635,14 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>>>      config.num_pages = cpu_to_le32(dev->num_pages);
>>>      config.actual = cpu_to_le32(dev->actual);
>>>
>>> -    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
>>> -        config.free_page_report_cmd_id =
>>> -                       cpu_to_le32(dev->free_page_report_cmd_id);
>>> -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
>>> -        config.free_page_report_cmd_id =
>>> +    if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
>>> +        config.free_page_hint_cmd_id =
>>> +                       cpu_to_le32(dev->free_page_hint_cmd_id);
>>> +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_STOP) {
>>> +        config.free_page_hint_cmd_id =
>>>                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_STOP);
>>> -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE) {
>>> -        config.free_page_report_cmd_id =
>>> +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_DONE) {
>>> +        config.free_page_hint_cmd_id =
>>>                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_DONE);
>>>      }
>>>
>>> @@ -743,14 +743,14 @@ static int virtio_balloon_post_load_device(void *opaque, int version_id)
>>>      return 0;
>>>  }
>>>
>>> -static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
>>> +static const VMStateDescription vmstate_virtio_balloon_free_page_hint = {
>>>      .name = "virtio-balloon-device/free-page-report",
>>>      .version_id = 1,
>>>      .minimum_version_id = 1,
>>>      .needed = virtio_balloon_free_page_support,
>>>      .fields = (VMStateField[]) {
>>> -        VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
>>> -        VMSTATE_UINT32(free_page_report_status, VirtIOBalloon),
>>> +        VMSTATE_UINT32(free_page_hint_cmd_id, VirtIOBalloon),
>>> +        VMSTATE_UINT32(free_page_hint_status, VirtIOBalloon),
>>>          VMSTATE_END_OF_LIST()
>>>      }
>>>  };
>>> @@ -766,7 +766,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
>>>          VMSTATE_END_OF_LIST()
>>>      },
>>>      .subsections = (const VMStateDescription * []) {
>>> -        &vmstate_virtio_balloon_free_page_report,
>>> +        &vmstate_virtio_balloon_free_page_hint,
>>>          NULL
>>>      }
>>>  };
>>> @@ -797,12 +797,12 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>>>                             VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>>>          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
>>>                                             virtio_balloon_handle_free_page_vq);
>>> -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
>>> -        s->free_page_report_cmd_id =
>>> -                           VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
>>> -        s->free_page_report_notify.notify =
>>> -                                       virtio_balloon_free_page_report_notify;
>>> -        precopy_add_notifier(&s->free_page_report_notify);
>>> +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
>>> +        s->free_page_hint_cmd_id =
>>> +                           VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
>>> +        s->free_page_hint_notify.notify =
>>> +                                       virtio_balloon_free_page_hint_notify;
>>> +        precopy_add_notifier(&s->free_page_hint_notify);
>>>          if (s->iothread) {
>>>              object_ref(OBJECT(s->iothread));
>>>              s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
>>> @@ -827,7 +827,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
>>>      if (virtio_balloon_free_page_support(s)) {
>>>          qemu_bh_delete(s->free_page_bh);
>>>          virtio_balloon_free_page_stop(s);
>>> -        precopy_remove_notifier(&s->free_page_report_notify);
>>> +        precopy_remove_notifier(&s->free_page_hint_notify);
>>>      }
>>>      balloon_stats_destroy_timer(s);
>>>      qemu_remove_balloon_handler(s);
>>> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
>>> index d1c968d2376e..108cff97e71a 100644
>>> --- a/include/hw/virtio/virtio-balloon.h
>>> +++ b/include/hw/virtio/virtio-balloon.h
>>> @@ -23,7 +23,7 @@
>>>  #define VIRTIO_BALLOON(obj) \
>>>          OBJECT_CHECK(VirtIOBalloon, (obj), TYPE_VIRTIO_BALLOON)
>>>
>>> -#define VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN 0x80000000
>>> +#define VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN 0x80000000
>>>
>>>  typedef struct virtio_balloon_stat VirtIOBalloonStat;
>>>
>>> @@ -33,20 +33,20 @@ typedef struct virtio_balloon_stat_modern {
>>>         uint64_t val;
>>>  } VirtIOBalloonStatModern;
>>>
>>> -enum virtio_balloon_free_page_report_status {
>>> -    FREE_PAGE_REPORT_S_STOP = 0,
>>> -    FREE_PAGE_REPORT_S_REQUESTED = 1,
>>> -    FREE_PAGE_REPORT_S_START = 2,
>>> -    FREE_PAGE_REPORT_S_DONE = 3,
>>> +enum virtio_balloon_free_page_hint_status {
>>> +    FREE_PAGE_HINT_S_STOP = 0,
>>> +    FREE_PAGE_HINT_S_REQUESTED = 1,
>>> +    FREE_PAGE_HINT_S_START = 2,
>>> +    FREE_PAGE_HINT_S_DONE = 3,
>>>  };
>>>
>>>  typedef struct VirtIOBalloon {
>>>      VirtIODevice parent_obj;
>>>      VirtQueue *ivq, *dvq, *svq, *free_page_vq;
>>> -    uint32_t free_page_report_status;
>>> +    uint32_t free_page_hint_status;
>>>      uint32_t num_pages;
>>>      uint32_t actual;
>>> -    uint32_t free_page_report_cmd_id;
>>> +    uint32_t free_page_hint_cmd_id;
>>>      uint64_t stats[VIRTIO_BALLOON_S_NR];
>>>      VirtQueueElement *stats_vq_elem;
>>>      size_t stats_vq_offset;
>>> @@ -55,7 +55,7 @@ typedef struct VirtIOBalloon {
>>>      QEMUBH *free_page_bh;
>>>      /*
>>>       * Lock to synchronize threads to access the free page reporting related
>>> -     * fields (e.g. free_page_report_status).
>>> +     * fields (e.g. free_page_hint_status).
>>>       */
>>>      QemuMutex free_page_lock;
>>>      QemuCond  free_page_cond;
>>> @@ -64,7 +64,7 @@ typedef struct VirtIOBalloon {
>>>       * stopped.
>>>       */
>>>      bool block_iothread;
>>> -    NotifierWithReturn free_page_report_notify;
>>> +    NotifierWithReturn free_page_hint_notify;
>>>      int64_t stats_last_update;
>>>      int64_t stats_poll_interval;
>>>      uint32_t host_features;
>>>
>>
>> Maybe split out the change to "free_page_report_cmd_id" in the uapi
>> (meaning: move that to a separate patch). So you can move forward with
>> most of this series (moving patch #1 and the free_page_report_cmd_id
>> change to the very last of this series) without depending on that change
>> to go upstream.
> 
> Okay. I can split it if that is needed. Any specific reason for
> splitting it I should cite in the patch? From what I can tell Michael
> has already accepted the renamed and pushed it to Linus.

Oh, I missed that - forget what I said. I thought this would still be
stuck in -next.

Acked-by: David Hildenbrand <david@redhat.com>

Thanks!

-- 
Thanks,

David / dhildenb

[virtio-dev] Re: [PATCH v21 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'

[David Hildenbrand]

On 24.04.20 16:56, Alexander Duyck wrote:
> On Fri, Apr 24, 2020 at 4:23 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 22.04.20 20:21, Alexander Duyck wrote:
>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>
>>> In an upcoming patch a feature named Free Page Reporting is about to be
>>> added. In order to avoid any confusion we should drop the use of the word
>>> 'report' when referring to Free Page Hinting. So what this patch does is go
>>> through and replace all instances of 'report' with 'hint" when we are
>>> referring to free page hinting.
>>>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>> ---
>>>  hw/virtio/virtio-balloon.c         |   74 ++++++++++++++++++------------------
>>>  include/hw/virtio/virtio-balloon.h |   20 +++++-----
>>>  2 files changed, 47 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>>> index a4729f7fc930..a1d6fb52c876 100644
>>> --- a/hw/virtio/virtio-balloon.c
>>> +++ b/hw/virtio/virtio-balloon.c
>>> @@ -466,21 +466,21 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>>>              ret = false;
>>>              goto out;
>>>          }
>>> -        if (id == dev->free_page_report_cmd_id) {
>>> -            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
>>> +        if (id == dev->free_page_hint_cmd_id) {
>>> +            dev->free_page_hint_status = FREE_PAGE_HINT_S_START;
>>>          } else {
>>>              /*
>>>               * Stop the optimization only when it has started. This
>>>               * avoids a stale stop sign for the previous command.
>>>               */
>>> -            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
>>> -                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
>>> +            if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
>>> +                dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
>>>              }
>>>          }
>>>      }
>>>
>>>      if (elem->in_num) {
>>> -        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
>>> +        if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
>>>              qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
>>>                                        elem->in_sg[0].iov_len);
>>>          }
>>> @@ -506,11 +506,11 @@ static void virtio_ballloon_get_free_page_hints(void *opaque)
>>>          qemu_mutex_unlock(&dev->free_page_lock);
>>>          virtio_notify(vdev, vq);
>>>        /*
>>> -       * Start to poll the vq once the reporting started. Otherwise, continue
>>> +       * Start to poll the vq once the hinting started. Otherwise, continue
>>>         * only when there are entries on the vq, which need to be given back.
>>>         */
>>>      } while (continue_to_get_hints ||
>>> -             dev->free_page_report_status == FREE_PAGE_REPORT_S_START);
>>> +             dev->free_page_hint_status == FREE_PAGE_HINT_S_START);
>>>      virtio_queue_set_notification(vq, 1);
>>>  }
>>>
>>> @@ -531,14 +531,14 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
>>>          return;
>>>      }
>>>
>>> -    if (s->free_page_report_cmd_id == UINT_MAX) {
>>> -        s->free_page_report_cmd_id =
>>> -                       VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
>>> +    if (s->free_page_hint_cmd_id == UINT_MAX) {
>>> +        s->free_page_hint_cmd_id =
>>> +                       VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
>>>      } else {
>>> -        s->free_page_report_cmd_id++;
>>> +        s->free_page_hint_cmd_id++;
>>>      }
>>>
>>> -    s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
>>> +    s->free_page_hint_status = FREE_PAGE_HINT_S_REQUESTED;
>>>      virtio_notify_config(vdev);
>>>  }
>>>
>>> @@ -546,18 +546,18 @@ static void virtio_balloon_free_page_stop(VirtIOBalloon *s)
>>>  {
>>>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
>>>
>>> -    if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
>>> +    if (s->free_page_hint_status != FREE_PAGE_HINT_S_STOP) {
>>>          /*
>>>           * The lock also guarantees us that the
>>>           * virtio_ballloon_get_free_page_hints exits after the
>>> -         * free_page_report_status is set to S_STOP.
>>> +         * free_page_hint_status is set to S_STOP.
>>>           */
>>>          qemu_mutex_lock(&s->free_page_lock);
>>>          /*
>>>           * The guest hasn't done the reporting, so host sends a notification
>>>           * to the guest to actively stop the reporting.
>>>           */
>>> -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
>>> +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
>>>          qemu_mutex_unlock(&s->free_page_lock);
>>>          virtio_notify_config(vdev);
>>>      }
>>> @@ -567,15 +567,15 @@ static void virtio_balloon_free_page_done(VirtIOBalloon *s)
>>>  {
>>>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
>>>
>>> -    s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
>>> +    s->free_page_hint_status = FREE_PAGE_HINT_S_DONE;
>>>      virtio_notify_config(vdev);
>>>  }
>>>
>>>  static int
>>> -virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
>>> +virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
>>>  {
>>>      VirtIOBalloon *dev = container_of(n, VirtIOBalloon,
>>> -                                      free_page_report_notify);
>>> +                                      free_page_hint_notify);
>>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>      PrecopyNotifyData *pnd = data;
>>>
>>> @@ -624,7 +624,7 @@ static size_t virtio_balloon_config_size(VirtIOBalloon *s)
>>>      if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>>>          return offsetof(struct virtio_balloon_config, poison_val);
>>>      }
>>> -    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
>>> +    return offsetof(struct virtio_balloon_config, free_page_hint_cmd_id);
>>>  }
>>>
>>>  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>>> @@ -635,14 +635,14 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>>>      config.num_pages = cpu_to_le32(dev->num_pages);
>>>      config.actual = cpu_to_le32(dev->actual);
>>>
>>> -    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
>>> -        config.free_page_report_cmd_id =
>>> -                       cpu_to_le32(dev->free_page_report_cmd_id);
>>> -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
>>> -        config.free_page_report_cmd_id =
>>> +    if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
>>> +        config.free_page_hint_cmd_id =
>>> +                       cpu_to_le32(dev->free_page_hint_cmd_id);
>>> +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_STOP) {
>>> +        config.free_page_hint_cmd_id =
>>>                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_STOP);
>>> -    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE) {
>>> -        config.free_page_report_cmd_id =
>>> +    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_DONE) {
>>> +        config.free_page_hint_cmd_id =
>>>                         cpu_to_le32(VIRTIO_BALLOON_CMD_ID_DONE);
>>>      }
>>>
>>> @@ -743,14 +743,14 @@ static int virtio_balloon_post_load_device(void *opaque, int version_id)
>>>      return 0;
>>>  }
>>>
>>> -static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
>>> +static const VMStateDescription vmstate_virtio_balloon_free_page_hint = {
>>>      .name = "virtio-balloon-device/free-page-report",
>>>      .version_id = 1,
>>>      .minimum_version_id = 1,
>>>      .needed = virtio_balloon_free_page_support,
>>>      .fields = (VMStateField[]) {
>>> -        VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
>>> -        VMSTATE_UINT32(free_page_report_status, VirtIOBalloon),
>>> +        VMSTATE_UINT32(free_page_hint_cmd_id, VirtIOBalloon),
>>> +        VMSTATE_UINT32(free_page_hint_status, VirtIOBalloon),
>>>          VMSTATE_END_OF_LIST()
>>>      }
>>>  };
>>> @@ -766,7 +766,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
>>>          VMSTATE_END_OF_LIST()
>>>      },
>>>      .subsections = (const VMStateDescription * []) {
>>> -        &vmstate_virtio_balloon_free_page_report,
>>> +        &vmstate_virtio_balloon_free_page_hint,
>>>          NULL
>>>      }
>>>  };
>>> @@ -797,12 +797,12 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>>>                             VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>>>          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
>>>                                             virtio_balloon_handle_free_page_vq);
>>> -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
>>> -        s->free_page_report_cmd_id =
>>> -                           VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
>>> -        s->free_page_report_notify.notify =
>>> -                                       virtio_balloon_free_page_report_notify;
>>> -        precopy_add_notifier(&s->free_page_report_notify);
>>> +        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
>>> +        s->free_page_hint_cmd_id =
>>> +                           VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
>>> +        s->free_page_hint_notify.notify =
>>> +                                       virtio_balloon_free_page_hint_notify;
>>> +        precopy_add_notifier(&s->free_page_hint_notify);
>>>          if (s->iothread) {
>>>              object_ref(OBJECT(s->iothread));
>>>              s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
>>> @@ -827,7 +827,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
>>>      if (virtio_balloon_free_page_support(s)) {
>>>          qemu_bh_delete(s->free_page_bh);
>>>          virtio_balloon_free_page_stop(s);
>>> -        precopy_remove_notifier(&s->free_page_report_notify);
>>> +        precopy_remove_notifier(&s->free_page_hint_notify);
>>>      }
>>>      balloon_stats_destroy_timer(s);
>>>      qemu_remove_balloon_handler(s);
>>> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
>>> index d1c968d2376e..108cff97e71a 100644
>>> --- a/include/hw/virtio/virtio-balloon.h
>>> +++ b/include/hw/virtio/virtio-balloon.h
>>> @@ -23,7 +23,7 @@
>>>  #define VIRTIO_BALLOON(obj) \
>>>          OBJECT_CHECK(VirtIOBalloon, (obj), TYPE_VIRTIO_BALLOON)
>>>
>>> -#define VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN 0x80000000
>>> +#define VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN 0x80000000
>>>
>>>  typedef struct virtio_balloon_stat VirtIOBalloonStat;
>>>
>>> @@ -33,20 +33,20 @@ typedef struct virtio_balloon_stat_modern {
>>>         uint64_t val;
>>>  } VirtIOBalloonStatModern;
>>>
>>> -enum virtio_balloon_free_page_report_status {
>>> -    FREE_PAGE_REPORT_S_STOP = 0,
>>> -    FREE_PAGE_REPORT_S_REQUESTED = 1,
>>> -    FREE_PAGE_REPORT_S_START = 2,
>>> -    FREE_PAGE_REPORT_S_DONE = 3,
>>> +enum virtio_balloon_free_page_hint_status {
>>> +    FREE_PAGE_HINT_S_STOP = 0,
>>> +    FREE_PAGE_HINT_S_REQUESTED = 1,
>>> +    FREE_PAGE_HINT_S_START = 2,
>>> +    FREE_PAGE_HINT_S_DONE = 3,
>>>  };
>>>
>>>  typedef struct VirtIOBalloon {
>>>      VirtIODevice parent_obj;
>>>      VirtQueue *ivq, *dvq, *svq, *free_page_vq;
>>> -    uint32_t free_page_report_status;
>>> +    uint32_t free_page_hint_status;
>>>      uint32_t num_pages;
>>>      uint32_t actual;
>>> -    uint32_t free_page_report_cmd_id;
>>> +    uint32_t free_page_hint_cmd_id;
>>>      uint64_t stats[VIRTIO_BALLOON_S_NR];
>>>      VirtQueueElement *stats_vq_elem;
>>>      size_t stats_vq_offset;
>>> @@ -55,7 +55,7 @@ typedef struct VirtIOBalloon {
>>>      QEMUBH *free_page_bh;
>>>      /*
>>>       * Lock to synchronize threads to access the free page reporting related
>>> -     * fields (e.g. free_page_report_status).
>>> +     * fields (e.g. free_page_hint_status).
>>>       */
>>>      QemuMutex free_page_lock;
>>>      QemuCond  free_page_cond;
>>> @@ -64,7 +64,7 @@ typedef struct VirtIOBalloon {
>>>       * stopped.
>>>       */
>>>      bool block_iothread;
>>> -    NotifierWithReturn free_page_report_notify;
>>> +    NotifierWithReturn free_page_hint_notify;
>>>      int64_t stats_last_update;
>>>      int64_t stats_poll_interval;
>>>      uint32_t host_features;
>>>
>>
>> Maybe split out the change to "free_page_report_cmd_id" in the uapi
>> (meaning: move that to a separate patch). So you can move forward with
>> most of this series (moving patch #1 and the free_page_report_cmd_id
>> change to the very last of this series) without depending on that change
>> to go upstream.
> 
> Okay. I can split it if that is needed. Any specific reason for
> splitting it I should cite in the patch? From what I can tell Michael
> has already accepted the renamed and pushed it to Linus.

Oh, I missed that - forget what I said. I thought this would still be
stuck in -next.

Acked-by: David Hildenbrand <david@redhat.com>

Thanks!

-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 5/5] virtio-balloon: Provide an interface for free page reporting

[Alexander Duyck]

On Fri, Apr 24, 2020 at 4:20 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 22.04.20 20:21, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Add support for free page reporting. The idea is to function very similar
> > to how the balloon works in that we basically end up madvising the page as
> > not being used. However we don't really need to bother with any deflate
> > type logic since the page will be faulted back into the guest when it is
> > read or written to.
> >
> > This provides a new way of letting the guest proactively report free
> > pages to the hypervisor, so the hypervisor can reuse them. In contrast to
> > inflate/deflate that is triggered via the hypervisor explicitly.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  hw/virtio/virtio-balloon.c         |   70 ++++++++++++++++++++++++++++++++++++
> >  include/hw/virtio/virtio-balloon.h |    2 +
> >  2 files changed, 71 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 5effc8b4653b..b473ff7f4b88 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -321,6 +321,60 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
> >      balloon_stats_change_timer(s, 0);
> >  }
> >
> > +static void virtio_balloon_handle_report(VirtIODevice *vdev, VirtQueue *vq)
> > +{
> > +    VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
> > +    VirtQueueElement *elem;
> > +
> > +    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
> > +        unsigned int i;
> > +
>
> Maybe add a comment like
>
> /*
>  * As discarded pages will be zero when re-accessed, all pages either
>  * have the old value, or were zeroed out. In case the guest expects
>  * another value, make sure to never discard.
>  */
>
> Whatever you think is best.

Okay I will add the following comment:
        /*
         * When we discard the page it has the effect of removing the page
         * from the hypervisor itself and causing it to be zeroed when it
         * is returned to us. So we must not discard the page if it is
         * accessible by another device or process, or if the guest is
         * expecting it to retain a non-zero value.
         */


> > +        if (qemu_balloon_is_inhibited() || dev->poison_val) {
> > +            goto skip_element;
> > +        }
> > +
> > +        for (i = 0; i < elem->in_num; i++) {
> > +            void *addr = elem->in_sg[i].iov_base;
> > +            size_t size = elem->in_sg[i].iov_len;
> > +            ram_addr_t ram_offset;
> > +            RAMBlock *rb;
> > +
> > +            /*
> > +             * There is no need to check the memory section to see if
> > +             * it is ram/readonly/romd like there is for handle_output
> > +             * below. If the region is not meant to be written to then
> > +             * address_space_map will have allocated a bounce buffer
> > +             * and it will be freed in address_space_unmap and trigger
> > +             * and unassigned_mem_write before failing to copy over the
> > +             * buffer. If more than one bad descriptor is provided it
> > +             * will return NULL after the first bounce buffer and fail
> > +             * to map any resources.
> > +             */
> > +            rb = qemu_ram_block_from_host(addr, false, &ram_offset);
> > +            if (!rb) {
> > +                trace_virtio_balloon_bad_addr(elem->in_addr[i]);
> > +                continue;
> > +            }
> > +
> > +            /*
> > +             * For now we will simply ignore unaligned memory regions, or
> > +             * regions that overrun the end of the RAMBlock.
> > +             */
> > +            if (!QEMU_IS_ALIGNED(ram_offset | size, qemu_ram_pagesize(rb)) ||
> > +                (ram_offset + size) > qemu_ram_get_used_length(rb)) {
> > +                continue;
> > +            }
> > +
> > +            ram_block_discard_range(rb, ram_offset, size);
> > +        }
> > +
> > +skip_element:
> > +        virtqueue_push(vq, elem, 0);
> > +        virtio_notify(vdev, vq);
> > +        g_free(elem);
> > +    }
> > +}
> > +
> >  static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> >  {
> >      VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> > @@ -782,6 +836,16 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
> >      int ret;
> >
> > +    /*
> > +     * Page reporting is dependant on page poison to make sure we can
> > +     * report a page without changing the state of the internal data.
> > +     * We need to set the flag before we call virtio_init as it will
> > +     * affect the config size of the vdev.
> > +     */
> > +    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
> > +        s->host_features |= 1 << VIRTIO_BALLOON_F_PAGE_POISON;
> > +    }
> > +
>
> As discussed, this hunk would go away. With that, this patch is really
> minimal, which is good :)

I have already removed it. :-)

> >      virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
> >                  virtio_balloon_config_size(s));
> >
> > @@ -798,6 +862,10 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
> >      s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
> >
> > +    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
> > +        s->rvq = virtio_add_queue(vdev, 32, virtio_balloon_handle_report);
> > +    }
> > +
> >      if (virtio_has_feature(s->host_features,
> >                             VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> >          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
> > @@ -923,6 +991,8 @@ static Property virtio_balloon_properties[] = {
> >                      VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
> >      DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
> >                      VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
> > +    DEFINE_PROP_BIT("free-page-reporting", VirtIOBalloon, host_features,
> > +                    VIRTIO_BALLOON_F_REPORTING, true),
>
> I think you'll have to similarly disable it via compat machines if you
> want to default enable. Otherwise, backward migration would be broken.

Yes, I realized that after you mentioned it for poison yesterday.

> Also, I do wonder if we want to default-enable it. It can still have a
> negative performance impact and some people might not want that.

The negative performance impact should be minimal. At this point the
hinting process ends up being very light since it only processes a
small chunk of the memory before it shuts down for a couple seconds.
In my original data the only time any significant performance
regression was seen was with page shuffling enabled, and that was
before I put limits on how many pages we could process per pass and
how frequently we would make a pass through memory. My thought is that
we are much better having it enabled by default rather than having it
disabled by default. In the worst case scenario we have a little bit
of extra thread noise from it popping up running for a few
milliseconds and then sleeping for about two seconds, but the benefit
side is that the VMs will do us a favor and restrict themselves to a
much smaller idle footprint until such time as the memory is actually
needed in the guest.

> Apart from that, looks good to me. Nothing else we have to migrate AFAIKs.

Thanks for the review.

- Alex

[virtio-dev] Re: [PATCH v21 QEMU 5/5] virtio-balloon: Provide an interface for free page reporting

[Alexander Duyck]

On Fri, Apr 24, 2020 at 4:20 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 22.04.20 20:21, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Add support for free page reporting. The idea is to function very similar
> > to how the balloon works in that we basically end up madvising the page as
> > not being used. However we don't really need to bother with any deflate
> > type logic since the page will be faulted back into the guest when it is
> > read or written to.
> >
> > This provides a new way of letting the guest proactively report free
> > pages to the hypervisor, so the hypervisor can reuse them. In contrast to
> > inflate/deflate that is triggered via the hypervisor explicitly.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  hw/virtio/virtio-balloon.c         |   70 ++++++++++++++++++++++++++++++++++++
> >  include/hw/virtio/virtio-balloon.h |    2 +
> >  2 files changed, 71 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 5effc8b4653b..b473ff7f4b88 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -321,6 +321,60 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
> >      balloon_stats_change_timer(s, 0);
> >  }
> >
> > +static void virtio_balloon_handle_report(VirtIODevice *vdev, VirtQueue *vq)
> > +{
> > +    VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
> > +    VirtQueueElement *elem;
> > +
> > +    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
> > +        unsigned int i;
> > +
>
> Maybe add a comment like
>
> /*
>  * As discarded pages will be zero when re-accessed, all pages either
>  * have the old value, or were zeroed out. In case the guest expects
>  * another value, make sure to never discard.
>  */
>
> Whatever you think is best.

Okay I will add the following comment:
        /*
         * When we discard the page it has the effect of removing the page
         * from the hypervisor itself and causing it to be zeroed when it
         * is returned to us. So we must not discard the page if it is
         * accessible by another device or process, or if the guest is
         * expecting it to retain a non-zero value.
         */


> > +        if (qemu_balloon_is_inhibited() || dev->poison_val) {
> > +            goto skip_element;
> > +        }
> > +
> > +        for (i = 0; i < elem->in_num; i++) {
> > +            void *addr = elem->in_sg[i].iov_base;
> > +            size_t size = elem->in_sg[i].iov_len;
> > +            ram_addr_t ram_offset;
> > +            RAMBlock *rb;
> > +
> > +            /*
> > +             * There is no need to check the memory section to see if
> > +             * it is ram/readonly/romd like there is for handle_output
> > +             * below. If the region is not meant to be written to then
> > +             * address_space_map will have allocated a bounce buffer
> > +             * and it will be freed in address_space_unmap and trigger
> > +             * and unassigned_mem_write before failing to copy over the
> > +             * buffer. If more than one bad descriptor is provided it
> > +             * will return NULL after the first bounce buffer and fail
> > +             * to map any resources.
> > +             */
> > +            rb = qemu_ram_block_from_host(addr, false, &ram_offset);
> > +            if (!rb) {
> > +                trace_virtio_balloon_bad_addr(elem->in_addr[i]);
> > +                continue;
> > +            }
> > +
> > +            /*
> > +             * For now we will simply ignore unaligned memory regions, or
> > +             * regions that overrun the end of the RAMBlock.
> > +             */
> > +            if (!QEMU_IS_ALIGNED(ram_offset | size, qemu_ram_pagesize(rb)) ||
> > +                (ram_offset + size) > qemu_ram_get_used_length(rb)) {
> > +                continue;
> > +            }
> > +
> > +            ram_block_discard_range(rb, ram_offset, size);
> > +        }
> > +
> > +skip_element:
> > +        virtqueue_push(vq, elem, 0);
> > +        virtio_notify(vdev, vq);
> > +        g_free(elem);
> > +    }
> > +}
> > +
> >  static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> >  {
> >      VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> > @@ -782,6 +836,16 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
> >      int ret;
> >
> > +    /*
> > +     * Page reporting is dependant on page poison to make sure we can
> > +     * report a page without changing the state of the internal data.
> > +     * We need to set the flag before we call virtio_init as it will
> > +     * affect the config size of the vdev.
> > +     */
> > +    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
> > +        s->host_features |= 1 << VIRTIO_BALLOON_F_PAGE_POISON;
> > +    }
> > +
>
> As discussed, this hunk would go away. With that, this patch is really
> minimal, which is good :)

I have already removed it. :-)

> >      virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
> >                  virtio_balloon_config_size(s));
> >
> > @@ -798,6 +862,10 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
> >      s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
> >
> > +    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
> > +        s->rvq = virtio_add_queue(vdev, 32, virtio_balloon_handle_report);
> > +    }
> > +
> >      if (virtio_has_feature(s->host_features,
> >                             VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> >          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
> > @@ -923,6 +991,8 @@ static Property virtio_balloon_properties[] = {
> >                      VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
> >      DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
> >                      VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
> > +    DEFINE_PROP_BIT("free-page-reporting", VirtIOBalloon, host_features,
> > +                    VIRTIO_BALLOON_F_REPORTING, true),
>
> I think you'll have to similarly disable it via compat machines if you
> want to default enable. Otherwise, backward migration would be broken.

Yes, I realized that after you mentioned it for poison yesterday.

> Also, I do wonder if we want to default-enable it. It can still have a
> negative performance impact and some people might not want that.

The negative performance impact should be minimal. At this point the
hinting process ends up being very light since it only processes a
small chunk of the memory before it shuts down for a couple seconds.
In my original data the only time any significant performance
regression was seen was with page shuffling enabled, and that was
before I put limits on how many pages we could process per pass and
how frequently we would make a pass through memory. My thought is that
we are much better having it enabled by default rather than having it
disabled by default. In the worst case scenario we have a little bit
of extra thread noise from it popping up running for a few
milliseconds and then sleeping for about two seconds, but the benefit
side is that the VMs will do us a favor and restrict themselves to a
much smaller idle footprint until such time as the memory is actually
needed in the guest.

> Apart from that, looks good to me. Nothing else we have to migrate AFAIKs.

Thanks for the review.

- Alex

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 5/5] virtio-balloon: Provide an interface for free page reporting

[David Hildenbrand]

>> Also, I do wonder if we want to default-enable it. It can still have a
>> negative performance impact and some people might not want that.
> 
> The negative performance impact should be minimal. At this point the
> hinting process ends up being very light since it only processes a
> small chunk of the memory before it shuts down for a couple seconds.
> In my original data the only time any significant performance
> regression was seen was with page shuffling enabled, and that was
> before I put limits on how many pages we could process per pass and
> how frequently we would make a pass through memory. My thought is that
> we are much better having it enabled by default rather than having it
> disabled by default. In the worst case scenario we have a little bit
> of extra thread noise from it popping up running for a few
> milliseconds and then sleeping for about two seconds, but the benefit
> side is that the VMs will do us a favor and restrict themselves to a
> much smaller idle footprint until such time as the memory is actually
> needed in the guest.

While I agree that the impact is small, there *is* a noticeable
performance impact. One of the main users is memory overcommit.

Also, imagine the following scenarios:
- RT workload in the guest. Just because you have a virtio-balloon
  device defined would mean something is suddenly active and
  discarding/trying to discard pages.
- vfio: free page reporting is completely useless right now and
  *only* overhead.
- prealloc does not expect that pages get suddenly discarded
- s390x, which has CMM and might not benefit at all (except when using
  huge pages as backing storage)

No, I don't think it's acceptable to enable this as default. I remember
that it is quite common to just define a balloon device but never use it.

E.g., "A virtual memory balloon device is added to all Xen and KVM/QEMU
guest virtual machines. It will be seen as <memballoon> element" [1].

I think we should let upper layers decide (just as we do for free page
hinting, for example).


[1]
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/virtualization_administration_guide/section-libvirt-dom-xml-memory-baloon-device

-- 
Thanks,

David / dhildenb

[virtio-dev] Re: [PATCH v21 QEMU 5/5] virtio-balloon: Provide an interface for free page reporting

[David Hildenbrand]

>> Also, I do wonder if we want to default-enable it. It can still have a
>> negative performance impact and some people might not want that.
> 
> The negative performance impact should be minimal. At this point the
> hinting process ends up being very light since it only processes a
> small chunk of the memory before it shuts down for a couple seconds.
> In my original data the only time any significant performance
> regression was seen was with page shuffling enabled, and that was
> before I put limits on how many pages we could process per pass and
> how frequently we would make a pass through memory. My thought is that
> we are much better having it enabled by default rather than having it
> disabled by default. In the worst case scenario we have a little bit
> of extra thread noise from it popping up running for a few
> milliseconds and then sleeping for about two seconds, but the benefit
> side is that the VMs will do us a favor and restrict themselves to a
> much smaller idle footprint until such time as the memory is actually
> needed in the guest.

While I agree that the impact is small, there *is* a noticeable
performance impact. One of the main users is memory overcommit.

Also, imagine the following scenarios:
- RT workload in the guest. Just because you have a virtio-balloon
  device defined would mean something is suddenly active and
  discarding/trying to discard pages.
- vfio: free page reporting is completely useless right now and
  *only* overhead.
- prealloc does not expect that pages get suddenly discarded
- s390x, which has CMM and might not benefit at all (except when using
  huge pages as backing storage)

No, I don't think it's acceptable to enable this as default. I remember
that it is quite common to just define a balloon device but never use it.

E.g., "A virtual memory balloon device is added to all Xen and KVM/QEMU
guest virtual machines. It will be seen as <memballoon> element" [1].

I think we should let upper layers decide (just as we do for free page
hinting, for example).


[1]
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/virtualization_administration_guide/section-libvirt-dom-xml-memory-baloon-device

-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v21 QEMU 5/5] virtio-balloon: Provide an interface for free page reporting

[Alexander Duyck]

On Fri, Apr 24, 2020 at 8:34 AM David Hildenbrand <david@redhat.com> wrote:
>
>
> >> Also, I do wonder if we want to default-enable it. It can still have a
> >> negative performance impact and some people might not want that.
> >
> > The negative performance impact should be minimal. At this point the
> > hinting process ends up being very light since it only processes a
> > small chunk of the memory before it shuts down for a couple seconds.
> > In my original data the only time any significant performance
> > regression was seen was with page shuffling enabled, and that was
> > before I put limits on how many pages we could process per pass and
> > how frequently we would make a pass through memory. My thought is that
> > we are much better having it enabled by default rather than having it
> > disabled by default. In the worst case scenario we have a little bit
> > of extra thread noise from it popping up running for a few
> > milliseconds and then sleeping for about two seconds, but the benefit
> > side is that the VMs will do us a favor and restrict themselves to a
> > much smaller idle footprint until such time as the memory is actually
> > needed in the guest.
>
> While I agree that the impact is small, there *is* a noticeable
> performance impact. One of the main users is memory overcommit.
>
> Also, imagine the following scenarios:
> - RT workload in the guest. Just because you have a virtio-balloon
>   device defined would mean something is suddenly active and
>   discarding/trying to discard pages.
> - vfio: free page reporting is completely useless right now and
>   *only* overhead.
> - prealloc does not expect that pages get suddenly discarded
> - s390x, which has CMM and might not benefit at all (except when using
>   huge pages as backing storage)
>
> No, I don't think it's acceptable to enable this as default. I remember
> that it is quite common to just define a balloon device but never use it.
>
> E.g., "A virtual memory balloon device is added to all Xen and KVM/QEMU
> guest virtual machines. It will be seen as <memballoon> element" [1].
>
> I think we should let upper layers decide (just as we do for free page
> hinting, for example).

Okay. I guess there are a number of other cases I hand't thought of. I
will switch the default to false.

Thanks.

- Alex

[virtio-dev] Re: [PATCH v21 QEMU 5/5] virtio-balloon: Provide an interface for free page reporting

[Alexander Duyck]

On Fri, Apr 24, 2020 at 8:34 AM David Hildenbrand <david@redhat.com> wrote:
>
>
> >> Also, I do wonder if we want to default-enable it. It can still have a
> >> negative performance impact and some people might not want that.
> >
> > The negative performance impact should be minimal. At this point the
> > hinting process ends up being very light since it only processes a
> > small chunk of the memory before it shuts down for a couple seconds.
> > In my original data the only time any significant performance
> > regression was seen was with page shuffling enabled, and that was
> > before I put limits on how many pages we could process per pass and
> > how frequently we would make a pass through memory. My thought is that
> > we are much better having it enabled by default rather than having it
> > disabled by default. In the worst case scenario we have a little bit
> > of extra thread noise from it popping up running for a few
> > milliseconds and then sleeping for about two seconds, but the benefit
> > side is that the VMs will do us a favor and restrict themselves to a
> > much smaller idle footprint until such time as the memory is actually
> > needed in the guest.
>
> While I agree that the impact is small, there *is* a noticeable
> performance impact. One of the main users is memory overcommit.
>
> Also, imagine the following scenarios:
> - RT workload in the guest. Just because you have a virtio-balloon
>   device defined would mean something is suddenly active and
>   discarding/trying to discard pages.
> - vfio: free page reporting is completely useless right now and
>   *only* overhead.
> - prealloc does not expect that pages get suddenly discarded
> - s390x, which has CMM and might not benefit at all (except when using
>   huge pages as backing storage)
>
> No, I don't think it's acceptable to enable this as default. I remember
> that it is quite common to just define a balloon device but never use it.
>
> E.g., "A virtual memory balloon device is added to all Xen and KVM/QEMU
> guest virtual machines. It will be seen as <memballoon> element" [1].
>
> I think we should let upper layers decide (just as we do for free page
> hinting, for example).

Okay. I guess there are a number of other cases I hand't thought of. I
will switch the default to false.

Thanks.

- Alex

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org