[Qemu-devel] [RFC] virtio: Report new guest memory statistics pertinent to memory ballooning (V2)

[Qemu-devel] [RFC] virtio: Report new guest memory statistics pertinent to memory ballooning (V2)

[Adam Litke]

Changes since V1:
- In the monitor, print all stats on one line with less abbreviated names
- Coding style changes

When using ballooning to manage overcommitted memory on a host, a system for
guests to communicate their memory usage to the host can provide information
that will minimize the impact of ballooning on the guests.  The current method
employs a daemon running in each guest that communicates memory statistics to a
host daemon at a specified time interval.  The host daemon aggregates this
information and inflates and/or deflates balloons according to the level of
host memory pressure.  This approach is effective but overly complex since a
daemon must be installed inside each guest and coordinated to communicate with
the host.  A simpler approach is to collect memory statistics in the virtio
balloon driver and communicate them to the host via the device config space.

This patch implements the qemu side of the communication channel.  I will post
the kernel driver modifications in-reply to this message.

Signed-off-by: Adam Litke <agl@us.ibm.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Cc: Avi Kivity <avi@redhat.com>

diff --git a/balloon.h b/balloon.h
index 60b4a5d..4008d1b 100644
--- a/balloon.h
+++ b/balloon.h
@@ -14,14 +14,21 @@
 #ifndef _QEMU_BALLOON_H
 #define _QEMU_BALLOON_H
 
+#include "hw/virtio-balloon.h"
 #include "cpu-defs.h"
 
-typedef ram_addr_t (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
+typedef struct QEMUBalloonState {
+    ram_addr_t actual;
+    virtio_balloon_stats stats;
+} QEMUBalloonState;
+
+typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target,
+                                      QEMUBalloonState *state);
 
 void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque);
 
 void qemu_balloon(ram_addr_t target);
 
-ram_addr_t qemu_balloon_status(void);
+int qemu_balloon_status(QEMUBalloonState *s);
 
 #endif
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index cfd3b41..3d74e7f 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -30,6 +30,7 @@ typedef struct VirtIOBalloon
     VirtQueue *ivq, *dvq;
     uint32_t num_pages;
     uint32_t actual;
+    virtio_balloon_stats stats;
 } VirtIOBalloon;
 
 static VirtIOBalloon *to_virtio_balloon(VirtIODevice *vdev)
@@ -111,8 +112,15 @@ 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);
-
-    memcpy(config_data, &config, 8);
+    config.stats.pswapin = cpu_to_le32(dev->stats.pswapin);
+    config.stats.pswapout = cpu_to_le32(dev->stats.pswapout);
+    config.stats.panon = cpu_to_le32(dev->stats.panon);
+    config.stats.pgmajfault = cpu_to_le32(dev->stats.pgmajfault);
+    config.stats.pgminfault = cpu_to_le32(dev->stats.pgminfault);
+    config.stats.memfree = cpu_to_le32(dev->stats.memfree);
+    config.stats.memtot = cpu_to_le32(dev->stats.memtot);
+
+    memcpy(config_data, &config, sizeof(config));
 }
 
 static void virtio_balloon_set_config(VirtIODevice *vdev,
@@ -120,16 +128,39 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
 {
     VirtIOBalloon *dev = to_virtio_balloon(vdev);
     struct virtio_balloon_config config;
-    memcpy(&config, config_data, 8);
+    memcpy(&config, config_data, sizeof(config));
     dev->actual = config.actual;
+    dev->stats.pswapin = config.stats.pswapin;
+    dev->stats.pswapout = config.stats.pswapout;
+    dev->stats.panon = config.stats.panon;
+    dev->stats.pgmajfault = config.stats.pgmajfault;
+    dev->stats.pgminfault = config.stats.pgminfault;
+    dev->stats.memfree = config.stats.memfree;
+    dev->stats.memtot = config.stats.memtot;
 }
 
 static uint32_t virtio_balloon_get_features(VirtIODevice *vdev)
 {
-    return 0;
+    uint32_t features = 0;
+
+    features |= (1 << VIRTIO_BALLOON_F_RPT_SWAP_IN)  |
+                (1 << VIRTIO_BALLOON_F_RPT_SWAP_OUT) |
+                (1 << VIRTIO_BALLOON_F_RPT_ANON)     |
+                (1 << VIRTIO_BALLOON_F_RPT_MAJFLT)   |
+                (1 << VIRTIO_BALLOON_F_RPT_MINFLT)   |
+                (1 << VIRTIO_BALLOON_F_RPT_MEMFREE)  |
+                (1 << VIRTIO_BALLOON_F_RPT_MEMTOT);
+		
+    return features;
+}
+
+static inline int has_feature(VirtIOBalloon *dev, int feature)
+{
+	return (dev->vdev.features & 1<<feature);
 }
 
-static ram_addr_t virtio_balloon_to_target(void *opaque, ram_addr_t target)
+static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
+                                     QEMUBalloonState *s)
 {
     VirtIOBalloon *dev = opaque;
 
@@ -141,7 +172,23 @@ static ram_addr_t virtio_balloon_to_target(void *opaque, ram_addr_t target)
         virtio_notify_config(&dev->vdev);
     }
 
-    return ram_size - (dev->actual << VIRTIO_BALLOON_PFN_SHIFT);
+    if (s) {
+        s->actual = ram_size - (dev->actual << VIRTIO_BALLOON_PFN_SHIFT);
+        s->stats.pswapin = has_feature(dev, VIRTIO_BALLOON_F_RPT_SWAP_OUT) ?
+                                       dev->stats.pswapin : -1;
+        s->stats.pswapout = has_feature(dev, VIRTIO_BALLOON_F_RPT_SWAP_OUT) ?
+                                        dev->stats.pswapout : -1;  
+        s->stats.panon = has_feature(dev, VIRTIO_BALLOON_F_RPT_ANON) ?
+                                     dev->stats.panon : -1;
+        s->stats.pgmajfault = has_feature(dev, VIRTIO_BALLOON_F_RPT_MAJFLT) ?
+                                          dev->stats.pgmajfault : -1;
+        s->stats.pgminfault = has_feature(dev, VIRTIO_BALLOON_F_RPT_MINFLT) ?
+                                          dev->stats.pgminfault : -1;
+        s->stats.memfree = has_feature(dev, VIRTIO_BALLOON_F_RPT_MEMFREE) ?
+                                       dev->stats.memfree : -1;
+        s->stats.memtot = has_feature(dev, VIRTIO_BALLOON_F_RPT_MEMTOT) ?
+                                      dev->stats.memtot : -1;
+    }
 }
 
 static void virtio_balloon_save(QEMUFile *f, void *opaque)
@@ -174,8 +221,9 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
     VirtIOBalloon *s;
 
     s = (VirtIOBalloon *)virtio_common_init("virtio-balloon",
-                                            VIRTIO_ID_BALLOON,
-                                            8, sizeof(VirtIOBalloon));
+                             VIRTIO_ID_BALLOON,
+                             sizeof(struct virtio_balloon_config),
+                             sizeof(VirtIOBalloon));
 
     s->vdev.get_config = virtio_balloon_get_config;
     s->vdev.set_config = virtio_balloon_set_config;
diff --git a/hw/virtio-balloon.h b/hw/virtio-balloon.h
index 9a0d119..7e635b7 100644
--- a/hw/virtio-balloon.h
+++ b/hw/virtio-balloon.h
@@ -25,16 +25,37 @@
 
 /* The feature bitmap for virtio balloon */
 #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
+                                        /* Guest memory statistic reporting */
+#define VIRTIO_BALLOON_F_RPT_SWAP_IN  1   /* Number of pages swapped in */
+#define VIRTIO_BALLOON_F_RPT_SWAP_OUT 2   /* Number of pages swapped out */
+#define VIRTIO_BALLOON_F_RPT_ANON     3   /* Number of anonymous pages in use */
+#define VIRTIO_BALLOON_F_RPT_MAJFLT   4   /* Number of major faults */
+#define VIRTIO_BALLOON_F_RPT_MINFLT   5   /* Number of minor faults */
+#define VIRTIO_BALLOON_F_RPT_MEMFREE  6   /* Total amount of free memory */
+#define VIRTIO_BALLOON_F_RPT_MEMTOT   7   /* Total amount of memory */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12
 
+typedef struct virtio_balloon_stats
+{
+    uint32_t pswapin;     /* pages swapped in */
+    uint32_t pswapout;    /* pages swapped out */
+    uint32_t panon;       /* anonymous pages in use */
+    uint32_t pgmajfault;  /* Major page faults */
+    uint32_t pgminfault;  /* Minor page faults */
+    uint32_t memfree;     /* Total amount of free memory (in kb) */
+    uint32_t memtot;      /* Total amount of memory (in kb) */
+} virtio_balloon_stats;
+
 struct virtio_balloon_config
 {
     /* Number of pages host wants Guest to give up. */
     uint32_t num_pages;
     /* Number of pages we've actually got in balloon. */
     uint32_t actual;
+    /* Memory statistics */
+    virtio_balloon_stats stats;
 };
 
 #endif
diff --git a/monitor.c b/monitor.c
index ed1ce6e..b071ee5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1581,18 +1581,36 @@ static void do_balloon(Monitor *mon, int value)
     qemu_balloon(target << 20);
 }
 
+static inline void print_stat(Monitor *mon, uint32_t val, const char *label)
+{
+    if (val != -1) {
+        monitor_printf(mon, ",%s=%u", label, val);
+    }
+}
+
 static void do_info_balloon(Monitor *mon)
 {
-    ram_addr_t actual;
+    QEMUBalloonState s;
+    int ret;
 
-    actual = qemu_balloon_status();
-    if (kvm_enabled() && !kvm_has_sync_mmu())
+    ret = qemu_balloon_status(&s);
+    if (kvm_enabled() && !kvm_has_sync_mmu()) {
         monitor_printf(mon, "Using KVM without synchronous MMU, "
                        "ballooning disabled\n");
-    else if (actual == 0)
+    } else if (ret < 0) {
         monitor_printf(mon, "Ballooning not activated in VM\n");
-    else
-        monitor_printf(mon, "balloon: actual=%d\n", (int)(actual >> 20));
+    } else {
+        monitor_printf(mon, "balloon: actual=%d", (int)(s.actual >> 20));
+        print_stat(mon, s.stats.pswapin, "pages_swapped_in");
+        print_stat(mon, s.stats.pswapout, "pages_swapped_out");
+        print_stat(mon, s.stats.panon, "anon_pages");
+        print_stat(mon, s.stats.pgmajfault, "major_page_faults");
+        print_stat(mon, s.stats.pgminfault, "minor_page_faults");
+        print_stat(mon, s.stats.memfree, "free_memory");
+        print_stat(mon, s.stats.memtot, "total_memory");
+        monitor_printf(mon, "\n");
+    }
+        
 }
 
 static qemu_acl *find_acl(Monitor *mon, const char *name)
diff --git a/vl.c b/vl.c
index 710d52e..de3b4ae 100644
--- a/vl.c
+++ b/vl.c
@@ -334,13 +334,13 @@ void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque)
 void qemu_balloon(ram_addr_t target)
 {
     if (qemu_balloon_event)
-        qemu_balloon_event(qemu_balloon_event_opaque, target);
+        qemu_balloon_event(qemu_balloon_event_opaque, target, NULL);
 }
 
-ram_addr_t qemu_balloon_status(void)
+int qemu_balloon_status(struct QEMUBalloonState *s)
 {
     if (qemu_balloon_event)
-        return qemu_balloon_event(qemu_balloon_event_opaque, 0);
+        qemu_balloon_event(qemu_balloon_event_opaque, 0, s);
     return 0;
 }


-- 
Thanks,
Adam

virtio: Add memory statistics reporting to the balloon driver

When using ballooning to manage overcommitted memory on a host, a system for
guests to communicate their memory usage to the host can provide information
that will minimize the impact of ballooning on the guests.  The current method
employs a daemon running in each guest that communicates memory statistics to a
host daemon at a specified time interval.  The host daemon aggregates this
information and inflates and/or deflates balloons according to the level of
host memory pressure.  This approach is effective but overly complex since a
daemon must be installed inside each guest and coordinated to communicate with
the host.  A simpler approach is to collect memory statistics in the virtio
balloon driver and communicate them to the host via the device config space.

This patch enables the guest-side support by adding stats collection and
reporting to the virtio balloon driver.

Comments?

Signed-off-by: Adam Litke <agl@us.ibm.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Avi Kivity <avi@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 200c22f..0c9a9a1 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -180,6 +180,41 @@ static void update_balloon_size(struct virtio_balloon *vb)
 			      &actual, sizeof(actual));
 }
 
+static inline void update_stat(struct virtio_device *vdev, int feature,
+				unsigned long value, unsigned offset)
+{
+	__le32 __v = cpu_to_le32(value);
+	if (virtio_has_feature(vdev, feature))
+		vdev->config->set(vdev, offset, &__v, sizeof(__v));
+}
+
+#define K(x) ((x) << (PAGE_SHIFT - 10))
+static void update_balloon_stats(struct virtio_balloon *vb)
+{
+	unsigned long events[NR_VM_EVENT_ITEMS];
+	struct sysinfo i;
+	unsigned off = offsetof(struct virtio_balloon_config, stats);
+
+	all_vm_events(events);
+
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_SWAP_IN, events[PSWPIN],
+		    off + offsetof(struct virtio_balloon_stats, pswapin));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_SWAP_OUT, events[PSWPOUT],
+		    off + offsetof(struct virtio_balloon_stats, pswapout));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_MAJFLT, events[PGMAJFAULT],
+		    off + offsetof(struct virtio_balloon_stats, pgmajfault));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_MINFLT, events[PGFAULT],
+		    off + offsetof(struct virtio_balloon_stats, pgminfault));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_ANON,
+		    K(global_page_state(NR_ANON_PAGES)),
+		    off + offsetof(struct virtio_balloon_stats, panon));
+	si_meminfo(&i);
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_MEMFREE, K(i.freeram),
+		    off + offsetof(struct virtio_balloon_stats, memfree));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_MEMTOT, K(i.totalram),
+		    off + offsetof(struct virtio_balloon_stats, memtot));
+}
+
 static int balloon(void *_vballoon)
 {
 	struct virtio_balloon *vb = _vballoon;
@@ -189,15 +224,17 @@ static int balloon(void *_vballoon)
 		s64 diff;
 
 		try_to_freeze();
-		wait_event_interruptible(vb->config_change,
+		wait_event_interruptible_timeout(vb->config_change,
 					 (diff = towards_target(vb)) != 0
 					 || kthread_should_stop()
-					 || freezing(current));
+					 || freezing(current),
+					 VIRTIO_BALLOON_TIMEOUT);
 		if (diff > 0)
 			fill_balloon(vb, diff);
 		else if (diff < 0)
 			leak_balloon(vb, -diff);
 		update_balloon_size(vb);
+		update_balloon_stats(vb);
 	}
 	return 0;
 }
@@ -265,7 +302,12 @@ static void virtballoon_remove(struct virtio_device *vdev)
 	kfree(vb);
 }
 
-static unsigned int features[] = { VIRTIO_BALLOON_F_MUST_TELL_HOST };
+static unsigned int features[] = {
+	VIRTIO_BALLOON_F_MUST_TELL_HOST, VIRTIO_BALLOON_F_RPT_SWAP_IN,
+	VIRTIO_BALLOON_F_RPT_SWAP_OUT, VIRTIO_BALLOON_F_RPT_ANON,
+	VIRTIO_BALLOON_F_RPT_MAJFLT, VIRTIO_BALLOON_F_RPT_MINFLT,
+	VIRTIO_BALLOON_F_RPT_MEMFREE, VIRTIO_BALLOON_F_RPT_MEMTOT,
+};
 
 static struct virtio_driver virtio_balloon = {
 	.feature_table = features,
diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
index 09d7300..0bff4b8 100644
--- a/include/linux/virtio_balloon.h
+++ b/include/linux/virtio_balloon.h
@@ -6,15 +6,39 @@
 
 /* The feature bitmap for virtio balloon */
 #define VIRTIO_BALLOON_F_MUST_TELL_HOST	0 /* Tell before reclaiming pages */
+                                          /* Guest memory statistic reporting */
+#define VIRTIO_BALLOON_F_RPT_SWAP_IN  1   /* Number of pages swapped in */
+#define VIRTIO_BALLOON_F_RPT_SWAP_OUT 2   /* Number of pages swapped out */
+#define VIRTIO_BALLOON_F_RPT_ANON     3   /* Number of anonymous pages in use */
+#define VIRTIO_BALLOON_F_RPT_MAJFLT   4   /* Number of major faults */
+#define VIRTIO_BALLOON_F_RPT_MINFLT   5   /* Number of minor faults */
+#define VIRTIO_BALLOON_F_RPT_MEMFREE  6   /* Total amount of free memory */
+#define VIRTIO_BALLOON_F_RPT_MEMTOT   7   /* Total amount of memory */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12
 
+struct virtio_balloon_stats
+{
+	__le32 pswapin;      /* pages swapped in */
+	__le32 pswapout;     /* pages swapped out */
+	__le32 panon;        /* anonymous pages in use (in kb) */
+	__le32 pgmajfault;   /* Major page faults */
+	__le32 pgminfault;   /* Minor page faults */
+	__le32 memfree;      /* Total amount of free memory (in kb) */
+	__le32 memtot;       /* Total amount of memory (in kb) */
+};
+
 struct virtio_balloon_config
 {
 	/* Number of pages host wants Guest to give up. */
 	__le32 num_pages;
 	/* Number of pages we've actually got in balloon. */
 	__le32 actual;
+	/* Memory statistics */
+	struct virtio_balloon_stats stats;
 };
+
+#define VIRTIO_BALLOON_TIMEOUT (30 * HZ)
+
 #endif /* _LINUX_VIRTIO_BALLOON_H */

-- 
Thanks,
Adam

[Qemu-devel] virtio: Add memory statistics reporting to the balloon driver

[Adam Litke]

When using ballooning to manage overcommitted memory on a host, a system for
guests to communicate their memory usage to the host can provide information
that will minimize the impact of ballooning on the guests.  The current method
employs a daemon running in each guest that communicates memory statistics to a
host daemon at a specified time interval.  The host daemon aggregates this
information and inflates and/or deflates balloons according to the level of
host memory pressure.  This approach is effective but overly complex since a
daemon must be installed inside each guest and coordinated to communicate with
the host.  A simpler approach is to collect memory statistics in the virtio
balloon driver and communicate them to the host via the device config space.

This patch enables the guest-side support by adding stats collection and
reporting to the virtio balloon driver.

Comments?

Signed-off-by: Adam Litke <agl@us.ibm.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Avi Kivity <avi@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 200c22f..0c9a9a1 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -180,6 +180,41 @@ static void update_balloon_size(struct virtio_balloon *vb)
 			      &actual, sizeof(actual));
 }
 
+static inline void update_stat(struct virtio_device *vdev, int feature,
+				unsigned long value, unsigned offset)
+{
+	__le32 __v = cpu_to_le32(value);
+	if (virtio_has_feature(vdev, feature))
+		vdev->config->set(vdev, offset, &__v, sizeof(__v));
+}
+
+#define K(x) ((x) << (PAGE_SHIFT - 10))
+static void update_balloon_stats(struct virtio_balloon *vb)
+{
+	unsigned long events[NR_VM_EVENT_ITEMS];
+	struct sysinfo i;
+	unsigned off = offsetof(struct virtio_balloon_config, stats);
+
+	all_vm_events(events);
+
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_SWAP_IN, events[PSWPIN],
+		    off + offsetof(struct virtio_balloon_stats, pswapin));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_SWAP_OUT, events[PSWPOUT],
+		    off + offsetof(struct virtio_balloon_stats, pswapout));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_MAJFLT, events[PGMAJFAULT],
+		    off + offsetof(struct virtio_balloon_stats, pgmajfault));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_MINFLT, events[PGFAULT],
+		    off + offsetof(struct virtio_balloon_stats, pgminfault));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_ANON,
+		    K(global_page_state(NR_ANON_PAGES)),
+		    off + offsetof(struct virtio_balloon_stats, panon));
+	si_meminfo(&i);
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_MEMFREE, K(i.freeram),
+		    off + offsetof(struct virtio_balloon_stats, memfree));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_MEMTOT, K(i.totalram),
+		    off + offsetof(struct virtio_balloon_stats, memtot));
+}
+
 static int balloon(void *_vballoon)
 {
 	struct virtio_balloon *vb = _vballoon;
@@ -189,15 +224,17 @@ static int balloon(void *_vballoon)
 		s64 diff;
 
 		try_to_freeze();
-		wait_event_interruptible(vb->config_change,
+		wait_event_interruptible_timeout(vb->config_change,
 					 (diff = towards_target(vb)) != 0
 					 || kthread_should_stop()
-					 || freezing(current));
+					 || freezing(current),
+					 VIRTIO_BALLOON_TIMEOUT);
 		if (diff > 0)
 			fill_balloon(vb, diff);
 		else if (diff < 0)
 			leak_balloon(vb, -diff);
 		update_balloon_size(vb);
+		update_balloon_stats(vb);
 	}
 	return 0;
 }
@@ -265,7 +302,12 @@ static void virtballoon_remove(struct virtio_device *vdev)
 	kfree(vb);
 }
 
-static unsigned int features[] = { VIRTIO_BALLOON_F_MUST_TELL_HOST };
+static unsigned int features[] = {
+	VIRTIO_BALLOON_F_MUST_TELL_HOST, VIRTIO_BALLOON_F_RPT_SWAP_IN,
+	VIRTIO_BALLOON_F_RPT_SWAP_OUT, VIRTIO_BALLOON_F_RPT_ANON,
+	VIRTIO_BALLOON_F_RPT_MAJFLT, VIRTIO_BALLOON_F_RPT_MINFLT,
+	VIRTIO_BALLOON_F_RPT_MEMFREE, VIRTIO_BALLOON_F_RPT_MEMTOT,
+};
 
 static struct virtio_driver virtio_balloon = {
 	.feature_table = features,
diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
index 09d7300..0bff4b8 100644
--- a/include/linux/virtio_balloon.h
+++ b/include/linux/virtio_balloon.h
@@ -6,15 +6,39 @@
 
 /* The feature bitmap for virtio balloon */
 #define VIRTIO_BALLOON_F_MUST_TELL_HOST	0 /* Tell before reclaiming pages */
+                                          /* Guest memory statistic reporting */
+#define VIRTIO_BALLOON_F_RPT_SWAP_IN  1   /* Number of pages swapped in */
+#define VIRTIO_BALLOON_F_RPT_SWAP_OUT 2   /* Number of pages swapped out */
+#define VIRTIO_BALLOON_F_RPT_ANON     3   /* Number of anonymous pages in use */
+#define VIRTIO_BALLOON_F_RPT_MAJFLT   4   /* Number of major faults */
+#define VIRTIO_BALLOON_F_RPT_MINFLT   5   /* Number of minor faults */
+#define VIRTIO_BALLOON_F_RPT_MEMFREE  6   /* Total amount of free memory */
+#define VIRTIO_BALLOON_F_RPT_MEMTOT   7   /* Total amount of memory */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12
 
+struct virtio_balloon_stats
+{
+	__le32 pswapin;      /* pages swapped in */
+	__le32 pswapout;     /* pages swapped out */
+	__le32 panon;        /* anonymous pages in use (in kb) */
+	__le32 pgmajfault;   /* Major page faults */
+	__le32 pgminfault;   /* Minor page faults */
+	__le32 memfree;      /* Total amount of free memory (in kb) */
+	__le32 memtot;       /* Total amount of memory (in kb) */
+};
+
 struct virtio_balloon_config
 {
 	/* Number of pages host wants Guest to give up. */
 	__le32 num_pages;
 	/* Number of pages we've actually got in balloon. */
 	__le32 actual;
+	/* Memory statistics */
+	struct virtio_balloon_stats stats;
 };
+
+#define VIRTIO_BALLOON_TIMEOUT (30 * HZ)
+
 #endif /* _LINUX_VIRTIO_BALLOON_H */

-- 
Thanks,
Adam

virtio: Add memory statistics reporting to the balloon driver

[Adam Litke]

When using ballooning to manage overcommitted memory on a host, a system for
guests to communicate their memory usage to the host can provide information
that will minimize the impact of ballooning on the guests.  The current method
employs a daemon running in each guest that communicates memory statistics to a
host daemon at a specified time interval.  The host daemon aggregates this
information and inflates and/or deflates balloons according to the level of
host memory pressure.  This approach is effective but overly complex since a
daemon must be installed inside each guest and coordinated to communicate with
the host.  A simpler approach is to collect memory statistics in the virtio
balloon driver and communicate them to the host via the device config space.

This patch enables the guest-side support by adding stats collection and
reporting to the virtio balloon driver.

Comments?

Signed-off-by: Adam Litke <agl@us.ibm.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Avi Kivity <avi@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 200c22f..0c9a9a1 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -180,6 +180,41 @@ static void update_balloon_size(struct virtio_balloon *vb)
 			      &actual, sizeof(actual));
 }
 
+static inline void update_stat(struct virtio_device *vdev, int feature,
+				unsigned long value, unsigned offset)
+{
+	__le32 __v = cpu_to_le32(value);
+	if (virtio_has_feature(vdev, feature))
+		vdev->config->set(vdev, offset, &__v, sizeof(__v));
+}
+
+#define K(x) ((x) << (PAGE_SHIFT - 10))
+static void update_balloon_stats(struct virtio_balloon *vb)
+{
+	unsigned long events[NR_VM_EVENT_ITEMS];
+	struct sysinfo i;
+	unsigned off = offsetof(struct virtio_balloon_config, stats);
+
+	all_vm_events(events);
+
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_SWAP_IN, events[PSWPIN],
+		    off + offsetof(struct virtio_balloon_stats, pswapin));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_SWAP_OUT, events[PSWPOUT],
+		    off + offsetof(struct virtio_balloon_stats, pswapout));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_MAJFLT, events[PGMAJFAULT],
+		    off + offsetof(struct virtio_balloon_stats, pgmajfault));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_MINFLT, events[PGFAULT],
+		    off + offsetof(struct virtio_balloon_stats, pgminfault));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_ANON,
+		    K(global_page_state(NR_ANON_PAGES)),
+		    off + offsetof(struct virtio_balloon_stats, panon));
+	si_meminfo(&i);
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_MEMFREE, K(i.freeram),
+		    off + offsetof(struct virtio_balloon_stats, memfree));
+	update_stat(vb->vdev, VIRTIO_BALLOON_F_RPT_MEMTOT, K(i.totalram),
+		    off + offsetof(struct virtio_balloon_stats, memtot));
+}
+
 static int balloon(void *_vballoon)
 {
 	struct virtio_balloon *vb = _vballoon;
@@ -189,15 +224,17 @@ static int balloon(void *_vballoon)
 		s64 diff;
 
 		try_to_freeze();
-		wait_event_interruptible(vb->config_change,
+		wait_event_interruptible_timeout(vb->config_change,
 					 (diff = towards_target(vb)) != 0
 					 || kthread_should_stop()
-					 || freezing(current));
+					 || freezing(current),
+					 VIRTIO_BALLOON_TIMEOUT);
 		if (diff > 0)
 			fill_balloon(vb, diff);
 		else if (diff < 0)
 			leak_balloon(vb, -diff);
 		update_balloon_size(vb);
+		update_balloon_stats(vb);
 	}
 	return 0;
 }
@@ -265,7 +302,12 @@ static void virtballoon_remove(struct virtio_device *vdev)
 	kfree(vb);
 }
 
