Re: virtio: Add memory statistics reporting to the balloon driver (V4)
On 12/01/2009 05:05 PM, Adam Litke wrote: > On Tue, 2009-12-01 at 16:56 +0200, Avi Kivity wrote: > >> What about a spec update? Did that happen and I just missed it? >> > Yes it did. I forgot to update this patch leader note with the link: > http://ozlabs.org/~rusty/virtio-spec/virtio-spec-0.8.2.pdf > Great, thanks. -- error compiling committee.c: too many arguments to function ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: virtio: Add memory statistics reporting to the balloon driver (V4)
On Tue, 2009-12-01 at 16:56 +0200, Avi Kivity wrote: > What about a spec update? Did that happen and I just missed it? Yes it did. I forgot to update this patch leader note with the link: http://ozlabs.org/~rusty/virtio-spec/virtio-spec-0.8.2.pdf -- Thanks, Adam ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: virtio: Add memory statistics reporting to the balloon driver (V4)
On 11/30/2009 06:14 PM, Adam Litke wrote: > Changes since V3: > - Do not do endian conversions as they will be done in the host > - Report stats that reference a quantity of memory in bytes > - Minor coding style updates > > 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. > > What about a spec update? Did that happen and I just missed it? -- error compiling committee.c: too many arguments to function ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: virtio: Add memory statistics reporting to the balloon driver (V4)
Thanks Rusty. Acked-by: Adam Litke On Tue, 2009-12-01 at 12:54 +1030, Rusty Russell wrote: > On Tue, 1 Dec 2009 02:44:15 am Adam Litke wrote: > > Changes since V3: > > OK, I applied this. And here's the fixes I applied afterwards (to save YA > round trip: please ack and I'll fold them together) > > virtio: balloon fixes > > 1) Tag and val args to update_stat() are no longer __le. > 2) pages_to_bytes() should promote to u64 so we don't truncate. > 3) Fix two checkpatch.pl warnings. > > Signed-off-by: Rusty Russell > --- > drivers/virtio/virtio_balloon.c |8 +--- > include/linux/virtio_balloon.h |3 +-- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -158,14 +158,15 @@ static void leak_balloon(struct virtio_b > } > > static inline void update_stat(struct virtio_balloon *vb, int idx, > - __le16 tag, __le64 val) > +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) ((x) << PAGE_SHIFT) > +#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]; > @@ -296,7 +297,8 @@ static int virtballoon_probe(struct virt >* use it to signal us later. >*/ > sg_init_one(&sg, vb->stats, sizeof vb->stats); > - if (vb->stats_vq->vq_ops->add_buf(vb->stats_vq, &sg, 1, 0, vb) > < 0) > + if (vb->stats_vq->vq_ops->add_buf(vb->stats_vq, > + &sg, 1, 0, vb) < 0) > BUG(); > vb->stats_vq->vq_ops->kick(vb->stats_vq); > } > diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h > --- a/include/linux/virtio_balloon.h > +++ b/include/linux/virtio_balloon.h > @@ -28,8 +28,7 @@ struct virtio_balloon_config > #define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */ > #define VIRTIO_BALLOON_S_NR 6 > > -struct virtio_balloon_stat > -{ > +struct virtio_balloon_stat { > u16 tag; > u64 val; > } __attribute__((packed)); > > -- Thanks, Adam ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: virtio: Add memory statistics reporting to the balloon driver (V4)
On Tue, 1 Dec 2009 02:44:15 am Adam Litke wrote: > Changes since V3: OK, I applied this. And here's the fixes I applied afterwards (to save YA round trip: please ack and I'll fold them together) virtio: balloon fixes 1) Tag and val args to update_stat() are no longer __le. 2) pages_to_bytes() should promote to u64 so we don't truncate. 3) Fix two checkpatch.pl warnings. Signed-off-by: Rusty Russell --- drivers/virtio/virtio_balloon.c |8 +--- include/linux/virtio_balloon.h |3 +-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -158,14 +158,15 @@ static void leak_balloon(struct virtio_b } static inline void update_stat(struct virtio_balloon *vb, int idx, - __le16 tag, __le64 val) + 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) ((x) << PAGE_SHIFT) +#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]; @@ -296,7 +297,8 @@ static int virtballoon_probe(struct virt * use it to signal us later. */ sg_init_one(&sg, vb->stats, sizeof vb->stats); - if (vb->stats_vq->vq_ops->add_buf(vb->stats_vq, &sg, 1, 0, vb) < 0) + if (vb->stats_vq->vq_ops->add_buf(vb->stats_vq, + &sg, 1, 0, vb) < 0) BUG(); vb->stats_vq->vq_ops->kick(vb->stats_vq); } diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h --- a/include/linux/virtio_balloon.h +++ b/include/linux/virtio_balloon.h @@ -28,8 +28,7 @@ struct virtio_balloon_config #define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */ #define VIRTIO_BALLOON_S_NR 6 -struct virtio_balloon_stat -{ +struct virtio_balloon_stat { u16 tag; u64 val; } __attribute__((packed)); ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
virtio: Add memory statistics reporting to the balloon driver (V4)
Changes since V3: - Do not do endian conversions as they will be done in the host - Report stats that reference a quantity of memory in bytes - Minor coding style updates 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: virtualization@lists.linux-foundation.org Cc: linux-ker...@vger.kernel.org diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 200c22f..f4c8cd8 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,61 @@ 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 = tag; + vb->stats[idx].val = val; +} + +#define pages_to_bytes(x) ((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. + */ +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 +263,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_ack }; - const char *names[] = { "inflate", "deflate" }; - int err; + struct virtqueue *vqs[3]; + vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_ack }; + const char *names[] = { "inflate", "deflate", "stats" }; + int err, nvqs; vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL); if (!vb) { @@ -221,13
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: virtualization@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 Z
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: virtualization@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 o
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: virtualization@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
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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
virtio: Add memory statistics reporting to the balloon driver (V3)
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: virtualization@lists.linux-foundation.org Cc: linux-ker...@vger.kernel.org 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); +} + +#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; + 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 virtio_device *vdev) { struct virtio_balloon *vb; - struct virtqueue *vqs[2]; - vq_callback_t *callbacks[] = { balloon_ack, balloon_ack }; - const char *names[] = { "inflate", "deflate" }; - int err; + struct virtqueue *vqs[3]; + vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_ack }; + const char *names[] = { "inflate", "deflate", "stats" }; + int err, nvqs; vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL); if (!vb) { @@ -221,13 +275,28 @@ static int virtballoon_probe(struct virtio_device *vdev) init_waitqueue_head(&vb->config_change); vb->vdev = vdev; - /* We expect two virtqueues. */ - err = vdev->config->find_vqs(
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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
virtio: Add memory statistics reporting to the balloon driver (V2)
virtio: Add memory statistics reporting to the balloon driver (V2) 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: virtualization@lists.linux-foundation.org Cc: linux-ker...@vger.kernel.org diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 200c22f..59b9533 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,60 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num) } } +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); +} + +#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 long anon_pages; + int idx = 0; + + all_vm_events(events); + si_meminfo(&i); + anon_pages = K(global_page_state(NR_ANON_PAGES)); + + update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN, events[PSWPIN]); + update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT, events[PSWPOUT]); + update_stat(vb, idx++, VIRTIO_BALLOON_S_ANON, anon_pages); + 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 +262,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_ack }; - const char *names[] = { "inflate", "deflate" }; - int err; + struct virtqueue *vqs[3]; + vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_ack }; + const char *names[] = { "inflate", "deflate", "stats" }; + int err, nvqs; vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL); if (!vb) { @@ -221,13 +278,28 @@ static int virtballoon_probe(struct virtio_device *vdev) init_waitqueue_head(&vb->config_change); vb-&g
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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 :| ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
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 Cc: Rusty Russell Cc: Anthony Liguori Cc: Avi Kivity Cc: virtualization@lists.linux-foundation.org Cc: linux-ker...@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_HOST0 /* Tell before reclaiming pages */ + /*
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization