[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V8)
On Tue, 09 Mar 2010 08:48:43 -0600 Adam Litke wrote: > On Tue, 2010-03-09 at 11:22 -0300, Luiz Capitulino wrote: > > On Tue, 09 Mar 2010 14:51:31 +0100 > > Juan Quintela wrote: > > > > > Any recompilation/etc would break migration. I have tried to understand > > > what happened with monitor async commands, and my head exploded in > > > indirections. > > > > The Monitor needs lots of cleanups to make things more obvious. > > > > > Is there any written explanation of what are we trying to do here? > > > > Only the commit log 940cc30. > > > > Basically, an asynchronous handler has a completion function which is > > called when the handler completes. > > > > If we're in the user Monitor, it's suspended until the completion > > function is called. In QMP, the handler returns immediately and we > > _should_ be emitting an event when we have the answer. > > > > The current code doesn't do that, which seems to be a new issue. > > With current git, I cannot get QMP to recognize any commands. Unless > this is a known issue, I will look into it further to see what has > caused it. You have to issue the 'qmp_capabilites' command before issuing commands, take a look at the QMP/README and QMP/qmp-spec.txt files.
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V8)
Adam Litke wrote:
> The changes in V8 of this patch are related to the monitor infrastructure. No
> changes to the virtio interface core have been made since V4. This is
> intended
> to apply on top of my API for asynchronous monitor commands patch.
I know that I am late reviewing this. Once told that, it has some
issues with migration.
> typedef struct VirtIOBalloon
> {
> +MonitorCompletion *stats_callback;
Notice this stats_callback.
typedef void (MonitorCompletion)(void *opaque, QObject *ret_data);
> +void *stats_opaque_callback_data;
> } VirtIOBalloon;
It don't update the version field, should be two (that is easy to fix).
> qemu_put_be32(f, s->num_pages);
> qemu_put_be32(f, s->actual);
> +qemu_put_buffer(f, (uint8_t *)&s->stats_vq_elem,
> sizeof(VirtQueueElement));
We send a struct directly, migration inter-architectures is broken (not
that virtio drivers in general are good here).
> +qemu_put_buffer(f, (uint8_t *)&s->stats_vq_offset, sizeof(size_t));
> +qemu_put_buffer(f, (uint8_t *)&s->stats_callback,
> sizeof(MonitorCompletion));
We send a pointer to one function.
> +qemu_put_buffer(f, (uint8_t *)&s->stats_opaque_callback_data,
> sizeof(void));
And a pointer to one opaque.
Any recompilation/etc would break migration. I have tried to understand
what happened with monitor async commands, and my head exploded in
indirections.
Is there any written explanation of what are we trying to do here?
Later, Juan.
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V8)
On Tue, 09 Mar 2010 14:51:31 +0100 Juan Quintela wrote: > Any recompilation/etc would break migration. I have tried to understand > what happened with monitor async commands, and my head exploded in > indirections. The Monitor needs lots of cleanups to make things more obvious. > Is there any written explanation of what are we trying to do here? Only the commit log 940cc30. Basically, an asynchronous handler has a completion function which is called when the handler completes. If we're in the user Monitor, it's suspended until the completion function is called. In QMP, the handler returns immediately and we _should_ be emitting an event when we have the answer. The current code doesn't do that, which seems to be a new issue.
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V8)
On Tue, 2010-03-09 at 11:22 -0300, Luiz Capitulino wrote: > On Tue, 09 Mar 2010 14:51:31 +0100 > Juan Quintela wrote: > > > Any recompilation/etc would break migration. I have tried to understand > > what happened with monitor async commands, and my head exploded in > > indirections. > > The Monitor needs lots of cleanups to make things more obvious. > > > Is there any written explanation of what are we trying to do here? > > Only the commit log 940cc30. > > Basically, an asynchronous handler has a completion function which is > called when the handler completes. > > If we're in the user Monitor, it's suspended until the completion > function is called. In QMP, the handler returns immediately and we > _should_ be emitting an event when we have the answer. > > The current code doesn't do that, which seems to be a new issue. With current git, I cannot get QMP to recognize any commands. Unless this is a known issue, I will look into it further to see what has caused it. -- Thanks, Adam
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V7)
On Mon, 2010-01-18 at 12:12 -0200, Luiz Capitulino wrote:
> On Fri, 15 Jan 2010 13:54:29 -0600
> Adam Litke wrote:
>
> > This version improves support for multiple monitors and has been ported up
> > to
> > HEAD as of 01/14.
>
> Overall review on the Monitor related changes seems ok, but I'm not sure
> how I should enable it.
You must run a Linux guest with the virtio balloon enabled (-balloon
virtio) and the guest kernel must support the stats API. Rusty Russell
has accepted my patch into his tree and will be pushing it upstream. In
the meantime, you could build a 2.6.33-rc3 kernel with the following
patch applied...
commit b9bb81c2f7ce82d636d47089d6aa279d9be704f2
Author: li...@us.ibm.com
Date: Fri Jan 8 14:17:45 2010 -0800
balloon memory stats patch
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 9dd5880..f95be86 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -28,7 +28,7 @@
struct virtio_balloon
{
struct virtio_device *vdev;
- struct virtqueue *inflate_vq, *deflate_vq;
+ struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
/* Where the ballooning thread waits for config to change. */
wait_queue_head_t config_change;
@@ -49,6 +49,10 @@ struct virtio_balloon
/* The array of pfns we tell the Host about. */
unsigned int num_pfns;
u32 pfns[256];
+
+ /* Memory statistics */
+ int need_stats_update;
+ struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
};
static struct virtio_device_id id_table[] = {
@@ -154,6 +158,72 @@ static void leak_balloon(struct virtio_balloon *vb, size_t
num)
}
}
+static inline void update_stat(struct virtio_balloon *vb, int idx,
+ u16 tag, u64 val)
+{
+ BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
+ vb->stats[idx].tag = tag;
+ vb->stats[idx].val = val;
+}
+
+#define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
+
+static void update_balloon_stats(struct virtio_balloon *vb)
+{
+ unsigned long events[NR_VM_EVENT_ITEMS];
+ struct sysinfo i;
+ int idx = 0;
+
+ all_vm_events(events);
+ si_meminfo(&i);
+
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
+ pages_to_bytes(events[PSWPIN]));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
+ pages_to_bytes(events[PSWPOUT]));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
+ pages_to_bytes(i.freeram));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
+ pages_to_bytes(i.totalram));
+}
+
+/*
+ * While most virtqueues communicate guest-initiated requests to the
hypervisor,
+ * the stats queue operates in reverse. The driver initializes the virtqueue
+ * with a single buffer. From that point forward, all conversations consist of
+ * a hypervisor request (a call to this function) which directs us to refill
+ * the virtqueue with a fresh stats buffer. Since stats collection can sleep,
+ * we notify our kthread which does the actual work via stats_handle_request().
+ */
+static void stats_request(struct virtqueue *vq)
+{
+ struct virtio_balloon *vb;
+ unsigned int len;
+
+ vb = vq->vq_ops->get_buf(vq, &len);
+ if (!vb)
+ return;
+ vb->need_stats_update = 1;
+ wake_up(&vb->config_change);
+}
+
+static void stats_handle_request(struct virtio_balloon *vb)
+{
+ struct virtqueue *vq;
+ struct scatterlist sg;
+
+ vb->need_stats_update = 0;
+ update_balloon_stats(vb);
+
+ vq = vb->stats_vq;
+ sg_init_one(&sg, vb->stats, sizeof(vb->stats));
+ if (vq->vq_ops->add_buf(vq, &sg, 1, 0, vb) < 0)
+ BUG();
+ vq->vq_ops->kick(vq);
+}
+
static void virtballoon_changed(struct virtio_device *vdev)
{
struct virtio_balloon *vb = vdev->priv;
@@ -190,8 +260,11 @@ static int balloon(void *_vballoon)
try_to_freeze();
wait_event_interruptible(vb->config_change,
(diff = towards_target(vb)) != 0
+|| vb->need_stats_update
|| kthread_should_stop()
|| freezing(current));
+ if (vb->need_stats_update)
+ stats_handle_request(vb);
if (diff > 0)
fill_balloon(vb, diff);
else if (diff < 0)
@@ -204,10 +277,10 @@ static int balloon(void *_vballoon)
static int virtballoon_probe(struct virtio_device *vdev)
{
struct virtio_balloon *vb;
- struct virtqueue *vqs[2];
- vq_callback_t *callbacks[] = { balloon_ack, balloon_
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V7)
On Fri, 15 Jan 2010 13:54:29 -0600 Adam Litke wrote: > This version improves support for multiple monitors and has been ported up to > HEAD as of 01/14. Overall review on the Monitor related changes seems ok, but I'm not sure how I should enable it.
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V6)
On Wed, 13 Jan 2010 12:59:25 -0600
Adam Litke wrote:
> > > +/*
> > > + * complete_stats_request - Clean up and report statistics.
> > > + */
> > > +static void complete_stats_request(VirtIOBalloon *vb)
> > > +{
> > > +QObject *stats = get_stats_qobject(vb);
> > > +
> > > +if (vb->stats_request_mode == QEMU_BALLOON_MODE_SYNC) {
> > > +qemu_del_timer(vb->stats_timer);
> > > +monitor_print_balloon(cur_mon, stats);
> > > +monitor_resume(cur_mon);
> > > +} else if (vb->stats_request_mode == QEMU_BALLOON_MODE_ASYNC) {
> > > +monitor_protocol_event(QEVENT_BALLOON, stats);
> > > +}
> > > +
> > > +vb->stats_request_mode = QEMU_BALLOON_MODE_NONE;
> > > +}
> >
> > In the previous thread Anthony raised some issues about the 'cur_mon'
> > usage that made me concerned, because some important code rely on
> > it (read async events).
> >
> > As far as I could check, 'cur_mon' is guaranteed to be the default
> > Monitor which is fine if you're aware of it when putting QEMU to run,
> > but I'm afraid that testing your patch with two Monitors (user and qmp)
> > is not going to work.
> >
> > Maybe not a big deal, but would be good to be aware of potential
> > issues.
>
> I talked to Anthony and will try to fix this up. I just need to start
> passing the monitor pointer around as well.
After doing that you could help us improving the Monitor and check
the other cur_mon usages too :)
> > > -void qemu_balloon(ram_addr_t target)
> > > +int qemu_balloon(ram_addr_t target)
> > > {
> > > -if (qemu_balloon_event)
> > > -qemu_balloon_event(qemu_balloon_event_opaque, target);
> > > +if (qemu_balloon_event) {
> > > +qemu_balloon_event(qemu_balloon_event_opaque, target,
> > > + QEMU_BALLOON_MODE_SYNC);
> > > +return 1;
> > > +} else {
> > > +return 0;
> > > +}
> > > }
> >
> > This is used by do_balloon() right? Which is also used by QMP,
> > shouldn't it also handle async vs. sync?
>
> qemu_balloon always acts synchronously. It does not wait on the guest
> to do anything and it does not return data.
Fine then.
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V6)
On Wed, 2010-01-13 at 16:04 -0200, Luiz Capitulino wrote:
> I've tried to apply this patch to play with it, but turns out it conflicts
> with recent changes in hw/virtio-balloon.
Ahh, I will continue my never-ending quest to stay current :)
> Some comments on the QMP side of the patch follows.
Thanks for your review!
> > +/*
> > + * complete_stats_request - Clean up and report statistics.
> > + */
> > +static void complete_stats_request(VirtIOBalloon *vb)
> > +{
> > +QObject *stats = get_stats_qobject(vb);
> > +
> > +if (vb->stats_request_mode == QEMU_BALLOON_MODE_SYNC) {
> > +qemu_del_timer(vb->stats_timer);
> > +monitor_print_balloon(cur_mon, stats);
> > +monitor_resume(cur_mon);
> > +} else if (vb->stats_request_mode == QEMU_BALLOON_MODE_ASYNC) {
> > +monitor_protocol_event(QEVENT_BALLOON, stats);
> > +}
> > +
> > +vb->stats_request_mode = QEMU_BALLOON_MODE_NONE;
> > +}
>
> In the previous thread Anthony raised some issues about the 'cur_mon'
> usage that made me concerned, because some important code rely on
> it (read async events).
>
> As far as I could check, 'cur_mon' is guaranteed to be the default
> Monitor which is fine if you're aware of it when putting QEMU to run,
> but I'm afraid that testing your patch with two Monitors (user and qmp)
> is not going to work.
>
> Maybe not a big deal, but would be good to be aware of potential
> issues.
I talked to Anthony and will try to fix this up. I just need to start
passing the monitor pointer around as well.
*
> > +if (monitor_ctrl_mode(mon))
> > +mode = QEMU_BALLOON_MODE_ASYNC;
> > +else
> > +
> > +mode = monitor_ctrl_mode(mon) ?
> > +QEMU_BALLOON_MODE_ASYNC :
> > QEMU_BALLOON_MODE_SYNC;
>
> I think what you want is:
>
> } else {
> mode = QEMU_BALLOON_MODE_SYNC;
> }
Bah... Left over gunk.
> > -void qemu_balloon(ram_addr_t target)
> > +int qemu_balloon(ram_addr_t target)
> > {
> > -if (qemu_balloon_event)
> > -qemu_balloon_event(qemu_balloon_event_opaque, target);
> > +if (qemu_balloon_event) {
> > +qemu_balloon_event(qemu_balloon_event_opaque, target,
> > + QEMU_BALLOON_MODE_SYNC);
> > +return 1;
> > +} else {
> > +return 0;
> > +}
> > }
>
> This is used by do_balloon() right? Which is also used by QMP,
> shouldn't it also handle async vs. sync?
qemu_balloon always acts synchronously. It does not wait on the guest
to do anything and it does not return data.
--
Thanks,
Adam
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V6)
On Mon, 11 Jan 2010 13:09:35 -0600
Adam Litke wrote:
> After some good discussion, V6 of this patch integrates well with the new QMP
> support. When the monitor is in QMP mode, the query-balloon command triggers
> a
> stats refresh request to the guest. This request is asynchronous. If the
> guest does not respond then nothing further happens. When stats are updated,
> a
> BALLOON monitor event is raised and the data element will contain the memory
> statistics.
>
> For the user monitor, a timer has been added to prevent monitor hangs with
> unresponsive guests. When the timer fires, the most recently collected stats
> are returned along with an additional entry 'age' which indicates the number
> of
> host_clock milliseconds that have passed since the stats were collected.
>
> This method for dealing with asynchronous commands may prove useful for other
> existing or future commands. In that case, we may want to consider
> incorporating this into the actual monitor API.
Yeah.
Sorry for the delay in reviewing.
I've tried to apply this patch to play with it, but turns out it conflicts
with recent changes in hw/virtio-balloon.
Some comments on the QMP side of the patch follows.
> diff --git a/balloon.h b/balloon.h
> index 60b4a5d..7e29028 100644
> --- a/balloon.h
> +++ b/balloon.h
> @@ -16,12 +16,22 @@
>
> #include "cpu-defs.h"
>
> -typedef ram_addr_t (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
> +/* Timeout for synchronous stats requests (in seconds) */
> +#define QEMU_BALLOON_SYNC_TIMEOUT 5
> +
> +typedef enum {
> +QEMU_BALLOON_MODE_NONE = 0, /* No stats request is active */
> +QEMU_BALLOON_MODE_SYNC = 1, /* Synchronous stats request */
> +QEMU_BALLOON_MODE_ASYNC = 2, /* Asynchronous stats request */
> +} balloon_mode_t;
> +
> +typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target,
> +balloon_mode_t mode);
>
> void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque);
>
> -void qemu_balloon(ram_addr_t target);
> +int qemu_balloon(ram_addr_t target);
>
> -ram_addr_t qemu_balloon_status(void);
> +int qemu_balloon_status(balloon_mode_t mode);
>
> #endif
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index cfd3b41..bf67f4d 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -19,6 +19,10 @@
> #include "balloon.h"
> #include "virtio-balloon.h"
> #include "kvm.h"
> +#include "monitor.h"
> +#include "qlist.h"
> +#include "qint.h"
> +#include "qstring.h"
>
> #if defined(__linux__)
> #include
> @@ -27,9 +31,15 @@
> typedef struct VirtIOBalloon
> {
> VirtIODevice vdev;
> -VirtQueue *ivq, *dvq;
> +VirtQueue *ivq, *dvq, *svq;
> uint32_t num_pages;
> uint32_t actual;
> +uint64_t stats[VIRTIO_BALLOON_S_NR];
> +VirtQueueElement stats_vq_elem;
> +size_t stats_vq_offset;
> +balloon_mode_t stats_request_mode;
> +QEMUTimer *stats_timer;
> +uint64_t stats_updated;
> } VirtIOBalloon;
>
> static VirtIOBalloon *to_virtio_balloon(VirtIODevice *vdev)
> @@ -46,6 +56,50 @@ static void balloon_page(void *addr, int deflate)
> #endif
> }
>
> +/*
> + * reset_stats - Mark all items in the stats array as unset
> + *
> + * This function needs to be called at device intialization and before
> + * before updating to a set of newly-generated stats. This will ensure that
> no
> + * stale values stick around in case the guest reports a subset of the
> supported
> + * statistics.
> + */
> +static inline void reset_stats(VirtIOBalloon *dev)
> +{
> +int i;
> +for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
> +dev->stats_updated = qemu_get_clock(host_clock);
> +}
> +
> +static void stat_put(QDict *dict, const char *label, uint64_t val)
> +{
> +if (val != -1)
> +qdict_put(dict, label, qint_from_int(val));
> +}
> +
> +static QObject *get_stats_qobject(VirtIOBalloon *dev)
> +{
> +QDict *dict = qdict_new();
> +uint32_t actual = ram_size - (dev->actual << VIRTIO_BALLOON_PFN_SHIFT);
> +uint64_t age;
> +
> +stat_put(dict, "actual", actual);
> +stat_put(dict, "mem_swapped_in", dev->stats[VIRTIO_BALLOON_S_SWAP_IN]);
> +stat_put(dict, "mem_swapped_out", dev->stats[VIRTIO_BALLOON_S_SWAP_OUT]);
> +stat_put(dict, "major_page_faults", dev->stats[VIRTIO_BALLOON_S_MAJFLT]);
> +stat_put(dict, "minor_page_faults", dev->stats[VIRTIO_BALLOON_S_MINFLT]);
> +stat_put(dict, "free_mem", dev->stats[VIRTIO_BALLOON_S_MEMFREE]);
> +stat_put(dict, "total_mem", dev->stats[VIRTIO_BALLOON_S_MEMTOT]);
> +
> +/* If age is over the timeout threshold, report it */
> +age = (qemu_get_clock(host_clock) - dev->stats_updated) /
> + (get_ticks_per_sec() / 1000);
> +if (age >= QEMU_BALLOON_SYNC_TIMEOUT * 1000)
> +stat_put(dict, "age", age);
> +
> +return QOBJECT(dict);
> +}
> +
> /* FIXME: once we do a virtio refactoring, this will get subsumed into common
> * code */
> static
Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)
On 11/23/2009 01:00 PM, Dor Laor wrote:
On 11/23/2009 11:44 AM, Michael S. Tsirkin wrote:
On Thu, Nov 19, 2009 at 09:19:05AM -0600, Adam Litke wrote:
Rusty and Anthony,
If I've addressed all outstanding issues, please consider this patch
for
inclusion. Thanks.
Changes since V2:
- Increase stat field size to 64 bits
- Report all sizes in kb (not pages)
- Drop anon_pages stat and fix endianness conversion
Changes since V1:
- Use a virtqueue instead of the device config space
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 directly to the hypervisor.
This patch enables the guest-side support by adding stats collection
and
reporting to the virtio balloon driver.
Signed-off-by: Adam Litke
Cc: Rusty Russell
Cc: Anthony Liguori
Cc: virtualizat...@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
that's pretty clean. some comments: I wrote them while off-line, and now
that I was going to send it out, I see that there's some overlap with
what Rusty wrote. Still, here it is:
diff --git a/drivers/virtio/virtio_balloon.c
b/drivers/virtio/virtio_balloon.c
index 200c22f..ebc9d39 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -29,7 +29,7 @@
struct virtio_balloon
{
struct virtio_device *vdev;
-struct virtqueue *inflate_vq, *deflate_vq;
+struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
/* Where the ballooning thread waits for config to change. */
wait_queue_head_t config_change;
@@ -50,6 +50,9 @@ struct virtio_balloon
/* The array of pfns we tell the Host about. */
unsigned int num_pfns;
u32 pfns[256];
+
+/* Memory statistics */
+struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
};
static struct virtio_device_id id_table[] = {
@@ -155,6 +158,57 @@ static void leak_balloon(struct virtio_balloon
*vb, size_t num)
}
}
+static inline void update_stat(struct virtio_balloon *vb, int idx,
+__le16 tag, __le64 val)
+{
+BUG_ON(idx>= VIRTIO_BALLOON_S_NR);
+vb->stats[idx].tag = cpu_to_le16(tag);
+vb->stats[idx].val = cpu_to_le64(val);
you should do le16_to_cpu etc on __le values.
Try running this patch through sparce checker.
+}
+
+#define K(x) ((x)<< (PAGE_SHIFT - 10))
can't this overflow?
also, won't it be simpler to just report memory is
in bytes, then just x * PAGE_SIZE instead of hairy constants?
+static void update_balloon_stats(struct virtio_balloon *vb)
+{
+unsigned long events[NR_VM_EVENT_ITEMS];
that's about 1/2K worth of stack space on a 64 bit machine.
better keep it as part of struct virtio_balloon.
+struct sysinfo i;
+int idx = 0;
+
+all_vm_events(events);
+si_meminfo(&i);
+
+update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
K(events[PSWPIN]));
+update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
K(events[PSWPOUT]));
+update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT,
events[PGMAJFAULT]);
+update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
+update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE, K(i.freeram));
+update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT, K(i.totalram));
Finally I found some data from our M$ neighbors. This is from
http://www.microsoft.com/whdc/system/sysperf/Perf_tun_srv-R2.mspx
"When running Windows in the child partition, you can use the
following performance counters within a child partition to identify
whether the child partition is experiencing memory pressure and is
likely to perform better with a higher VM memory size:
Memory – Standby Cache Reserve Bytes:
Sum of Standby Cache Reserve Bytes and Free and Zero Page List Bytes
should be 200 MB or more on systems with 1 GB, and 300 MB or more on
systems with 2 GB or more of visible RAM.
Memory – Free & Zero Page List Bytes:
Sum of Standby Cache Reserve Bytes and Free and Zero Page List Bytes
should be 200 MB or more on systems with 1 GB, and 300 MB or more on
systems with 2 GB or more of visible RAM.
Memory – Pages Input/Sec:
Average over a 1-hour period is less than 10.
"
The biggest take out of it is that we may get #0-pages in windows.
It's valuable information for the host since KSM will collapse all of
them anyway, so there is not point in ballooning them.
Vadim, can you check if we can extract
Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)
On 11/23/2009 11:44 AM, Michael S. Tsirkin wrote:
On Thu, Nov 19, 2009 at 09:19:05AM -0600, Adam Litke wrote:
Rusty and Anthony,
If I've addressed all outstanding issues, please consider this patch for
inclusion. Thanks.
Changes since V2:
- Increase stat field size to 64 bits
- Report all sizes in kb (not pages)
- Drop anon_pages stat and fix endianness conversion
Changes since V1:
- Use a virtqueue instead of the device config space
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 directly to the hypervisor.
This patch enables the guest-side support by adding stats collection and
reporting to the virtio balloon driver.
Signed-off-by: Adam Litke
Cc: Rusty Russell
Cc: Anthony Liguori
Cc: virtualizat...@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
that's pretty clean. some comments: I wrote them while off-line, and now
that I was going to send it out, I see that there's some overlap with
what Rusty wrote. Still, here it is:
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 200c22f..ebc9d39 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -29,7 +29,7 @@
struct virtio_balloon
{
struct virtio_device *vdev;
- struct virtqueue *inflate_vq, *deflate_vq;
+ struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
/* Where the ballooning thread waits for config to change. */
wait_queue_head_t config_change;
@@ -50,6 +50,9 @@ struct virtio_balloon
/* The array of pfns we tell the Host about. */
unsigned int num_pfns;
u32 pfns[256];
+
+ /* Memory statistics */
+ struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
};
static struct virtio_device_id id_table[] = {
@@ -155,6 +158,57 @@ static void leak_balloon(struct virtio_balloon *vb, size_t
num)
}
}
+static inline void update_stat(struct virtio_balloon *vb, int idx,
+ __le16 tag, __le64 val)
+{
+ BUG_ON(idx>= VIRTIO_BALLOON_S_NR);
+ vb->stats[idx].tag = cpu_to_le16(tag);
+ vb->stats[idx].val = cpu_to_le64(val);
you should do le16_to_cpu etc on __le values.
Try running this patch through sparce checker.
+}
+
+#define K(x) ((x)<< (PAGE_SHIFT - 10))
can't this overflow?
also, won't it be simpler to just report memory is
in bytes, then just x * PAGE_SIZE instead of hairy constants?
+static void update_balloon_stats(struct virtio_balloon *vb)
+{
+ unsigned long events[NR_VM_EVENT_ITEMS];
that's about 1/2K worth of stack space on a 64 bit machine.
better keep it as part of struct virtio_balloon.
+ struct sysinfo i;
+ int idx = 0;
+
+ all_vm_events(events);
+ si_meminfo(&i);
+
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN, K(events[PSWPIN]));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT, K(events[PSWPOUT]));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE, K(i.freeram));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT, K(i.totalram));
Finally I found some data from our M$ neighbors. This is from
http://www.microsoft.com/whdc/system/sysperf/Perf_tun_srv-R2.mspx
"When running Windows in the child partition, you can use the following
performance counters within a child partition to identify whether the
child partition is experiencing memory pressure and is likely to perform
better with a higher VM memory size:
Memory – Standby Cache Reserve Bytes:
Sum of Standby Cache Reserve Bytes and Free and Zero Page List Bytes
should be 200 MB or more on systems with 1 GB, and 300 MB or more on
systems with 2 GB or more of visible RAM.
Memory – Free & Zero Page List Bytes:
Sum of Standby Cache Reserve Bytes and Free and Zero Page List Bytes
should be 200 MB or more on systems with 1 GB, and 300 MB or more on
systems with 2 GB or more of visible RAM.
Memory – Pages Input/Sec:
Average over a 1-hour period is less than 10.
"
The biggest take out of it is that we may get #0-pages in windows.
It's valuable information for the host since KSM will collapse all of
them anyway, so there is not point in ballooning them.
Vadim, can you ch
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)
On Thu, Nov 19, 2009 at 09:19:05AM -0600, Adam Litke wrote:
> Rusty and Anthony,
> If I've addressed all outstanding issues, please consider this patch for
> inclusion. Thanks.
>
> Changes since V2:
> - Increase stat field size to 64 bits
> - Report all sizes in kb (not pages)
> - Drop anon_pages stat and fix endianness conversion
>
> Changes since V1:
> - Use a virtqueue instead of the device config space
>
> 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 directly to the hypervisor.
>
> This patch enables the guest-side support by adding stats collection and
> reporting to the virtio balloon driver.
>
> Signed-off-by: Adam Litke
> Cc: Rusty Russell
> Cc: Anthony Liguori
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: linux-ker...@vger.kernel.org
that's pretty clean. some comments: I wrote them while off-line, and now
that I was going to send it out, I see that there's some overlap with
what Rusty wrote. Still, here it is:
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 200c22f..ebc9d39 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -29,7 +29,7 @@
> struct virtio_balloon
> {
> struct virtio_device *vdev;
> - struct virtqueue *inflate_vq, *deflate_vq;
> + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
>
> /* Where the ballooning thread waits for config to change. */
> wait_queue_head_t config_change;
> @@ -50,6 +50,9 @@ struct virtio_balloon
> /* The array of pfns we tell the Host about. */
> unsigned int num_pfns;
> u32 pfns[256];
> +
> + /* Memory statistics */
> + struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
> };
>
> static struct virtio_device_id id_table[] = {
> @@ -155,6 +158,57 @@ static void leak_balloon(struct virtio_balloon *vb,
> size_t num)
> }
> }
>
> +static inline void update_stat(struct virtio_balloon *vb, int idx,
> + __le16 tag, __le64 val)
> +{
> + BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
> + vb->stats[idx].tag = cpu_to_le16(tag);
> + vb->stats[idx].val = cpu_to_le64(val);
you should do le16_to_cpu etc on __le values.
Try running this patch through sparce checker.
> +}
> +
> +#define K(x) ((x) << (PAGE_SHIFT - 10))
can't this overflow?
also, won't it be simpler to just report memory is
in bytes, then just x * PAGE_SIZE instead of hairy constants?
> +static void update_balloon_stats(struct virtio_balloon *vb)
> +{
> + unsigned long events[NR_VM_EVENT_ITEMS];
that's about 1/2K worth of stack space on a 64 bit machine.
better keep it as part of struct virtio_balloon.
> + struct sysinfo i;
> + int idx = 0;
> +
> + all_vm_events(events);
> + si_meminfo(&i);
> +
> + update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN, K(events[PSWPIN]));
> + update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT, K(events[PSWPOUT]));
> + update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
> + update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> + update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE, K(i.freeram));
> + update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT, K(i.totalram));
> +}
> +
> +/*
> + * While most virtqueues communicate guest-initiated requests to the
> hypervisor,
> + * the stats queue operates in reverse. The driver initializes the virtqueue
> + * with a single buffer. From that point forward, all conversations consist
> of
> + * a hypervisor request (a call to this function) which directs us to refill
> + * the virtqueue with a fresh stats buffer.
> + */
> +static void stats_ack(struct virtqueue *vq)
> +{
> + struct virtio_balloon *vb;
> + unsigned int len;
> + struct scatterlist sg;
> +
> + vb = vq->vq_ops->get_buf(vq, &len);
> + if (!vb)
> + return;
> +
> + update_balloon_stats(vb);
> +
> + sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> + if (vq->vq_ops->add_buf(vq, &sg, 1, 0, vb) < 0)
> + BUG();
> + vq->vq_ops->kick(vq);
> +}
> +
> static void virtballoon_changed(struct virtio_device *vdev)
> {
> struct virtio_balloon *vb = vdev->priv;
> @@ -205,10 +259,10 @@ static int balloon(void *_vballoon)
> static int virtballoon_probe(struct
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)
On Fri, 20 Nov 2009 01:49:05 am Adam Litke wrote:
> Rusty and Anthony,
> If I've addressed all outstanding issues, please consider this patch for
> inclusion. Thanks.
>
> Changes since V2:
> - Increase stat field size to 64 bits
> - Report all sizes in kb (not pages)
Hi Adam,
Looks like we're very close. A few minor things:
Why k? Why not just do the simplest possible thing, and just report all
stats as straight numbers, now we have 64 bits.
> - Drop anon_pages stat and fix endianness conversion
Please drop endianness conversion.
> +struct virtio_balloon_stat
> +{
> + __le16 tag;
> + __le64 val;
> +};
Let's not introduce padding as well; __attribute__((packed)) here please.
Thanks,
Rusty.
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)
On Thu, 2009-11-19 at 18:13 +0200, Avi Kivity wrote:
> On 11/19/2009 05:58 PM, Adam Litke wrote:
> > On Thu, 2009-11-19 at 17:22 +0200, Avi Kivity wrote:
> >
> >> On 11/19/2009 05:19 PM, Adam Litke wrote:
> >>
> >>> Rusty and Anthony,
> >>> If I've addressed all outstanding issues, please consider this patch for
> >>> inclusion. Thanks.
> >>>
> >>> +struct virtio_balloon_stat
> >>> +{
> >>> + __le16 tag;
> >>> + __le64 val;
> >>> +};
> >>> +
> >>>
> >>>
> >> You're not doing endian conversion in the host?
> >>
> > No. I was following by example. For the virtio_balloon, the existing
> > code is careful so that the guest always writes data in little endian.
> >
>
> I don't follow. If the guest is careful to write little-endian, surely
> the host must be equally careful to read little-endian?
That is true and, by my reading of the existing qemu virtio-balloon
device code, isn't virtio_balloon_set_config() on a big endian host
already broken?
--
Thanks,
Adam
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)
On 11/19/2009 05:58 PM, Adam Litke wrote:
On Thu, 2009-11-19 at 17:22 +0200, Avi Kivity wrote:
On 11/19/2009 05:19 PM, Adam Litke wrote:
Rusty and Anthony,
If I've addressed all outstanding issues, please consider this patch for
inclusion. Thanks.
+struct virtio_balloon_stat
+{
+ __le16 tag;
+ __le64 val;
+};
+
You're not doing endian conversion in the host?
No. I was following by example. For the virtio_balloon, the existing
code is careful so that the guest always writes data in little endian.
I don't follow. If the guest is careful to write little-endian, surely
the host must be equally careful to read little-endian?
--
error compiling committee.c: too many arguments to function
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)
On Thu, 2009-11-19 at 17:22 +0200, Avi Kivity wrote:
> On 11/19/2009 05:19 PM, Adam Litke wrote:
> > Rusty and Anthony,
> > If I've addressed all outstanding issues, please consider this patch for
> > inclusion. Thanks.
> >
> > +struct virtio_balloon_stat
> > +{
> > + __le16 tag;
> > + __le64 val;
> > +};
> > +
> >
>
> You're not doing endian conversion in the host?
No. I was following by example. For the virtio_balloon, the existing
code is careful so that the guest always writes data in little endian.
--
Thanks,
Adam
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)
On 11/19/2009 05:19 PM, Adam Litke wrote:
Rusty and Anthony,
If I've addressed all outstanding issues, please consider this patch for
inclusion. Thanks.
+struct virtio_balloon_stat
+{
+ __le16 tag;
+ __le64 val;
+};
+
You're not doing endian conversion in the host?
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V2)
On Thu, 19 Nov 2009 01:32:26 am Anthony Liguori wrote: > Rusty Russell wrote: > > The little-endian conversion of the balloon driver is a historical mistake > > (no other driver does this). Let's not extend it to the stats. > > I think the mistake is that the other drivers don't do that. > > We cheat in qemu and assume that the guest is always in a fixed > endianness but this is not always the case for all architectures. Perhaps, but it's documented in the spec. My assertion remains that to do any virtualization you need to know what the guest endian is anyway, so endian converts throughout the drivers just add pain for driver authors. > I think making the interface u64 and byte based would be the best > solution. Making assumptions about page size across guest and host is > another thing we should try to avoid. Yep, just report the raw byte counts as u64. Cheers, Rusty.
Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V2)
Anthony Liguori wrote: > Rusty Russell wrote: > >The little-endian conversion of the balloon driver is a historical mistake > >(no other driver does this). Let's not extend it to the stats. > > I think the mistake is that the other drivers don't do that. > > We cheat in qemu and assume that the guest is always in a fixed > endianness but this is not always the case for all architectures. If guests can have different endianness (reasonable on some CPUs where it's switchable - some even have more than 2 options), then I guess the *host* on those systems have different endianness too. Is the host's endianness signalled to the guest anywhere, so that guest drivers can do cpu_to_qemuhost32(), when someone eventually finds that necessary? -- Jamie
Re: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V2)
Rusty Russell wrote:
On Wed, 18 Nov 2009 07:06:29 am Adam Litke wrote:
virtio: Add memory statistics reporting to the balloon driver (V2)
Changes since V1:
- Use a virtqueue instead of the device config space
Hi Adam,
If Anthony's happy, I'm happy with this approach.
Couple of minor points:
+static inline void update_stat(struct virtio_balloon *vb, int idx,
+ unsigned int tag, unsigned long val)
+{
+ BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
+ vb->stats[idx].tag = tag;
+ vb->stats[idx].val = cpu_to_le32(val);
+}
The little-endian conversion of the balloon driver is a historical mistake
(no other driver does this). Let's not extend it to the stats.
I think the mistake is that the other drivers don't do that.
We cheat in qemu and assume that the guest is always in a fixed
endianness but this is not always the case for all architectures.
That said, since we make this mistake everywhere, I guess I understand
the argument to have consistency and to just admit that we're broken
here. But this is where the endianness bits come from.
Here you've done one and not the other, which is even worse. (Sparse would
have found this, I assume).
Yup, that's definitely wrong.
+struct virtio_balloon_stat
+{
+ __le16 tag;
+ __le32 val;
+};
Is 32 bits sufficient? A big machine might get over 4bn faults, and certainly
4 TB of memory isn't that far away.
I think making the interface u64 and byte based would be the best
solution. Making assumptions about page size across guest and host is
another thing we should try to avoid.
Regards,
Anthony Liguori
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V2)
On Wed, 18 Nov 2009 07:06:29 am Adam Litke wrote:
> virtio: Add memory statistics reporting to the balloon driver (V2)
>
> Changes since V1:
> - Use a virtqueue instead of the device config space
Hi Adam,
If Anthony's happy, I'm happy with this approach.
Couple of minor points:
> +static inline void update_stat(struct virtio_balloon *vb, int idx,
> + unsigned int tag, unsigned long val)
> +{
> + BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
> + vb->stats[idx].tag = tag;
> + vb->stats[idx].val = cpu_to_le32(val);
> +}
The little-endian conversion of the balloon driver is a historical mistake
(no other driver does this). Let's not extend it to the stats.
Here you've done one and not the other, which is even worse. (Sparse would
have found this, I assume).
> +struct virtio_balloon_stat
> +{
> + __le16 tag;
> + __le32 val;
> +};
Is 32 bits sufficient? A big machine might get over 4bn faults, and certainly
4 TB of memory isn't that far away.
Thanks,
Rusty.
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver
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
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: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver
On 11/11/2009 03:26 PM, Adam Litke wrote: On Wed, 2009-11-11 at 10:12 +, 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
On Wed, 2009-11-11 at 10:12 +, 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
On Wed, Nov 11, 2009 at 09:24:09AM +, 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
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
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver
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 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
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: [Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver
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.
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver
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
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
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
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
[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver
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
a...@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
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