-static unsigned int features[] = { VIRTIO_BALLOON_F_MUST_TELL_HOST };
+static unsigned int features[] = {
+	VIRTIO_BALLOON_F_MUST_TELL_HOST, VIRTIO_BALLOON_F_RPT_SWAP_IN,
+	VIRTIO_BALLOON_F_RPT_SWAP_OUT, VIRTIO_BALLOON_F_RPT_ANON,
+	VIRTIO_BALLOON_F_RPT_MAJFLT, VIRTIO_BALLOON_F_RPT_MINFLT,
+	VIRTIO_BALLOON_F_RPT_MEMFREE, VIRTIO_BALLOON_F_RPT_MEMTOT,
+};
 
 static struct virtio_driver virtio_balloon = {
 	.feature_table = features,
diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
index 09d7300..0bff4b8 100644
--- a/include/linux/virtio_balloon.h
+++ b/include/linux/virtio_balloon.h
@@ -6,15 +6,39 @@
 
 /* The feature bitmap for virtio balloon */
 #define VIRTIO_BALLOON_F_MUST_TELL_HOST	0 /* Tell before reclaiming pages */
+                                          /* Guest memory statistic reporting */
+#define VIRTIO_BALLOON_F_RPT_SWAP_IN  1   /* Number of pages swapped in */
+#define VIRTIO_BALLOON_F_RPT_SWAP_OUT 2   /* Number of pages swapped out */
+#define VIRTIO_BALLOON_F_RPT_ANON     3   /* Number of anonymous pages in use */
+#define VIRTIO_BALLOON_F_RPT_MAJFLT   4   /* Number of major faults */
+#define VIRTIO_BALLOON_F_RPT_MINFLT   5   /* Number of minor faults */
+#define VIRTIO_BALLOON_F_RPT_MEMFREE  6   /* Total amount of free memory */
+#define VIRTIO_BALLOON_F_RPT_MEMTOT   7   /* Total amount of memory */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12
 
+struct virtio_balloon_stats
+{
+	__le32 pswapin;      /* pages swapped in */
+	__le32 pswapout;     /* pages swapped out */
+	__le32 panon;        /* anonymous pages in use (in kb) */
+	__le32 pgmajfault;   /* Major page faults */
+	__le32 pgminfault;   /* Minor page faults */
+	__le32 memfree;      /* Total amount of free memory (in kb) */
+	__le32 memtot;       /* Total amount of memory (in kb) */
+};
+
 struct virtio_balloon_config
 {
 	/* Number of pages host wants Guest to give up. */
 	__le32 num_pages;
 	/* Number of pages we've actually got in balloon. */
 	__le32 actual;
+	/* Memory statistics */
+	struct virtio_balloon_stats stats;
 };
+
+#define VIRTIO_BALLOON_TIMEOUT (30 * HZ)
+
 #endif /* _LINUX_VIRTIO_BALLOON_H */

-- 
Thanks,
Adam

Re: [Qemu-devel] [RFC] virtio: Report new guest memory statistics pertinent to memory ballooning (V2)

[Jamie Lokier]

Adam Litke wrote:
> +        s->stats.pswapin = has_feature(dev, VIRTIO_BALLOON_F_RPT_SWAP_OUT) ?
> +                                       dev->stats.pswapin : -1;

(etc.)

Why not simply have the guest fill in the unused fields with -1, and
say that's how "no meaningful value" is represented in the ABI?

All guests have to know about all those fields anyway, for the
structure layout.  Is there any benefit to specifying feature bits in
advance over simply storing -1 there?

-- Jamie

Re: [Qemu-devel] [RFC] virtio: Report new guest memory statistics pertinent to memory ballooning (V2)

[Jamie Lokier]

Adam Litke wrote:
> +static inline void print_stat(Monitor *mon, uint32_t val, const char *label)
> +{
> +    if (val != -1) {
> +        monitor_printf(mon, ",%s=%u", label, val);
> +    }
> +}
> +
>  static void do_info_balloon(Monitor *mon)
>  {
> -    ram_addr_t actual;
> +    QEMUBalloonState s;
> +    int ret;
>  
> -    actual = qemu_balloon_status();
> -    if (kvm_enabled() && !kvm_has_sync_mmu())
> +    ret = qemu_balloon_status(&s);
> +    if (kvm_enabled() && !kvm_has_sync_mmu()) {
>          monitor_printf(mon, "Using KVM without synchronous MMU, "
>                         "ballooning disabled\n");
> -    else if (actual == 0)
> +    } else if (ret < 0) {
>          monitor_printf(mon, "Ballooning not activated in VM\n");
> -    else
> -        monitor_printf(mon, "balloon: actual=%d\n", (int)(actual >> 20));
> +    } else {
> +        monitor_printf(mon, "balloon: actual=%d", (int)(s.actual >> 20));
> +        print_stat(mon, s.stats.pswapin, "pages_swapped_in");
> +        print_stat(mon, s.stats.pswapout, "pages_swapped_out");
> +        print_stat(mon, s.stats.panon, "anon_pages");
> +        print_stat(mon, s.stats.pgmajfault, "major_page_faults");
> +        print_stat(mon, s.stats.pgminfault, "minor_page_faults");
> +        print_stat(mon, s.stats.memfree, "free_memory");
> +        print_stat(mon, s.stats.memtot, "total_memory");
> +        monitor_printf(mon, "\n");
> +    }
> +        
>  }

These days, would it make more sense to emit a QJSON object?

In this case the JSON object is quite human readable too.

-- Jamie

Re: [Qemu-devel] [RFC] virtio: Report new guest memory statistics pertinent to memory ballooning (V2)

[Adam Litke]

On Mon, 2009-11-09 at 19:00 +0000, Jamie Lokier wrote:
> Adam Litke wrote:
> > +        s->stats.pswapin = has_feature(dev, VIRTIO_BALLOON_F_RPT_SWAP_OUT) ?
> > +                                       dev->stats.pswapin : -1;
> 
> (etc.)
> 
> Why not simply have the guest fill in the unused fields with -1, and
> say that's how "no meaningful value" is represented in the ABI?
> 
> All guests have to know about all those fields anyway, for the
> structure layout.  Is there any benefit to specifying feature bits in
> advance over simply storing -1 there?

This holds true for fields that exist but might not be supported by a
guest, but what about the case where we add a new field?  In that case,
qemu and the guest may disagree on the structure layout.

-- 
Thanks,
Adam

Re: [Qemu-devel] [RFC] virtio: Report new guest memory statistics pertinent to memory ballooning (V2)

[Adam Litke]

On Mon, 2009-11-09 at 19:01 +0000, Jamie Lokier wrote:
> These days, would it make more sense to emit a QJSON object?
> 
> In this case the JSON object is quite human readable too.

I'm not very particular on the output format since it's main consumer is
likely another program.  Is XML output via the monitor a new precedent
in qemu?  I was unable to find any other monitor command that emits
XML. 

-- 
Thanks,
Adam

Re: [Qemu-devel] [RFC] virtio: Report new guest memory statistics pertinent to memory ballooning (V2)

[Anthony Liguori]

Jamie Lokier wrote:
> Adam Litke wrote:
>   
>> +        s->stats.pswapin = has_feature(dev, VIRTIO_BALLOON_F_RPT_SWAP_OUT) ?
>> +                                       dev->stats.pswapin : -1;
>>     
>
> (etc.)
>
> Why not simply have the guest fill in the unused fields with -1, and
> say that's how "no meaningful value" is represented in the ABI?
>
> All guests have to know about all those fields anyway, for the
> structure layout.  Is there any benefit to specifying feature bits in
> advance over simply storing -1 there?
>   

Features are negotiated.  It lets a host advertise the support of a 
feature and it lets the guest acknowledge it's support of a feature.

Most importantly, why invent a new mechanism when we already have one?

-- 
Regards,

Anthony Liguori

Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
> A simpler approach is to collect memory statistics in the virtio
> balloon driver and communicate them to the host via the device config space.

There are two issues I see with this.  First, there's an atomicity problem
since you can't tell when the stats are consistent.  Second, polling is
ugly.

A stats vq might solve this more cleanly?
Rusty.

[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
> A simpler approach is to collect memory statistics in the virtio
> balloon driver and communicate them to the host via the device config space.

There are two issues I see with this.  First, there's an atomicity problem
since you can't tell when the stats are consistent.  Second, polling is
ugly.

A stats vq might solve this more cleanly?
Rusty.

Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
> A simpler approach is to collect memory statistics in the virtio
> balloon driver and communicate them to the host via the device config space.

There are two issues I see with this.  First, there's an atomicity problem
since you can't tell when the stats are consistent.  Second, polling is
ugly.

A stats vq might solve this more cleanly?
Rusty.

[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
> A simpler approach is to collect memory statistics in the virtio
> balloon driver and communicate them to the host via the device config space.

There are two issues I see with this.  First, there's an atomicity problem
since you can't tell when the stats are consistent.  Second, polling is
ugly.

A stats vq might solve this more cleanly?
Rusty.

[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
> A simpler approach is to collect memory statistics in the virtio
> balloon driver and communicate them to the host via the device config space.

There are two issues I see with this.  First, there's an atomicity problem
since you can't tell when the stats are consistent.  Second, polling is
ugly.

A stats vq might solve this more cleanly?
Rusty.

Re: [Qemu-devel] [RFC] virtio: Report new guest memory statistics pertinent to memory ballooning (V2)

[Jamie Lokier]

Anthony Liguori wrote:
> Jamie Lokier wrote:
> >Adam Litke wrote:
> >  
> >>+        s->stats.pswapin = has_feature(dev, 
> >>VIRTIO_BALLOON_F_RPT_SWAP_OUT) ?
> >>+                                       dev->stats.pswapin : -1;
> >>    
> >
> >(etc.)
> >
> >Why not simply have the guest fill in the unused fields with -1, and
> >say that's how "no meaningful value" is represented in the ABI?
> >
> >All guests have to know about all those fields anyway, for the
> >structure layout.  Is there any benefit to specifying feature bits in
> >advance over simply storing -1 there?
> >  
> 
> Features are negotiated.  It lets a host advertise the support of a 
> feature and it lets the guest acknowledge it's support of a feature.
> 
> Most importantly, why invent a new mechanism when we already have one?

In this case, the guest already has to have those fields in the
structure.  It's not like something where the guest doesn't know about
a feature at all, and then a later driver adds new capabilities.  That
would require feature bits or an ABI version, I agree.

But the real motivation for my comment was seeing the bulk of the
patch being definitions and tests for lots of feature bits which do
something trivial that the guest can easily do.

The feature bits don't seem to simplify anything in this case.  As for
preparing to be consistent with future additions, presumably if
another 20 fields are added, there won't be sufficient bits in the
feature word anyway, so an "extended feature bits" feature will be
needed.

-- Jamie

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

Rusty Russell wrote:
> On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
>   
>> A simpler approach is to collect memory statistics in the virtio
>> balloon driver and communicate them to the host via the device config space.
>>     
>
> There are two issues I see with this.  First, there's an atomicity problem
> since you can't tell when the stats are consistent.

Actually, config writes always require notification from the guest to 
the host.  This means the host knows when they config space is changed 
so atomicity isn't a problem.

In fact, if it were a problem, then the balloon driver would be 
fundamentally broken because target and actual are stored in the config 
space.

If you recall, we had this discussion originally wrt the balloon driver :-)

>   Second, polling is
> ugly.
>   

As opposed to?  The guest could set a timer and update the values 
periodically but that's even uglier because then the host cannot 
determine the update granularity.

> A stats vq might solve this more cleanly?
>   

actual and target are both really just stats.  Had we implemented those 
with a vq, I'd be inclined to agree with you but since they're 
implemented in the config space, it seems natural to extend the config 
space with other stats.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

Rusty Russell wrote:
> On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
>   
>> A simpler approach is to collect memory statistics in the virtio
>> balloon driver and communicate them to the host via the device config space.
>>     
>
> There are two issues I see with this.  First, there's an atomicity problem
> since you can't tell when the stats are consistent.

Actually, config writes always require notification from the guest to 
the host.  This means the host knows when they config space is changed 
so atomicity isn't a problem.

In fact, if it were a problem, then the balloon driver would be 
fundamentally broken because target and actual are stored in the config 
space.

If you recall, we had this discussion originally wrt the balloon driver :-)

>   Second, polling is
> ugly.
>   

As opposed to?  The guest could set a timer and update the values 
periodically but that's even uglier because then the host cannot 
determine the update granularity.

> A stats vq might solve this more cleanly?
>   

actual and target are both really just stats.  Had we implemented those 
with a vq, I'd be inclined to agree with you but since they're 
implemented in the config space, it seems natural to extend the config 
space with other stats.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

Rusty Russell wrote:
> On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
>   
>> A simpler approach is to collect memory statistics in the virtio
>> balloon driver and communicate them to the host via the device config space.
>>     
>
> There are two issues I see with this.  First, there's an atomicity problem
> since you can't tell when the stats are consistent.

Actually, config writes always require notification from the guest to 
the host.  This means the host knows when they config space is changed 
so atomicity isn't a problem.

In fact, if it were a problem, then the balloon driver would be 
fundamentally broken because target and actual are stored in the config 
space.

If you recall, we had this discussion originally wrt the balloon driver :-)

>   Second, polling is
> ugly.
>   

As opposed to?  The guest could set a timer and update the values 
periodically but that's even uglier because then the host cannot 
determine the update granularity.

> A stats vq might solve this more cleanly?
>   

actual and target are both really just stats.  Had we implemented those 
with a vq, I'd be inclined to agree with you but since they're 
implemented in the config space, it seems natural to extend the config 
space with other stats.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Avi Kivity]

On 11/10/2009 04:36 PM, Anthony Liguori wrote:
>
>> A stats vq might solve this more cleanly?
>
> actual and target are both really just stats.  Had we implemented 
> those with a vq, I'd be inclined to agree with you but since they're 
> implemented in the config space, it seems natural to extend the config 
> space with other stats.
>

There is in fact a difference; actual and target are very rarely 
updated, while the stats are updated very often.  Using a vq means a 
constant number of exits per batch instead of one exit per statistic.  
If the vq is host-driven, it also allows the host to control the update 
frequency dynamically (i.e. stop polling when there is no memory pressure).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Avi Kivity]

On 11/10/2009 04:36 PM, Anthony Liguori wrote:
>
>> A stats vq might solve this more cleanly?
>
> actual and target are both really just stats.  Had we implemented 
> those with a vq, I'd be inclined to agree with you but since they're 
> implemented in the config space, it seems natural to extend the config 
> space with other stats.
>

There is in fact a difference; actual and target are very rarely 
updated, while the stats are updated very often.  Using a vq means a 
constant number of exits per batch instead of one exit per statistic.  
If the vq is host-driven, it also allows the host to control the update 
frequency dynamically (i.e. stop polling when there is no memory pressure).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Avi Kivity]

On 11/10/2009 04:36 PM, Anthony Liguori wrote:
>
>> A stats vq might solve this more cleanly?
>
> actual and target are both really just stats.  Had we implemented 
> those with a vq, I'd be inclined to agree with you but since they're 
> implemented in the config space, it seems natural to extend the config 
> space with other stats.
>

There is in fact a difference; actual and target are very rarely 
updated, while the stats are updated very often.  Using a vq means a 
constant number of exits per batch instead of one exit per statistic.  
If the vq is host-driven, it also allows the host to control the update 
frequency dynamically (i.e. stop polling when there is no memory pressure).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

Avi Kivity wrote:
> On 11/10/2009 04:36 PM, Anthony Liguori wrote:
>>
>>> A stats vq might solve this more cleanly?
>>
>> actual and target are both really just stats.  Had we implemented 
>> those with a vq, I'd be inclined to agree with you but since they're 
>> implemented in the config space, it seems natural to extend the 
>> config space with other stats.
>>
>
> There is in fact a difference; actual and target are very rarely 
> updated, while the stats are updated very often.  Using a vq means a 
> constant number of exits per batch instead of one exit per statistic.  
> If the vq is host-driven, it also allows the host to control the 
> update frequency dynamically (i.e. stop polling when there is no 
> memory pressure).

I'm not terribly opposed to using a vq for this.  I would expect the 
stat update interval to be rather long (10s probably) but a vq works 
just as well.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

Avi Kivity wrote:
> On 11/10/2009 04:36 PM, Anthony Liguori wrote:
>>
>>> A stats vq might solve this more cleanly?
>>
>> actual and target are both really just stats.  Had we implemented 
>> those with a vq, I'd be inclined to agree with you but since they're 
>> implemented in the config space, it seems natural to extend the 
>> config space with other stats.
>>
>
> There is in fact a difference; actual and target are very rarely 
> updated, while the stats are updated very often.  Using a vq means a 
> constant number of exits per batch instead of one exit per statistic.  
> If the vq is host-driven, it also allows the host to control the 
> update frequency dynamically (i.e. stop polling when there is no 
> memory pressure).

I'm not terribly opposed to using a vq for this.  I would expect the 
stat update interval to be rather long (10s probably) but a vq works 
just as well.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

Avi Kivity wrote:
> On 11/10/2009 04:36 PM, Anthony Liguori wrote:
>>
>>> A stats vq might solve this more cleanly?
>>
>> actual and target are both really just stats.  Had we implemented 
>> those with a vq, I'd be inclined to agree with you but since they're 
>> implemented in the config space, it seems natural to extend the 
>> config space with other stats.
>>
>
> There is in fact a difference; actual and target are very rarely 
> updated, while the stats are updated very often.  Using a vq means a 
> constant number of exits per batch instead of one exit per statistic.  
> If the vq is host-driven, it also allows the host to control the 
> update frequency dynamically (i.e. stop polling when there is no 
> memory pressure).

I'm not terribly opposed to using a vq for this.  I would expect the 
stat update interval to be rather long (10s probably) but a vq works 
just as well.

-- 
Regards,

Anthony Liguori

Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

Rusty Russell wrote:
> On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
>   
>> A simpler approach is to collect memory statistics in the virtio
>> balloon driver and communicate them to the host via the device config space.
>>     
>
> There are two issues I see with this.  First, there's an atomicity problem
> since you can't tell when the stats are consistent.  Second, polling is
> ugly.
>
> A stats vq might solve this more cleanly?
>   

This turns out to not work so nicely.  You really need bidirectional 
communication.  You need to request that stats be collected and then you 
need to tell the hypervisor about the stats that were collected.  You 
don't need any real correlation between requests and stat reports either.

This really models how target/actual work and I think it suggests that 
we want to reuse that mechanism for the stats too.

> Rusty.
>   


-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

Rusty Russell wrote:
> On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
>   
>> A simpler approach is to collect memory statistics in the virtio
>> balloon driver and communicate them to the host via the device config space.
>>     
>
> There are two issues I see with this.  First, there's an atomicity problem
> since you can't tell when the stats are consistent.  Second, polling is
> ugly.
>
> A stats vq might solve this more cleanly?
>   

This turns out to not work so nicely.  You really need bidirectional 
communication.  You need to request that stats be collected and then you 
need to tell the hypervisor about the stats that were collected.  You 
don't need any real correlation between requests and stat reports either.

This really models how target/actual work and I think it suggests that 
we want to reuse that mechanism for the stats too.

> Rusty.
>   


-- 
Regards,

Anthony Liguori

Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

Rusty Russell wrote:
> On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
>   
>> A simpler approach is to collect memory statistics in the virtio
>> balloon driver and communicate them to the host via the device config space.
>>     
>
> There are two issues I see with this.  First, there's an atomicity problem
> since you can't tell when the stats are consistent.  Second, polling is
> ugly.
>
> A stats vq might solve this more cleanly?
>   

This turns out to not work so nicely.  You really need bidirectional 
communication.  You need to request that stats be collected and then you 
need to tell the hypervisor about the stats that were collected.  You 
don't need any real correlation between requests and stat reports either.

This really models how target/actual work and I think it suggests that 
we want to reuse that mechanism for the stats too.

> Rusty.
>   


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Wed, 11 Nov 2009 01:06:14 am Anthony Liguori wrote:
> Rusty Russell wrote:
> > On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
> >   
> >> A simpler approach is to collect memory statistics in the virtio
> >> balloon driver and communicate them to the host via the device config space.
> >>     
> >
> > There are two issues I see with this.  First, there's an atomicity problem
> > since you can't tell when the stats are consistent.
> 
> Actually, config writes always require notification from the guest to 
> the host.  This means the host knows when they config space is changed 
> so atomicity isn't a problem.

I think you missed my point: the stats are inter-related, so they should be
served together.

> In fact, if it were a problem, then the balloon driver would be 
> fundamentally broken because target and actual are stored in the config 
> space.

No, one is written by the host, the other the guest.  Still works.

> If you recall, we had this discussion originally wrt the balloon driver :-)

And I never did get around to the lguest implementation, which would have
seen if this really is an issue.

> >   Second, polling is ugly.
> 
> As opposed to?

As opposed to giving the stats whenever asked by the host.

> > A stats vq might solve this more cleanly?
> >   
> 
> actual and target are both really just stats.  Had we implemented those 
> with a vq, I'd be inclined to agree with you but since they're 
> implemented in the config space, it seems natural to extend the config 
> space with other stats.

It does, *if* we don't need accuracy.  Otherwise, it seems like we need
something else.

Cheers,
Rusty.

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Wed, 11 Nov 2009 01:06:14 am Anthony Liguori wrote:
> Rusty Russell wrote:
> > On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
> >   
> >> A simpler approach is to collect memory statistics in the virtio
> >> balloon driver and communicate them to the host via the device config space.
> >>     
> >
> > There are two issues I see with this.  First, there's an atomicity problem
> > since you can't tell when the stats are consistent.
> 
> Actually, config writes always require notification from the guest to 
> the host.  This means the host knows when they config space is changed 
> so atomicity isn't a problem.

I think you missed my point: the stats are inter-related, so they should be
served together.

> In fact, if it were a problem, then the balloon driver would be 
> fundamentally broken because target and actual are stored in the config 
> space.

No, one is written by the host, the other the guest.  Still works.

> If you recall, we had this discussion originally wrt the balloon driver :-)

And I never did get around to the lguest implementation, which would have
seen if this really is an issue.

> >   Second, polling is ugly.
> 
> As opposed to?

As opposed to giving the stats whenever asked by the host.

> > A stats vq might solve this more cleanly?
> >   
> 
> actual and target are both really just stats.  Had we implemented those 
> with a vq, I'd be inclined to agree with you but since they're 
> implemented in the config space, it seems natural to extend the config 
> space with other stats.

It does, *if* we don't need accuracy.  Otherwise, it seems like we need
something else.

Cheers,
Rusty.

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Wed, 11 Nov 2009 01:06:14 am Anthony Liguori wrote:
> Rusty Russell wrote:
> > On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
> >   
> >> A simpler approach is to collect memory statistics in the virtio
> >> balloon driver and communicate them to the host via the device config space.
> >>     
> >
> > There are two issues I see with this.  First, there's an atomicity problem
> > since you can't tell when the stats are consistent.
> 
> Actually, config writes always require notification from the guest to 
> the host.  This means the host knows when they config space is changed 
> so atomicity isn't a problem.

I think you missed my point: the stats are inter-related, so they should be
served together.

> In fact, if it were a problem, then the balloon driver would be 
> fundamentally broken because target and actual are stored in the config 
> space.

No, one is written by the host, the other the guest.  Still works.

> If you recall, we had this discussion originally wrt the balloon driver :-)

And I never did get around to the lguest implementation, which would have
seen if this really is an issue.

> >   Second, polling is ugly.
> 
> As opposed to?

As opposed to giving the stats whenever asked by the host.

> > A stats vq might solve this more cleanly?
> >   
> 
> actual and target are both really just stats.  Had we implemented those 
> with a vq, I'd be inclined to agree with you but since they're 
> implemented in the config space, it seems natural to extend the config 
> space with other stats.

It does, *if* we don't need accuracy.  Otherwise, it seems like we need
something else.

Cheers,
Rusty.

Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Wed, 11 Nov 2009 08:22:42 am Anthony Liguori wrote:
> Rusty Russell wrote:
> > On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
> >   
> >> A simpler approach is to collect memory statistics in the virtio
> >> balloon driver and communicate them to the host via the device config space.
> >>     
> >
> > There are two issues I see with this.  First, there's an atomicity problem
> > since you can't tell when the stats are consistent.  Second, polling is
> > ugly.
> >
> > A stats vq might solve this more cleanly?
> >   
> 
> This turns out to not work so nicely.  You really need bidirectional 
> communication.  You need to request that stats be collected and then you 
> need to tell the hypervisor about the stats that were collected.  You 
> don't need any real correlation between requests and stat reports either.

You register an outbuf at initialization time.  The host hands it back when
it wants you to refill it with stats.

> This really models how target/actual work and I think it suggests that 
> we want to reuse that mechanism for the stats too.

Sure, I want to.  You want to.  It's simple.

But the universe is remarkably indifferent to what we want.  Is it actually
sufficient or are we going to regret our laziness?

Cheers,
Rusty.

[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Wed, 11 Nov 2009 08:22:42 am Anthony Liguori wrote:
> Rusty Russell wrote:
> > On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
> >   
> >> A simpler approach is to collect memory statistics in the virtio
> >> balloon driver and communicate them to the host via the device config space.
> >>     
> >
> > There are two issues I see with this.  First, there's an atomicity problem
> > since you can't tell when the stats are consistent.  Second, polling is
> > ugly.
> >
> > A stats vq might solve this more cleanly?
> >   
> 
> This turns out to not work so nicely.  You really need bidirectional 
> communication.  You need to request that stats be collected and then you 
> need to tell the hypervisor about the stats that were collected.  You 
> don't need any real correlation between requests and stat reports either.

You register an outbuf at initialization time.  The host hands it back when
it wants you to refill it with stats.

> This really models how target/actual work and I think it suggests that 
> we want to reuse that mechanism for the stats too.

Sure, I want to.  You want to.  It's simple.

But the universe is remarkably indifferent to what we want.  Is it actually
sufficient or are we going to regret our laziness?

Cheers,
Rusty.

Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Wed, 11 Nov 2009 08:22:42 am Anthony Liguori wrote:
> Rusty Russell wrote:
> > On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
> >   
> >> A simpler approach is to collect memory statistics in the virtio
> >> balloon driver and communicate them to the host via the device config space.
> >>     
> >
> > There are two issues I see with this.  First, there's an atomicity problem
> > since you can't tell when the stats are consistent.  Second, polling is
> > ugly.
> >
> > A stats vq might solve this more cleanly?
> >   
> 
> This turns out to not work so nicely.  You really need bidirectional 
> communication.  You need to request that stats be collected and then you 
> need to tell the hypervisor about the stats that were collected.  You 
> don't need any real correlation between requests and stat reports either.

You register an outbuf at initialization time.  The host hands it back when
it wants you to refill it with stats.

> This really models how target/actual work and I think it suggests that 
> we want to reuse that mechanism for the stats too.

Sure, I want to.  You want to.  It's simple.

But the universe is remarkably indifferent to what we want.  Is it actually
sufficient or are we going to regret our laziness?

Cheers,
Rusty.

Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

Rusty Russell wrote:
> On Wed, 11 Nov 2009 08:22:42 am Anthony Liguori wrote:
>   
>> Rusty Russell wrote:
>>     
>>> On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
>>>   
>>>       
>>>> A simpler approach is to collect memory statistics in the virtio
>>>> balloon driver and communicate them to the host via the device config space.
>>>>     
>>>>         
>>> There are two issues I see with this.  First, there's an atomicity problem
>>> since you can't tell when the stats are consistent.  Second, polling is
>>> ugly.
>>>
>>> A stats vq might solve this more cleanly?
>>>   
>>>       
>> This turns out to not work so nicely.  You really need bidirectional 
>> communication.  You need to request that stats be collected and then you 
>> need to tell the hypervisor about the stats that were collected.  You 
>> don't need any real correlation between requests and stat reports either.
>>     
>
> You register an outbuf at initialization time.  The host hands it back when
> it wants you to refill it with stats.
>   

That's strangely backwards.  Guest send a stat buffer that's filled out, 
host acks it when it wants another.  That doesn't seem bizarre to you?

>> This really models how target/actual work and I think it suggests that 
>> we want to reuse that mechanism for the stats too.
>>     
>
> Sure, I want to.  You want to.  It's simple.
>
> But the universe is remarkably indifferent to what we want.  Is it actually
> sufficient or are we going to regret our laziness?
>   

It's not laziness, it's consistency.  How is actual different than free 
memory or any other stat?

> Cheers,
> Rusty.
>   


-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

Rusty Russell wrote:
> On Wed, 11 Nov 2009 08:22:42 am Anthony Liguori wrote:
>   
>> Rusty Russell wrote:
>>     
>>> On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
>>>   
>>>       
>>>> A simpler approach is to collect memory statistics in the virtio
>>>> balloon driver and communicate them to the host via the device config space.
>>>>     
>>>>         
>>> There are two issues I see with this.  First, there's an atomicity problem
>>> since you can't tell when the stats are consistent.  Second, polling is
>>> ugly.
>>>
>>> A stats vq might solve this more cleanly?
>>>   
>>>       
>> This turns out to not work so nicely.  You really need bidirectional 
>> communication.  You need to request that stats be collected and then you 
>> need to tell the hypervisor about the stats that were collected.  You 
>> don't need any real correlation between requests and stat reports either.
>>     
>
> You register an outbuf at initialization time.  The host hands it back when
> it wants you to refill it with stats.
>   

That's strangely backwards.  Guest send a stat buffer that's filled out, 
host acks it when it wants another.  That doesn't seem bizarre to you?

>> This really models how target/actual work and I think it suggests that 
>> we want to reuse that mechanism for the stats too.
>>     
>
> Sure, I want to.  You want to.  It's simple.
>
> But the universe is remarkably indifferent to what we want.  Is it actually
> sufficient or are we going to regret our laziness?
>   

It's not laziness, it's consistency.  How is actual different than free 
memory or any other stat?

> Cheers,
> Rusty.
>   


-- 
Regards,

Anthony Liguori

Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

Rusty Russell wrote:
> On Wed, 11 Nov 2009 08:22:42 am Anthony Liguori wrote:
>   
>> Rusty Russell wrote:
>>     
>>> On Tue, 10 Nov 2009 03:02:06 am Adam Litke wrote:
>>>   
>>>       
>>>> A simpler approach is to collect memory statistics in the virtio
>>>> balloon driver and communicate them to the host via the device config space.
>>>>     
>>>>         
>>> There are two issues I see with this.  First, there's an atomicity problem
>>> since you can't tell when the stats are consistent.  Second, polling is
>>> ugly.
>>>
>>> A stats vq might solve this more cleanly?
>>>   
>>>       
>> This turns out to not work so nicely.  You really need bidirectional 
>> communication.  You need to request that stats be collected and then you 
>> need to tell the hypervisor about the stats that were collected.  You 
>> don't need any real correlation between requests and stat reports either.
>>     
>
> You register an outbuf at initialization time.  The host hands it back when
> it wants you to refill it with stats.
>   

That's strangely backwards.  Guest send a stat buffer that's filled out, 
host acks it when it wants another.  That doesn't seem bizarre to you?

>> This really models how target/actual work and I think it suggests that 
>> we want to reuse that mechanism for the stats too.
>>     
>
> Sure, I want to.  You want to.  It's simple.
>
> But the universe is remarkably indifferent to what we want.  Is it actually
> sufficient or are we going to regret our laziness?
>   

It's not laziness, it's consistency.  How is actual different than free 
memory or any other stat?

> Cheers,
> Rusty.
>   


-- 
Regards,

Anthony Liguori

Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Wed, 11 Nov 2009 10:37:56 am Anthony Liguori wrote:
> Rusty Russell wrote:
> > You register an outbuf at initialization time.  The host hands it back when
> > it wants you to refill it with stats.
> 
> That's strangely backwards.  Guest send a stat buffer that's filled out, 
> host acks it when it wants another.  That doesn't seem bizarre to you?

Yep!  But that's a limitation of our brains, not the infrastructure ;)

Think of the stats as an infinite stream of data.  Read from it at your
leisure.  This is how, for example, console output works.

> > But the universe is remarkably indifferent to what we want.  Is it actually
> > sufficient or are we going to regret our laziness?
> 
> It's not laziness, it's consistency.  How is actual different than free 
> memory or any other stat?

Because it's a COLLECTION of stats.  For example, swap in should be < swap
out.  Now, the current Linux implementation of all_vm_events() is non-atomic
anyway, so maybe we can just document this as best-effort.  I'm saying that
if it *is* a problem, I think we need a vq.

But it raises the question: what stats are generally useful cross-OS?  Should
we be supplying numbers like "unused" (free) "instantly discardable" (ie.
clean), "discardable to disk" (ie. file-backed), "discardable to swap"
(ie. swap-backed) and "unswappable" instead?

(I just made those up, of course, but it seems like that would give a fair
indication of real memory pressure in any OS).

Thanks,
Rusty.

[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Wed, 11 Nov 2009 10:37:56 am Anthony Liguori wrote:
> Rusty Russell wrote:
> > You register an outbuf at initialization time.  The host hands it back when
> > it wants you to refill it with stats.
> 
> That's strangely backwards.  Guest send a stat buffer that's filled out, 
> host acks it when it wants another.  That doesn't seem bizarre to you?

Yep!  But that's a limitation of our brains, not the infrastructure ;)

Think of the stats as an infinite stream of data.  Read from it at your
leisure.  This is how, for example, console output works.

> > But the universe is remarkably indifferent to what we want.  Is it actually
> > sufficient or are we going to regret our laziness?
> 
> It's not laziness, it's consistency.  How is actual different than free 
> memory or any other stat?

Because it's a COLLECTION of stats.  For example, swap in should be < swap
out.  Now, the current Linux implementation of all_vm_events() is non-atomic
anyway, so maybe we can just document this as best-effort.  I'm saying that
if it *is* a problem, I think we need a vq.

But it raises the question: what stats are generally useful cross-OS?  Should
we be supplying numbers like "unused" (free) "instantly discardable" (ie.
clean), "discardable to disk" (ie. file-backed), "discardable to swap"
(ie. swap-backed) and "unswappable" instead?

(I just made those up, of course, but it seems like that would give a fair
indication of real memory pressure in any OS).

Thanks,
Rusty.

Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Wed, 11 Nov 2009 10:37:56 am Anthony Liguori wrote:
> Rusty Russell wrote:
> > You register an outbuf at initialization time.  The host hands it back when
> > it wants you to refill it with stats.
> 
> That's strangely backwards.  Guest send a stat buffer that's filled out, 
> host acks it when it wants another.  That doesn't seem bizarre to you?

Yep!  But that's a limitation of our brains, not the infrastructure ;)

Think of the stats as an infinite stream of data.  Read from it at your
leisure.  This is how, for example, console output works.

> > But the universe is remarkably indifferent to what we want.  Is it actually
> > sufficient or are we going to regret our laziness?
> 
> It's not laziness, it's consistency.  How is actual different than free 
> memory or any other stat?

Because it's a COLLECTION of stats.  For example, swap in should be < swap
out.  Now, the current Linux implementation of all_vm_events() is non-atomic
anyway, so maybe we can just document this as best-effort.  I'm saying that
if it *is* a problem, I think we need a vq.

But it raises the question: what stats are generally useful cross-OS?  Should
we be supplying numbers like "unused" (free) "instantly discardable" (ie.
clean), "discardable to disk" (ie. file-backed), "discardable to swap"
(ie. swap-backed) and "unswappable" instead?

(I just made those up, of course, but it seems like that would give a fair
indication of real memory pressure in any OS).

Thanks,
Rusty.

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Jamie Lokier]

Anthony Liguori wrote:
> Avi Kivity wrote:
> >On 11/10/2009 04:36 PM, Anthony Liguori wrote:
> >>
> >>>A stats vq might solve this more cleanly?
> >>
> >>actual and target are both really just stats.  Had we implemented 
> >>those with a vq, I'd be inclined to agree with you but since they're 
> >>implemented in the config space, it seems natural to extend the 
> >>config space with other stats.
> >>
> >
> >There is in fact a difference; actual and target are very rarely 
> >updated, while the stats are updated very often.  Using a vq means a 
> >constant number of exits per batch instead of one exit per statistic.  
> >If the vq is host-driven, it also allows the host to control the 
> >update frequency dynamically (i.e. stop polling when there is no 
> >memory pressure).
> 
> I'm not terribly opposed to using a vq for this.  I would expect the 
> stat update interval to be rather long (10s probably) but a vq works 
> just as well.

If there's no memory pressure and no guest activity, you probably want
the stat update to be as rare as possible to avoid wakeups.  Save
power on laptops, that sort of thing.

If there's a host user interested in the state ("qemutop?"), you may
want updates more often than 10s.

-- Jamie

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Jamie Lokier]

Anthony Liguori wrote:
> Avi Kivity wrote:
> >On 11/10/2009 04:36 PM, Anthony Liguori wrote:
> >>
> >>>A stats vq might solve this more cleanly?
> >>
> >>actual and target are both really just stats.  Had we implemented 
> >>those with a vq, I'd be inclined to agree with you but since they're 
> >>implemented in the config space, it seems natural to extend the 
> >>config space with other stats.
> >>
> >
> >There is in fact a difference; actual and target are very rarely 
> >updated, while the stats are updated very often.  Using a vq means a 
> >constant number of exits per batch instead of one exit per statistic.  
> >If the vq is host-driven, it also allows the host to control the 
> >update frequency dynamically (i.e. stop polling when there is no 
> >memory pressure).
> 
> I'm not terribly opposed to using a vq for this.  I would expect the 
> stat update interval to be rather long (10s probably) but a vq works 
> just as well.

If there's no memory pressure and no guest activity, you probably want
the stat update to be as rare as possible to avoid wakeups.  Save
power on laptops, that sort of thing.

If there's a host user interested in the state ("qemutop?"), you may
want updates more often than 10s.

-- Jamie

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Jamie Lokier]

Anthony Liguori wrote:
> Avi Kivity wrote:
> >On 11/10/2009 04:36 PM, Anthony Liguori wrote:
> >>
> >>>A stats vq might solve this more cleanly?
> >>
> >>actual and target are both really just stats.  Had we implemented 
> >>those with a vq, I'd be inclined to agree with you but since they're 
> >>implemented in the config space, it seems natural to extend the 
> >>config space with other stats.
> >>
> >
> >There is in fact a difference; actual and target are very rarely 
> >updated, while the stats are updated very often.  Using a vq means a 
> >constant number of exits per batch instead of one exit per statistic.  
> >If the vq is host-driven, it also allows the host to control the 
> >update frequency dynamically (i.e. stop polling when there is no 
> >memory pressure).
> 
> I'm not terribly opposed to using a vq for this.  I would expect the 
> stat update interval to be rather long (10s probably) but a vq works 
> just as well.

If there's no memory pressure and no guest activity, you probably want
the stat update to be as rare as possible to avoid wakeups.  Save
power on laptops, that sort of thing.

If there's a host user interested in the state ("qemutop?"), you may
want updates more often than 10s.

-- Jamie

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Daniel P. Berrange]

On Wed, Nov 11, 2009 at 09:24:09AM +0000, Jamie Lokier wrote:
> Anthony Liguori wrote:
> > Avi Kivity wrote:
> > >On 11/10/2009 04:36 PM, Anthony Liguori wrote:
> > >>
> > >>>A stats vq might solve this more cleanly?
> > >>
> > >>actual and target are both really just stats.  Had we implemented 
> > >>those with a vq, I'd be inclined to agree with you but since they're 
> > >>implemented in the config space, it seems natural to extend the 
> > >>config space with other stats.
> > >>
> > >
> > >There is in fact a difference; actual and target are very rarely 
> > >updated, while the stats are updated very often.  Using a vq means a 
> > >constant number of exits per batch instead of one exit per statistic.  
> > >If the vq is host-driven, it also allows the host to control the 
> > >update frequency dynamically (i.e. stop polling when there is no 
> > >memory pressure).
> > 
> > I'm not terribly opposed to using a vq for this.  I would expect the 
> > stat update interval to be rather long (10s probably) but a vq works 
> > just as well.
> 
> If there's no memory pressure and no guest activity, you probably want
> the stat update to be as rare as possible to avoid wakeups.  Save
> power on laptops, that sort of thing.
> 
> If there's a host user interested in the state ("qemutop?"), you may
> want updates more often than 10s.

This all suggests that we should only update the stats from the guest
when something on the host actually asks for them by issuing the QEMU
monitor command. We don't want any kind of continuous polling of stats
at any frequency, if nothing is using these stats on the host.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Daniel P. Berrange]

On Wed, Nov 11, 2009 at 09:24:09AM +0000, Jamie Lokier wrote:
> Anthony Liguori wrote:
> > Avi Kivity wrote:
> > >On 11/10/2009 04:36 PM, Anthony Liguori wrote:
> > >>
> > >>>A stats vq might solve this more cleanly?
> > >>
> > >>actual and target are both really just stats.  Had we implemented 
> > >>those with a vq, I'd be inclined to agree with you but since they're 
> > >>implemented in the config space, it seems natural to extend the 
> > >>config space with other stats.
> > >>
> > >
> > >There is in fact a difference; actual and target are very rarely 
> > >updated, while the stats are updated very often.  Using a vq means a 
> > >constant number of exits per batch instead of one exit per statistic.  
> > >If the vq is host-driven, it also allows the host to control the 
> > >update frequency dynamically (i.e. stop polling when there is no 
> > >memory pressure).
> > 
> > I'm not terribly opposed to using a vq for this.  I would expect the 
> > stat update interval to be rather long (10s probably) but a vq works 
> > just as well.
> 
> If there's no memory pressure and no guest activity, you probably want
> the stat update to be as rare as possible to avoid wakeups.  Save
> power on laptops, that sort of thing.
> 
> If there's a host user interested in the state ("qemutop?"), you may
> want updates more often than 10s.

This all suggests that we should only update the stats from the guest
when something on the host actually asks for them by issuing the QEMU
monitor command. We don't want any kind of continuous polling of stats
at any frequency, if nothing is using these stats on the host.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Daniel P. Berrange]

On Wed, Nov 11, 2009 at 09:24:09AM +0000, Jamie Lokier wrote:
> Anthony Liguori wrote:
> > Avi Kivity wrote:
> > >On 11/10/2009 04:36 PM, Anthony Liguori wrote:
> > >>
> > >>>A stats vq might solve this more cleanly?
> > >>
> > >>actual and target are both really just stats.  Had we implemented 
> > >>those with a vq, I'd be inclined to agree with you but since they're 
> > >>implemented in the config space, it seems natural to extend the 
> > >>config space with other stats.
> > >>
> > >
> > >There is in fact a difference; actual and target are very rarely 
> > >updated, while the stats are updated very often.  Using a vq means a 
> > >constant number of exits per batch instead of one exit per statistic.  
> > >If the vq is host-driven, it also allows the host to control the 
> > >update frequency dynamically (i.e. stop polling when there is no 
> > >memory pressure).
> > 
> > I'm not terribly opposed to using a vq for this.  I would expect the 
> > stat update interval to be rather long (10s probably) but a vq works 
> > just as well.
> 
> If there's no memory pressure and no guest activity, you probably want
> the stat update to be as rare as possible to avoid wakeups.  Save
> power on laptops, that sort of thing.
> 
> If there's a host user interested in the state ("qemutop?"), you may
> want updates more often than 10s.

This all suggests that we should only update the stats from the guest
when something on the host actually asks for them by issuing the QEMU
monitor command. We don't want any kind of continuous polling of stats
at any frequency, if nothing is using these stats on the host.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Adam Litke]

On Wed, 2009-11-11 at 10:12 +0000, Daniel P. Berrange wrote:
> This all suggests that we should only update the stats from the guest
> when something on the host actually asks for them by issuing the QEMU
> monitor command. We don't want any kind of continuous polling of stats
> at any frequency, if nothing is using these stats on the host.

Agreed.  The next version of the patch will remove the timer completely.
We'll wake up in response to config change notifications only.

-- 
Thanks,
Adam

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Adam Litke]

On Wed, 2009-11-11 at 10:12 +0000, Daniel P. Berrange wrote:
> This all suggests that we should only update the stats from the guest
> when something on the host actually asks for them by issuing the QEMU
> monitor command. We don't want any kind of continuous polling of stats
> at any frequency, if nothing is using these stats on the host.

Agreed.  The next version of the patch will remove the timer completely.
We'll wake up in response to config change notifications only.

-- 
Thanks,
Adam

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Adam Litke]

On Wed, 2009-11-11 at 10:12 +0000, Daniel P. Berrange wrote:
> This all suggests that we should only update the stats from the guest
> when something on the host actually asks for them by issuing the QEMU
> monitor command. We don't want any kind of continuous polling of stats
> at any frequency, if nothing is using these stats on the host.

Agreed.  The next version of the patch will remove the timer completely.
We'll wake up in response to config change notifications only.

-- 
Thanks,
Adam

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Avi Kivity]

On 11/11/2009 03:26 PM, Adam Litke wrote:
> On Wed, 2009-11-11 at 10:12 +0000, Daniel P. Berrange wrote:
>    
>> This all suggests that we should only update the stats from the guest
>> when something on the host actually asks for them by issuing the QEMU
>> monitor command. We don't want any kind of continuous polling of stats
>> at any frequency, if nothing is using these stats on the host.
>>      
> Agreed.  The next version of the patch will remove the timer completely.
> We'll wake up in response to config change notifications only.
>    

A vq with its own interrupt would be much nicer.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Avi Kivity]

On 11/11/2009 03:26 PM, Adam Litke wrote:
> On Wed, 2009-11-11 at 10:12 +0000, Daniel P. Berrange wrote:
>    
>> This all suggests that we should only update the stats from the guest
>> when something on the host actually asks for them by issuing the QEMU
>> monitor command. We don't want any kind of continuous polling of stats
>> at any frequency, if nothing is using these stats on the host.
>>      
> Agreed.  The next version of the patch will remove the timer completely.
> We'll wake up in response to config change notifications only.
>    

A vq with its own interrupt would be much nicer.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Avi Kivity]

On 11/11/2009 03:26 PM, Adam Litke wrote:
> On Wed, 2009-11-11 at 10:12 +0000, Daniel P. Berrange wrote:
>    
>> This all suggests that we should only update the stats from the guest
>> when something on the host actually asks for them by issuing the QEMU
>> monitor command. We don't want any kind of continuous polling of stats
>> at any frequency, if nothing is using these stats on the host.
>>      
> Agreed.  The next version of the patch will remove the timer completely.
> We'll wake up in response to config change notifications only.
>    

A vq with its own interrupt would be much nicer.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: virtio: Add memory statistics reporting to the balloon driver

[Adam Litke]

On Wed, 2009-11-11 at 13:13 +1030, Rusty Russell wrote:
> > It's not laziness, it's consistency.  How is actual different than free 
> > memory or any other stat?
> 
> Because it's a COLLECTION of stats.  For example, swap in should be < swap
> out.  Now, the current Linux implementation of all_vm_events() is non-atomic
> anyway, so maybe we can just document this as best-effort.  I'm saying that
> if it *is* a problem, I think we need a vq.

I can't see why we would care about the atomicity of the collection of
statistics.  Best-effort is good enough.  Any variance within the stats
will be overshadowed by the latency of the host-side management daemon.

> But it raises the question: what stats are generally useful cross-OS?  Should
> we be supplying numbers like "unused" (free) "instantly discardable" (ie.
> clean), "discardable to disk" (ie. file-backed), "discardable to swap"
> (ie. swap-backed) and "unswappable" instead?

While I see the virtue in presenting abstracted memory stats that seem
more digestible in a virtualization context, I think we should keep the
raw stats.  This concentrates the complexity in the host-side management
daemon, and allows the host daemon to make better decisions (ie. by
reacting to trends in individual statistics).  Different OSes (or
different versions of the same OS), may also have different sets of
statistics that will provide the answers that a management daemon needs.


-- 
Thanks,
Adam

[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Adam Litke]

On Wed, 2009-11-11 at 13:13 +1030, Rusty Russell wrote:
> > It's not laziness, it's consistency.  How is actual different than free 
> > memory or any other stat?
> 
> Because it's a COLLECTION of stats.  For example, swap in should be < swap
> out.  Now, the current Linux implementation of all_vm_events() is non-atomic
> anyway, so maybe we can just document this as best-effort.  I'm saying that
> if it *is* a problem, I think we need a vq.

I can't see why we would care about the atomicity of the collection of
statistics.  Best-effort is good enough.  Any variance within the stats
will be overshadowed by the latency of the host-side management daemon.

> But it raises the question: what stats are generally useful cross-OS?  Should
> we be supplying numbers like "unused" (free) "instantly discardable" (ie.
> clean), "discardable to disk" (ie. file-backed), "discardable to swap"
> (ie. swap-backed) and "unswappable" instead?

While I see the virtue in presenting abstracted memory stats that seem
more digestible in a virtualization context, I think we should keep the
raw stats.  This concentrates the complexity in the host-side management
daemon, and allows the host daemon to make better decisions (ie. by
reacting to trends in individual statistics).  Different OSes (or
different versions of the same OS), may also have different sets of
statistics that will provide the answers that a management daemon needs.


-- 
Thanks,
Adam

Re: virtio: Add memory statistics reporting to the balloon driver

[Adam Litke]

On Wed, 2009-11-11 at 13:13 +1030, Rusty Russell wrote:
> > It's not laziness, it's consistency.  How is actual different than free 
> > memory or any other stat?
> 
> Because it's a COLLECTION of stats.  For example, swap in should be < swap
> out.  Now, the current Linux implementation of all_vm_events() is non-atomic
> anyway, so maybe we can just document this as best-effort.  I'm saying that
> if it *is* a problem, I think we need a vq.

I can't see why we would care about the atomicity of the collection of
statistics.  Best-effort is good enough.  Any variance within the stats
will be overshadowed by the latency of the host-side management daemon.

> But it raises the question: what stats are generally useful cross-OS?  Should
> we be supplying numbers like "unused" (free) "instantly discardable" (ie.
> clean), "discardable to disk" (ie. file-backed), "discardable to swap"
> (ie. swap-backed) and "unswappable" instead?

While I see the virtue in presenting abstracted memory stats that seem
more digestible in a virtualization context, I think we should keep the
raw stats.  This concentrates the complexity in the host-side management
daemon, and allows the host daemon to make better decisions (ie. by
reacting to trends in individual statistics).  Different OSes (or
different versions of the same OS), may also have different sets of
statistics that will provide the answers that a management daemon needs.


-- 
Thanks,
Adam

Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Thu, 12 Nov 2009 01:38:34 am Adam Litke wrote:
> > But it raises the question: what stats are generally useful cross-OS?  Should
> > we be supplying numbers like "unused" (free) "instantly discardable" (ie.
> > clean), "discardable to disk" (ie. file-backed), "discardable to swap"
> > (ie. swap-backed) and "unswappable" instead?
> 
> While I see the virtue in presenting abstracted memory stats that seem
> more digestible in a virtualization context, I think we should keep the
> raw stats.  This concentrates the complexity in the host-side management
> daemon, and allows the host daemon to make better decisions (ie. by
> reacting to trends in individual statistics).  Different OSes (or
> different versions of the same OS), may also have different sets of
> statistics that will provide the answers that a management daemon needs.

OK, I see you made each one a separate feature bit, which does allow this
somewhat.  But you can't just change the meaning arbitrarily, all you can
do is refuse to supply some of them.  This is because virtio is an ABI,
but also it's plain sanity: run a new guest on an old host and get crazy
answers.

Two more questions:

I assume memtot should be equal to the initial memory granted to the guest
(perhaps reduced if the guest can't use all the memory for internal reasons)?

I'm not sure of the relevance to the host of the number of anonymous pages?
That's why I wondered if unswappable pages would be a better number to supply?

Thanks,
Rusty.

[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Thu, 12 Nov 2009 01:38:34 am Adam Litke wrote:
> > But it raises the question: what stats are generally useful cross-OS?  Should
> > we be supplying numbers like "unused" (free) "instantly discardable" (ie.
> > clean), "discardable to disk" (ie. file-backed), "discardable to swap"
> > (ie. swap-backed) and "unswappable" instead?
> 
> While I see the virtue in presenting abstracted memory stats that seem
> more digestible in a virtualization context, I think we should keep the
> raw stats.  This concentrates the complexity in the host-side management
> daemon, and allows the host daemon to make better decisions (ie. by
> reacting to trends in individual statistics).  Different OSes (or
> different versions of the same OS), may also have different sets of
> statistics that will provide the answers that a management daemon needs.

OK, I see you made each one a separate feature bit, which does allow this
somewhat.  But you can't just change the meaning arbitrarily, all you can
do is refuse to supply some of them.  This is because virtio is an ABI,
but also it's plain sanity: run a new guest on an old host and get crazy
answers.

Two more questions:

I assume memtot should be equal to the initial memory granted to the guest
(perhaps reduced if the guest can't use all the memory for internal reasons)?

I'm not sure of the relevance to the host of the number of anonymous pages?
That's why I wondered if unswappable pages would be a better number to supply?

Thanks,
Rusty.

Re: virtio: Add memory statistics reporting to the balloon driver

[Rusty Russell]

On Thu, 12 Nov 2009 01:38:34 am Adam Litke wrote:
> > But it raises the question: what stats are generally useful cross-OS?  Should
> > we be supplying numbers like "unused" (free) "instantly discardable" (ie.
> > clean), "discardable to disk" (ie. file-backed), "discardable to swap"
> > (ie. swap-backed) and "unswappable" instead?
> 
> While I see the virtue in presenting abstracted memory stats that seem
> more digestible in a virtualization context, I think we should keep the
> raw stats.  This concentrates the complexity in the host-side management
> daemon, and allows the host daemon to make better decisions (ie. by
> reacting to trends in individual statistics).  Different OSes (or
> different versions of the same OS), may also have different sets of
> statistics that will provide the answers that a management daemon needs.

OK, I see you made each one a separate feature bit, which does allow this
somewhat.  But you can't just change the meaning arbitrarily, all you can
do is refuse to supply some of them.  This is because virtio is an ABI,
but also it's plain sanity: run a new guest on an old host and get crazy
answers.

Two more questions:

I assume memtot should be equal to the initial memory granted to the guest
(perhaps reduced if the guest can't use all the memory for internal reasons)?

I'm not sure of the relevance to the host of the number of anonymous pages?
That's why I wondered if unswappable pages would be a better number to supply?

Thanks,
Rusty.

Re: virtio: Add memory statistics reporting to the balloon driver

[Anthony Liguori]

agl@linux.vnet.ibm.com wrote:
> Here are the corresponding changes to the Linux virtio driver...
>
>     virtio: Add memory statistics reporting to the balloon driver
>     
>     When using ballooning to manage overcommitted memory on a host, a system for
>     guests to communicate their memory usage to the host can provide information
>     that will minimize the impact of ballooning on the guests.  The current method
>     employs a daemon running in each guest that communicates memory statistics to a
>     host daemon at a specified time interval.  The host daemon aggregates this
>     information and inflates and/or deflates balloons according to the level of
>     host memory pressure.  This approach is effective but overly complex since a
>     daemon must be installed inside each guest and coordinated to communicate with
>     the host.  A simpler approach is to collect memory statistics in the virtio
>     balloon driver and communicate them to the host via the device config space.
>     
>     This patch enables the guest-side support by adding stats collection and
>     reporting to the virtio balloon driver.
>     
>     Signed-off-by: Adam Litke <agl@us.ibm.com>
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 3a43ebf..1029363 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -135,6 +135,7 @@ static int virtio_dev_probe(struct device *_d)
>  			set_bit(i, dev->features);
>
>  	dev->config->finalize_features(dev);
> +	printk("virtio_dev_probe: final features = %lx\n", dev->features[0]);
>   

Looks like leftover debugging.

>  	err = drv->probe(dev);
>  	if (err)
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 200c22f..77cb953 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -180,6 +180,45 @@ static void update_balloon_size(struct virtio_balloon *vb)
>  			      &actual, sizeof(actual));
>  }
>
> +static inline void update_stat(struct virtio_device *vdev, int feature,
> +				unsigned long value, unsigned offset)
> +{
> +	if (virtio_has_feature(vdev, feature)) {
> +		vdev->config->set(vdev, offset, &value, sizeof(value));
>   

I think this bit assumes a little endian guest.  We shouldn't make that 
assumption.

For virtio kernel patches, please CC the virtualization list and Rusty 
as he's the maintainer.  It wouldn't hurt to CC lkml either.

-- 
Regards,

Anthony Liguori