| Message ID | 20190724171050.7888.62199.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,QEMU] virtio-balloon: Provide a interface for "bubble hinting" | expand |
On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > Add support for what I am referring to as "bubble hinting". Basically the > idea is to function very similar to how the balloon works in that we > basically end up madvising the page as not being used. However we don't > really need to bother with any deflate type logic since the page will be > faulted back into the guest when it is read or written to. > > This is meant to be a simplification of the existing balloon interface > to use for providing hints to what memory needs to be freed. I am assuming > this is safe to do as the deflate logic does not actually appear to do very > much other than tracking what subpages have been released and which ones > haven't. > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > --- > hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > include/hw/virtio/virtio-balloon.h | 2 + > include/standard-headers/linux/virtio_balloon.h | 1 + > 3 files changed, 42 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index 2112874055fb..70c0004c0f88 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > balloon_stats_change_timer(s, 0); > } > > +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > +{ > + VirtQueueElement *elem; > + > + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > + unsigned int i; > + > + for (i = 0; i < elem->in_num; i++) { > + void *addr = elem->in_sg[i].iov_base; > + size_t size = elem->in_sg[i].iov_len; > + ram_addr_t ram_offset; > + size_t rb_page_size; > + RAMBlock *rb; > + > + if (qemu_balloon_is_inhibited()) > + continue; > + > + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > + rb_page_size = qemu_ram_pagesize(rb); > + > + /* For now we will simply ignore unaligned memory regions */ > + if ((ram_offset | size) & (rb_page_size - 1)) > + continue; > + > + ram_block_discard_range(rb, ram_offset, size); I suspect this needs to do like the migration type of hinting and get disabled if page poisoning is in effect. Right? > + } > + > + virtqueue_push(vq, elem, 0); > + virtio_notify(vdev, vq); > + g_free(elem); > + } > +} > + > static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > { > VirtIOBalloon *s = VIRTIO_BALLOON(vdev); > @@ -782,6 +815,11 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp) > s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats); > > if (virtio_has_feature(s->host_features, > + VIRTIO_BALLOON_F_HINTING)) { > + s->hvq = virtio_add_queue(vdev, 128, virtio_bubble_handle_output); > + } > + > + if (virtio_has_feature(s->host_features, > VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { > s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE, > virtio_balloon_handle_free_page_vq); > @@ -897,6 +935,8 @@ static Property virtio_balloon_properties[] = { > VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false), > DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features, > VIRTIO_BALLOON_F_FREE_PAGE_HINT, false), > + DEFINE_PROP_BIT("guest-page-hinting", VirtIOBalloon, host_features, > + VIRTIO_BALLOON_F_HINTING, true), > DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD, > IOThread *), > DEFINE_PROP_END_OF_LIST(), > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h > index 1afafb12f6bc..a58b24fdf29d 100644 > --- a/include/hw/virtio/virtio-balloon.h > +++ b/include/hw/virtio/virtio-balloon.h > @@ -44,7 +44,7 @@ enum virtio_balloon_free_page_report_status { > > typedef struct VirtIOBalloon { > VirtIODevice parent_obj; > - VirtQueue *ivq, *dvq, *svq, *free_page_vq; > + VirtQueue *ivq, *dvq, *svq, *free_page_vq, *hvq; > uint32_t free_page_report_status; > uint32_t num_pages; > uint32_t actual; > diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h > index 9375ca2a70de..f9e3e8256261 100644 > --- a/include/standard-headers/linux/virtio_balloon.h > +++ b/include/standard-headers/linux/virtio_balloon.h > @@ -36,6 +36,7 @@ > #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */ > #define VIRTIO_BALLOON_F_FREE_PAGE_HINT 3 /* VQ to report free pages */ > #define VIRTIO_BALLOON_F_PAGE_POISON 4 /* Guest is using page poisoning */ > +#define VIRTIO_BALLOON_F_HINTING 5 /* Page hinting virtqueue */ > > /* Size of a PFN in the balloon interface. */ > #define VIRTIO_BALLOON_PFN_SHIFT 12
On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > Add support for what I am referring to as "bubble hinting". Basically the > > idea is to function very similar to how the balloon works in that we > > basically end up madvising the page as not being used. However we don't > > really need to bother with any deflate type logic since the page will be > > faulted back into the guest when it is read or written to. > > > > This is meant to be a simplification of the existing balloon interface > > to use for providing hints to what memory needs to be freed. I am assuming > > this is safe to do as the deflate logic does not actually appear to do very > > much other than tracking what subpages have been released and which ones > > haven't. > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > --- > > hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > > include/hw/virtio/virtio-balloon.h | 2 + > > include/standard-headers/linux/virtio_balloon.h | 1 + > > 3 files changed, 42 insertions(+), 1 deletion(-) > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > index 2112874055fb..70c0004c0f88 100644 > > --- a/hw/virtio/virtio-balloon.c > > +++ b/hw/virtio/virtio-balloon.c > > @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > > balloon_stats_change_timer(s, 0); > > } > > > > +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > +{ > > + VirtQueueElement *elem; > > + > > + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > > + unsigned int i; > > + > > + for (i = 0; i < elem->in_num; i++) { > > + void *addr = elem->in_sg[i].iov_base; > > + size_t size = elem->in_sg[i].iov_len; > > + ram_addr_t ram_offset; > > + size_t rb_page_size; > > + RAMBlock *rb; > > + > > + if (qemu_balloon_is_inhibited()) > > + continue; > > + > > + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > > + rb_page_size = qemu_ram_pagesize(rb); > > + > > + /* For now we will simply ignore unaligned memory regions */ > > + if ((ram_offset | size) & (rb_page_size - 1)) > > + continue; > > + > > + ram_block_discard_range(rb, ram_offset, size); > > I suspect this needs to do like the migration type of > hinting and get disabled if page poisoning is in effect. > Right? Shouldn't something like that end up getting handled via qemu_balloon_is_inhibited, or did I miss something there? I assumed cases like that would end up setting qemu_balloon_is_inhibited to true, if that isn't the case then I could add some additional conditions. I would do it in about the same spot as the qemu_balloon_is_inhibited check.
On 7/24/19 4:18 PM, Alexander Duyck wrote: > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: >> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> >>> >>> Add support for what I am referring to as "bubble hinting". Basically the >>> idea is to function very similar to how the balloon works in that we >>> basically end up madvising the page as not being used. However we don't >>> really need to bother with any deflate type logic since the page will be >>> faulted back into the guest when it is read or written to. >>> >>> This is meant to be a simplification of the existing balloon interface >>> to use for providing hints to what memory needs to be freed. I am assuming >>> this is safe to do as the deflate logic does not actually appear to do very >>> much other than tracking what subpages have been released and which ones >>> haven't. >>> >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> >>> --- >>> hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ >>> include/hw/virtio/virtio-balloon.h | 2 + >>> include/standard-headers/linux/virtio_balloon.h | 1 + >>> 3 files changed, 42 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c >>> index 2112874055fb..70c0004c0f88 100644 >>> --- a/hw/virtio/virtio-balloon.c >>> +++ b/hw/virtio/virtio-balloon.c >>> @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, >>> balloon_stats_change_timer(s, 0); >>> } >>> >>> +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) >>> +{ >>> + VirtQueueElement *elem; >>> + >>> + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { >>> + unsigned int i; >>> + >>> + for (i = 0; i < elem->in_num; i++) { >>> + void *addr = elem->in_sg[i].iov_base; >>> + size_t size = elem->in_sg[i].iov_len; >>> + ram_addr_t ram_offset; >>> + size_t rb_page_size; >>> + RAMBlock *rb; >>> + >>> + if (qemu_balloon_is_inhibited()) >>> + continue; >>> + >>> + rb = qemu_ram_block_from_host(addr, false, &ram_offset); >>> + rb_page_size = qemu_ram_pagesize(rb); >>> + >>> + /* For now we will simply ignore unaligned memory regions */ >>> + if ((ram_offset | size) & (rb_page_size - 1)) >>> + continue; >>> + >>> + ram_block_discard_range(rb, ram_offset, size); >> I suspect this needs to do like the migration type of >> hinting and get disabled if page poisoning is in effect. >> Right? > Shouldn't something like that end up getting handled via > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases > like that would end up setting qemu_balloon_is_inhibited to true, if that > isn't the case then I could add some additional conditions. I would do it > in about the same spot as the qemu_balloon_is_inhibited check. I don't think qemu_balloon_is_inhibited() will take care of the page poisoning situations. If I am not wrong we may have to look to extend VIRTIO_BALLOON_F_PAGE_POISON support as per Michael's suggestion. > >
On Wed, Jul 24, 2019 at 04:29:27PM -0400, Nitesh Narayan Lal wrote: > > On 7/24/19 4:18 PM, Alexander Duyck wrote: > > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: > >> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > >>> > >>> Add support for what I am referring to as "bubble hinting". Basically the > >>> idea is to function very similar to how the balloon works in that we > >>> basically end up madvising the page as not being used. However we don't > >>> really need to bother with any deflate type logic since the page will be > >>> faulted back into the guest when it is read or written to. > >>> > >>> This is meant to be a simplification of the existing balloon interface > >>> to use for providing hints to what memory needs to be freed. I am assuming > >>> this is safe to do as the deflate logic does not actually appear to do very > >>> much other than tracking what subpages have been released and which ones > >>> haven't. > >>> > >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > >>> --- > >>> hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > >>> include/hw/virtio/virtio-balloon.h | 2 + > >>> include/standard-headers/linux/virtio_balloon.h | 1 + > >>> 3 files changed, 42 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > >>> index 2112874055fb..70c0004c0f88 100644 > >>> --- a/hw/virtio/virtio-balloon.c > >>> +++ b/hw/virtio/virtio-balloon.c > >>> @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > >>> balloon_stats_change_timer(s, 0); > >>> } > >>> > >>> +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > >>> +{ > >>> + VirtQueueElement *elem; > >>> + > >>> + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > >>> + unsigned int i; > >>> + > >>> + for (i = 0; i < elem->in_num; i++) { > >>> + void *addr = elem->in_sg[i].iov_base; > >>> + size_t size = elem->in_sg[i].iov_len; > >>> + ram_addr_t ram_offset; > >>> + size_t rb_page_size; > >>> + RAMBlock *rb; > >>> + > >>> + if (qemu_balloon_is_inhibited()) > >>> + continue; > >>> + > >>> + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > >>> + rb_page_size = qemu_ram_pagesize(rb); > >>> + > >>> + /* For now we will simply ignore unaligned memory regions */ > >>> + if ((ram_offset | size) & (rb_page_size - 1)) > >>> + continue; > >>> + > >>> + ram_block_discard_range(rb, ram_offset, size); > >> I suspect this needs to do like the migration type of > >> hinting and get disabled if page poisoning is in effect. > >> Right? > > Shouldn't something like that end up getting handled via > > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases > > like that would end up setting qemu_balloon_is_inhibited to true, if that > > isn't the case then I could add some additional conditions. I would do it > > in about the same spot as the qemu_balloon_is_inhibited check. > I don't think qemu_balloon_is_inhibited() will take care of the page poisoning > situations. > If I am not wrong we may have to look to extend VIRTIO_BALLOON_F_PAGE_POISON > support as per Michael's suggestion. BTW upstream qemu seems to ignore VIRTIO_BALLOON_F_PAGE_POISON ATM. Which is probably a bug. Wei, could you take a look pls? > > > > > -- > Thanks > Nitesh
On Wed, Jul 24, 2019 at 01:18:00PM -0700, Alexander Duyck wrote: > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: > > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > Add support for what I am referring to as "bubble hinting". Basically the > > > idea is to function very similar to how the balloon works in that we > > > basically end up madvising the page as not being used. However we don't > > > really need to bother with any deflate type logic since the page will be > > > faulted back into the guest when it is read or written to. > > > > > > This is meant to be a simplification of the existing balloon interface > > > to use for providing hints to what memory needs to be freed. I am assuming > > > this is safe to do as the deflate logic does not actually appear to do very > > > much other than tracking what subpages have been released and which ones > > > haven't. > > > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > --- > > > hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > > > include/hw/virtio/virtio-balloon.h | 2 + > > > include/standard-headers/linux/virtio_balloon.h | 1 + > > > 3 files changed, 42 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > index 2112874055fb..70c0004c0f88 100644 > > > --- a/hw/virtio/virtio-balloon.c > > > +++ b/hw/virtio/virtio-balloon.c > > > @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > > > balloon_stats_change_timer(s, 0); > > > } > > > > > > +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > +{ > > > + VirtQueueElement *elem; > > > + > > > + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > > > + unsigned int i; > > > + > > > + for (i = 0; i < elem->in_num; i++) { > > > + void *addr = elem->in_sg[i].iov_base; > > > + size_t size = elem->in_sg[i].iov_len; > > > + ram_addr_t ram_offset; > > > + size_t rb_page_size; > > > + RAMBlock *rb; > > > + > > > + if (qemu_balloon_is_inhibited()) > > > + continue; > > > + > > > + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > > > + rb_page_size = qemu_ram_pagesize(rb); > > > + > > > + /* For now we will simply ignore unaligned memory regions */ > > > + if ((ram_offset | size) & (rb_page_size - 1)) > > > + continue; > > > + > > > + ram_block_discard_range(rb, ram_offset, size); > > > > I suspect this needs to do like the migration type of > > hinting and get disabled if page poisoning is in effect. > > Right? > > Shouldn't something like that end up getting handled via > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases > like that would end up setting qemu_balloon_is_inhibited to true, if that > isn't the case then I could add some additional conditions. I would do it > in about the same spot as the qemu_balloon_is_inhibited check. Well qemu_balloon_is_inhibited is for the regular ballooning, mostly a work-around for limitations is host linux iommu APIs when it's used with VFIO.
On Wed, 2019-07-24 at 16:46 -0400, Michael S. Tsirkin wrote: > On Wed, Jul 24, 2019 at 01:18:00PM -0700, Alexander Duyck wrote: > > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: > > > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > > Add support for what I am referring to as "bubble hinting". Basically the > > > > idea is to function very similar to how the balloon works in that we > > > > basically end up madvising the page as not being used. However we don't > > > > really need to bother with any deflate type logic since the page will be > > > > faulted back into the guest when it is read or written to. > > > > > > > > This is meant to be a simplification of the existing balloon interface > > > > to use for providing hints to what memory needs to be freed. I am assuming > > > > this is safe to do as the deflate logic does not actually appear to do very > > > > much other than tracking what subpages have been released and which ones > > > > haven't. > > > > > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > --- > > > > hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > > > > include/hw/virtio/virtio-balloon.h | 2 + > > > > include/standard-headers/linux/virtio_balloon.h | 1 + > > > > 3 files changed, 42 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > > index 2112874055fb..70c0004c0f88 100644 > > > > --- a/hw/virtio/virtio-balloon.c > > > > +++ b/hw/virtio/virtio-balloon.c > > > > @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > > > > balloon_stats_change_timer(s, 0); > > > > } > > > > > > > > +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > > +{ > > > > + VirtQueueElement *elem; > > > > + > > > > + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > > > > + unsigned int i; > > > > + > > > > + for (i = 0; i < elem->in_num; i++) { > > > > + void *addr = elem->in_sg[i].iov_base; > > > > + size_t size = elem->in_sg[i].iov_len; > > > > + ram_addr_t ram_offset; > > > > + size_t rb_page_size; > > > > + RAMBlock *rb; > > > > + > > > > + if (qemu_balloon_is_inhibited()) > > > > + continue; > > > > + > > > > + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > > > > + rb_page_size = qemu_ram_pagesize(rb); > > > > + > > > > + /* For now we will simply ignore unaligned memory regions */ > > > > + if ((ram_offset | size) & (rb_page_size - 1)) > > > > + continue; > > > > + > > > > + ram_block_discard_range(rb, ram_offset, size); > > > > > > I suspect this needs to do like the migration type of > > > hinting and get disabled if page poisoning is in effect. > > > Right? > > > > Shouldn't something like that end up getting handled via > > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases > > like that would end up setting qemu_balloon_is_inhibited to true, if that > > isn't the case then I could add some additional conditions. I would do it > > in about the same spot as the qemu_balloon_is_inhibited check. > > Well qemu_balloon_is_inhibited is for the regular ballooning, > mostly a work-around for limitations is host linux iommu > APIs when it's used with VFIO. I understood that. However it also addresses the shared memory case as well if I recall correctly. Basically any case where us discarding the page could cause issues we should be causing that function to return true.
On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > Add support for what I am referring to as "bubble hinting". Basically the > idea is to function very similar to how the balloon works in that we > basically end up madvising the page as not being used. However we don't > really need to bother with any deflate type logic since the page will be > faulted back into the guest when it is read or written to. > > This is meant to be a simplification of the existing balloon interface > to use for providing hints to what memory needs to be freed. I am assuming > this is safe to do as the deflate logic does not actually appear to do very > much other than tracking what subpages have been released and which ones > haven't. > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> BTW I wonder about migration here. When we migrate we lose all hints right? Well destination could be smarter, detect that page is full of 0s and just map a zero page. Then we don't need a hint as such - but I don't think it's done like that ATM. I also wonder about interaction with deflate. ATM deflate will add pages to the free list, then balloon will come right back and report them as free. > --- > hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > include/hw/virtio/virtio-balloon.h | 2 + > include/standard-headers/linux/virtio_balloon.h | 1 + > 3 files changed, 42 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index 2112874055fb..70c0004c0f88 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > balloon_stats_change_timer(s, 0); > } > > +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > +{ > + VirtQueueElement *elem; > + > + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > + unsigned int i; > + > + for (i = 0; i < elem->in_num; i++) { > + void *addr = elem->in_sg[i].iov_base; > + size_t size = elem->in_sg[i].iov_len; > + ram_addr_t ram_offset; > + size_t rb_page_size; > + RAMBlock *rb; > + > + if (qemu_balloon_is_inhibited()) > + continue; > + > + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > + rb_page_size = qemu_ram_pagesize(rb); > + > + /* For now we will simply ignore unaligned memory regions */ > + if ((ram_offset | size) & (rb_page_size - 1)) > + continue; > + > + ram_block_discard_range(rb, ram_offset, size); > + } > + > + virtqueue_push(vq, elem, 0); > + virtio_notify(vdev, vq); > + g_free(elem); > + } > +} > + > static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > { > VirtIOBalloon *s = VIRTIO_BALLOON(vdev); > @@ -782,6 +815,11 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp) > s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats); > > if (virtio_has_feature(s->host_features, > + VIRTIO_BALLOON_F_HINTING)) { > + s->hvq = virtio_add_queue(vdev, 128, virtio_bubble_handle_output); > + } > + > + if (virtio_has_feature(s->host_features, > VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { > s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE, > virtio_balloon_handle_free_page_vq); > @@ -897,6 +935,8 @@ static Property virtio_balloon_properties[] = { > VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false), > DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features, > VIRTIO_BALLOON_F_FREE_PAGE_HINT, false), > + DEFINE_PROP_BIT("guest-page-hinting", VirtIOBalloon, host_features, > + VIRTIO_BALLOON_F_HINTING, true), > DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD, > IOThread *), > DEFINE_PROP_END_OF_LIST(), > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h > index 1afafb12f6bc..a58b24fdf29d 100644 > --- a/include/hw/virtio/virtio-balloon.h > +++ b/include/hw/virtio/virtio-balloon.h > @@ -44,7 +44,7 @@ enum virtio_balloon_free_page_report_status { > > typedef struct VirtIOBalloon { > VirtIODevice parent_obj; > - VirtQueue *ivq, *dvq, *svq, *free_page_vq; > + VirtQueue *ivq, *dvq, *svq, *free_page_vq, *hvq; > uint32_t free_page_report_status; > uint32_t num_pages; > uint32_t actual; > diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h > index 9375ca2a70de..f9e3e8256261 100644 > --- a/include/standard-headers/linux/virtio_balloon.h > +++ b/include/standard-headers/linux/virtio_balloon.h > @@ -36,6 +36,7 @@ > #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */ > #define VIRTIO_BALLOON_F_FREE_PAGE_HINT 3 /* VQ to report free pages */ > #define VIRTIO_BALLOON_F_PAGE_POISON 4 /* Guest is using page poisoning */ > +#define VIRTIO_BALLOON_F_HINTING 5 /* Page hinting virtqueue */ > > /* Size of a PFN in the balloon interface. */ > #define VIRTIO_BALLOON_PFN_SHIFT 12
On Wed, 2019-07-24 at 17:38 -0400, Michael S. Tsirkin wrote: > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > Add support for what I am referring to as "bubble hinting". Basically the > > idea is to function very similar to how the balloon works in that we > > basically end up madvising the page as not being used. However we don't > > really need to bother with any deflate type logic since the page will be > > faulted back into the guest when it is read or written to. > > > > This is meant to be a simplification of the existing balloon interface > > to use for providing hints to what memory needs to be freed. I am assuming > > this is safe to do as the deflate logic does not actually appear to do very > > much other than tracking what subpages have been released and which ones > > haven't. > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > BTW I wonder about migration here. When we migrate we lose all hints > right? Well destination could be smarter, detect that page is full of > 0s and just map a zero page. Then we don't need a hint as such - but I > don't think it's done like that ATM. I was wondering about that a bit myself. If you migrate with a balloon active what currently happens with the pages in the balloon? Do you actually migrate them, or do you ignore them and just assume a zero page? I'm just reusing the ram_block_discard_range logic that was being used for the balloon inflation so I would assume the behavior would be the same. > I also wonder about interaction with deflate. ATM deflate will add > pages to the free list, then balloon will come right back and report > them as free. I don't know how likely it is that somebody who is getting the free page reporting is likely to want to also use the balloon to take up memory. However hinting on a page that came out of deflate might make sense when you consider that the balloon operates on 4K pages and the hints are on 2M pages. You are likely going to lose track of it all anyway as you have to work to merge the 4K pages up to the higher order page.
On Wed, Jul 24, 2019 at 03:03:56PM -0700, Alexander Duyck wrote: > On Wed, 2019-07-24 at 17:38 -0400, Michael S. Tsirkin wrote: > > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > Add support for what I am referring to as "bubble hinting". Basically the > > > idea is to function very similar to how the balloon works in that we > > > basically end up madvising the page as not being used. However we don't > > > really need to bother with any deflate type logic since the page will be > > > faulted back into the guest when it is read or written to. > > > > > > This is meant to be a simplification of the existing balloon interface > > > to use for providing hints to what memory needs to be freed. I am assuming > > > this is safe to do as the deflate logic does not actually appear to do very > > > much other than tracking what subpages have been released and which ones > > > haven't. > > > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > BTW I wonder about migration here. When we migrate we lose all hints > > right? Well destination could be smarter, detect that page is full of > > 0s and just map a zero page. Then we don't need a hint as such - but I > > don't think it's done like that ATM. > > I was wondering about that a bit myself. If you migrate with a balloon > active what currently happens with the pages in the balloon? Do you > actually migrate them, or do you ignore them and just assume a zero page? Ignore and assume zero page. > I'm just reusing the ram_block_discard_range logic that was being used for > the balloon inflation so I would assume the behavior would be the same. > > > I also wonder about interaction with deflate. ATM deflate will add > > pages to the free list, then balloon will come right back and report > > them as free. > > I don't know how likely it is that somebody who is getting the free page > reporting is likely to want to also use the balloon to take up memory. Why not? > However hinting on a page that came out of deflate might make sense when > you consider that the balloon operates on 4K pages and the hints are on 2M > pages. You are likely going to lose track of it all anyway as you have to > work to merge the 4K pages up to the higher order page. Right - we need to fix inflate/deflate anyway. When we do, we can do whatever :)
On Wed, 2019-07-24 at 18:08 -0400, Michael S. Tsirkin wrote: > On Wed, Jul 24, 2019 at 03:03:56PM -0700, Alexander Duyck wrote: > > On Wed, 2019-07-24 at 17:38 -0400, Michael S. Tsirkin wrote: > > > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > > Add support for what I am referring to as "bubble hinting". Basically the > > > > idea is to function very similar to how the balloon works in that we > > > > basically end up madvising the page as not being used. However we don't > > > > really need to bother with any deflate type logic since the page will be > > > > faulted back into the guest when it is read or written to. > > > > > > > > This is meant to be a simplification of the existing balloon interface > > > > to use for providing hints to what memory needs to be freed. I am assuming > > > > this is safe to do as the deflate logic does not actually appear to do very > > > > much other than tracking what subpages have been released and which ones > > > > haven't. > > > > > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > BTW I wonder about migration here. When we migrate we lose all hints > > > right? Well destination could be smarter, detect that page is full of > > > 0s and just map a zero page. Then we don't need a hint as such - but I > > > don't think it's done like that ATM. > > > > I was wondering about that a bit myself. If you migrate with a balloon > > active what currently happens with the pages in the balloon? Do you > > actually migrate them, or do you ignore them and just assume a zero page? > > Ignore and assume zero page. > > > I'm just reusing the ram_block_discard_range logic that was being used for > > the balloon inflation so I would assume the behavior would be the same. > > > > > I also wonder about interaction with deflate. ATM deflate will add > > > pages to the free list, then balloon will come right back and report > > > them as free. > > > > I don't know how likely it is that somebody who is getting the free page > > reporting is likely to want to also use the balloon to take up memory. > > Why not? The two functions are essentially doing the same thing. The only real difference is enforcement. If the balloon takes the pages the guest cannot get them back. I suppose there might be some advantage if you are wanting for force shrink a guest but that would be about it. > > However hinting on a page that came out of deflate might make sense when > > you consider that the balloon operates on 4K pages and the hints are on 2M > > pages. You are likely going to lose track of it all anyway as you have to > > work to merge the 4K pages up to the higher order page. > > Right - we need to fix inflate/deflate anyway. > When we do, we can do whatever :) One thing we could probably look at for the future would be to more closely merge the balloon and this reporting logic. Ideally the balloon would grab pages that were already hinted in order to enforce a certain size limit on the guest, and then when it gave the pages back they would retain their hinted status if possible. The only problem is that right now both of those require that hinting/reporting be active for the zone being accessed since we otherwise don't have pointers to the pages at the head of the "hinted" list.
On Wed, Jul 24, 2019 at 03:27:37PM -0700, Alexander Duyck wrote: > On Wed, 2019-07-24 at 18:08 -0400, Michael S. Tsirkin wrote: > > On Wed, Jul 24, 2019 at 03:03:56PM -0700, Alexander Duyck wrote: > > > On Wed, 2019-07-24 at 17:38 -0400, Michael S. Tsirkin wrote: > > > > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > > > > Add support for what I am referring to as "bubble hinting". Basically the > > > > > idea is to function very similar to how the balloon works in that we > > > > > basically end up madvising the page as not being used. However we don't > > > > > really need to bother with any deflate type logic since the page will be > > > > > faulted back into the guest when it is read or written to. > > > > > > > > > > This is meant to be a simplification of the existing balloon interface > > > > > to use for providing hints to what memory needs to be freed. I am assuming > > > > > this is safe to do as the deflate logic does not actually appear to do very > > > > > much other than tracking what subpages have been released and which ones > > > > > haven't. > > > > > > > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > > BTW I wonder about migration here. When we migrate we lose all hints > > > > right? Well destination could be smarter, detect that page is full of > > > > 0s and just map a zero page. Then we don't need a hint as such - but I > > > > don't think it's done like that ATM. > > > > > > I was wondering about that a bit myself. If you migrate with a balloon > > > active what currently happens with the pages in the balloon? Do you > > > actually migrate them, or do you ignore them and just assume a zero page? > > > > Ignore and assume zero page. > > > > > I'm just reusing the ram_block_discard_range logic that was being used for > > > the balloon inflation so I would assume the behavior would be the same. > > > > > > > I also wonder about interaction with deflate. ATM deflate will add > > > > pages to the free list, then balloon will come right back and report > > > > them as free. > > > > > > I don't know how likely it is that somebody who is getting the free page > > > reporting is likely to want to also use the balloon to take up memory. > > > > Why not? > > The two functions are essentially doing the same thing. The only real > difference is enforcement. If the balloon takes the pages the guest cannot > get them back. I suppose there might be some advantage if you are wanting > for force shrink a guest but that would be about it. Yea, that's a common use of the balloon ATM. Helps partition the host so guests don't conflict. OTOH deflate on oom thing probably will never be used with hinting. > > > However hinting on a page that came out of deflate might make sense when > > > you consider that the balloon operates on 4K pages and the hints are on 2M > > > pages. You are likely going to lose track of it all anyway as you have to > > > work to merge the 4K pages up to the higher order page. > > > > Right - we need to fix inflate/deflate anyway. > > When we do, we can do whatever :) > > One thing we could probably look at for the future would be to more > closely merge the balloon and this reporting logic. Ideally the balloon > would grab pages that were already hinted in order to enforce a certain > size limit on the guest, and then when it gave the pages back they would > retain their hinted status if possible. > > The only problem is that right now both of those require that > hinting/reporting be active for the zone being accessed since we otherwise > don't have pointers to the pages at the head of the "hinted" list. I guess I was talking about reworking host/guest ABI, you were talking about the internals. So both need to change :)
On 7/24/19 6:03 PM, Alexander Duyck wrote: > On Wed, 2019-07-24 at 17:38 -0400, Michael S. Tsirkin wrote: >> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> >>> >>> Add support for what I am referring to as "bubble hinting". Basically the >>> idea is to function very similar to how the balloon works in that we >>> basically end up madvising the page as not being used. However we don't >>> really need to bother with any deflate type logic since the page will be >>> faulted back into the guest when it is read or written to. >>> >>> This is meant to be a simplification of the existing balloon interface >>> to use for providing hints to what memory needs to be freed. I am assuming >>> this is safe to do as the deflate logic does not actually appear to do very >>> much other than tracking what subpages have been released and which ones >>> haven't. >>> >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> >> BTW I wonder about migration here. When we migrate we lose all hints >> right? Well destination could be smarter, detect that page is full of >> 0s and just map a zero page. Then we don't need a hint as such - but I >> don't think it's done like that ATM. > I was wondering about that a bit myself. If you migrate with a balloon > active what currently happens with the pages in the balloon? Do you > actually migrate them, or do you ignore them and just assume a zero page? > I'm just reusing the ram_block_discard_range logic that was being used for > the balloon inflation so I would assume the behavior would be the same. I agree, however, I think it is worth investigating to see if enabling hinting adds some sort of overhead specifically in this kind of scenarios. What do you think? >> I also wonder about interaction with deflate. ATM deflate will add >> pages to the free list, then balloon will come right back and report >> them as free. > I don't know how likely it is that somebody who is getting the free page > reporting is likely to want to also use the balloon to take up memory. I think it is possible. There are two possibilities: 1. User has a workload running, which is allocating and freeing the pages and at the same time, user deflates. If these new pages get used by this workload, we don't have to worry as you are already handling that by not hinting the free pages immediately. 2. Guest is idle and the user adds up some memory, for this situation what you have explained below does seems reasonable. > However hinting on a page that came out of deflate might make sense when > you consider that the balloon operates on 4K pages and the hints are on 2M > pages. You are likely going to lose track of it all anyway as you have to > work to merge the 4K pages up to the higher order page. >
On 7/24/19 4:18 PM, Alexander Duyck wrote: > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: >> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> >>> >>> Add support for what I am referring to as "bubble hinting". Basically the >>> idea is to function very similar to how the balloon works in that we >>> basically end up madvising the page as not being used. However we don't >>> really need to bother with any deflate type logic since the page will be >>> faulted back into the guest when it is read or written to. >>> >>> This is meant to be a simplification of the existing balloon interface >>> to use for providing hints to what memory needs to be freed. I am assuming >>> this is safe to do as the deflate logic does not actually appear to do very >>> much other than tracking what subpages have been released and which ones >>> haven't. >>> >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> >>> --- >>> hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ >>> include/hw/virtio/virtio-balloon.h | 2 + >>> include/standard-headers/linux/virtio_balloon.h | 1 + >>> 3 files changed, 42 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c >>> index 2112874055fb..70c0004c0f88 100644 >>> --- a/hw/virtio/virtio-balloon.c >>> +++ b/hw/virtio/virtio-balloon.c >>> @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, >>> balloon_stats_change_timer(s, 0); >>> } >>> >>> +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) >>> +{ >>> + VirtQueueElement *elem; >>> + >>> + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { >>> + unsigned int i; >>> + >>> + for (i = 0; i < elem->in_num; i++) { >>> + void *addr = elem->in_sg[i].iov_base; >>> + size_t size = elem->in_sg[i].iov_len; >>> + ram_addr_t ram_offset; >>> + size_t rb_page_size; >>> + RAMBlock *rb; >>> + >>> + if (qemu_balloon_is_inhibited()) >>> + continue; >>> + >>> + rb = qemu_ram_block_from_host(addr, false, &ram_offset); >>> + rb_page_size = qemu_ram_pagesize(rb); >>> + >>> + /* For now we will simply ignore unaligned memory regions */ >>> + if ((ram_offset | size) & (rb_page_size - 1)) >>> + continue; >>> + >>> + ram_block_discard_range(rb, ram_offset, size); >> I suspect this needs to do like the migration type of >> hinting and get disabled if page poisoning is in effect. >> Right? > Shouldn't something like that end up getting handled via > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases > like that would end up setting qemu_balloon_is_inhibited to true, if that > isn't the case then I could add some additional conditions. I would do it > in about the same spot as the qemu_balloon_is_inhibited check. Just wondering if you have tried running these patches in an environment with directly assigned devices? Ideally, I would expect qemu_balloon_is_inhibited() to take care of it. > >
On Thu, 2019-07-25 at 07:57 -0400, Nitesh Narayan Lal wrote: > On 7/24/19 4:18 PM, Alexander Duyck wrote: > > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: > > > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > > Add support for what I am referring to as "bubble hinting". Basically the > > > > idea is to function very similar to how the balloon works in that we > > > > basically end up madvising the page as not being used. However we don't > > > > really need to bother with any deflate type logic since the page will be > > > > faulted back into the guest when it is read or written to. > > > > > > > > This is meant to be a simplification of the existing balloon interface > > > > to use for providing hints to what memory needs to be freed. I am assuming > > > > this is safe to do as the deflate logic does not actually appear to do very > > > > much other than tracking what subpages have been released and which ones > > > > haven't. > > > > > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > --- > > > > hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > > > > include/hw/virtio/virtio-balloon.h | 2 + > > > > include/standard-headers/linux/virtio_balloon.h | 1 + > > > > 3 files changed, 42 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > > index 2112874055fb..70c0004c0f88 100644 > > > > --- a/hw/virtio/virtio-balloon.c > > > > +++ b/hw/virtio/virtio-balloon.c > > > > @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > > > > balloon_stats_change_timer(s, 0); > > > > } > > > > > > > > +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > > +{ > > > > + VirtQueueElement *elem; > > > > + > > > > + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > > > > + unsigned int i; > > > > + > > > > + for (i = 0; i < elem->in_num; i++) { > > > > + void *addr = elem->in_sg[i].iov_base; > > > > + size_t size = elem->in_sg[i].iov_len; > > > > + ram_addr_t ram_offset; > > > > + size_t rb_page_size; > > > > + RAMBlock *rb; > > > > + > > > > + if (qemu_balloon_is_inhibited()) > > > > + continue; > > > > + > > > > + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > > > > + rb_page_size = qemu_ram_pagesize(rb); > > > > + > > > > + /* For now we will simply ignore unaligned memory regions */ > > > > + if ((ram_offset | size) & (rb_page_size - 1)) > > > > + continue; > > > > + > > > > + ram_block_discard_range(rb, ram_offset, size); > > > I suspect this needs to do like the migration type of > > > hinting and get disabled if page poisoning is in effect. > > > Right? > > Shouldn't something like that end up getting handled via > > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases > > like that would end up setting qemu_balloon_is_inhibited to true, if that > > isn't the case then I could add some additional conditions. I would do it > > in about the same spot as the qemu_balloon_is_inhibited check. > > Just wondering if you have tried running these patches in an environment with > directly assigned devices? Ideally, I would expect qemu_balloon_is_inhibited() > to take care of it. Yes, I have run that as a test to actually benchmark the effect of things without the madvise bits since it essentially disables the hinting in the hypervisor but not the guest.
On Thu, 2019-07-25 at 07:35 -0400, Nitesh Narayan Lal wrote: > On 7/24/19 6:03 PM, Alexander Duyck wrote: > > On Wed, 2019-07-24 at 17:38 -0400, Michael S. Tsirkin wrote: > > > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > > Add support for what I am referring to as "bubble hinting". Basically the > > > > idea is to function very similar to how the balloon works in that we > > > > basically end up madvising the page as not being used. However we don't > > > > really need to bother with any deflate type logic since the page will be > > > > faulted back into the guest when it is read or written to. > > > > > > > > This is meant to be a simplification of the existing balloon interface > > > > to use for providing hints to what memory needs to be freed. I am assuming > > > > this is safe to do as the deflate logic does not actually appear to do very > > > > much other than tracking what subpages have been released and which ones > > > > haven't. > > > > > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > BTW I wonder about migration here. When we migrate we lose all hints > > > right? Well destination could be smarter, detect that page is full of > > > 0s and just map a zero page. Then we don't need a hint as such - but I > > > don't think it's done like that ATM. > > I was wondering about that a bit myself. If you migrate with a balloon > > active what currently happens with the pages in the balloon? Do you > > actually migrate them, or do you ignore them and just assume a zero page? > > I'm just reusing the ram_block_discard_range logic that was being used for > > the balloon inflation so I would assume the behavior would be the same. > I agree, however, I think it is worth investigating to see if enabling hinting > adds some sort of overhead specifically in this kind of scenarios. What do you > think? I suspect that the hinting/reporting would probably improve migration times based on the fact that from the sound of things it would just be migrated as a zero page. I don't have a good setup for testing migration though and I am not that familiar with trying to do a live migration. That is one of the reasons why I didn't want to stray too far from the existing balloon code as that has already been tested with migration so I would assume as long as I am doing almost the exact same thing to hint the pages away it should behave exactly the same. > > > I also wonder about interaction with deflate. ATM deflate will add > > > pages to the free list, then balloon will come right back and report > > > them as free. > > I don't know how likely it is that somebody who is getting the free page > > reporting is likely to want to also use the balloon to take up memory. > I think it is possible. There are two possibilities: > 1. User has a workload running, which is allocating and freeing the pages and at > the same time, user deflates. > If these new pages get used by this workload, we don't have to worry as you are > already handling that by not hinting the free pages immediately. > 2. Guest is idle and the user adds up some memory, for this situation what you > have explained below does seems reasonable. Us hinting on pages that are freed up via deflate wouldn't be too big of a deal. I would think that is something we could look at addressing as more of a follow-on if we ever needed to since it would just add more complexity. Really what I would like to see is the balloon itself get updated first to perhaps work with variable sized pages first so that we could then have pages come directly out of the balloon and go back into the freelist as hinted, or visa-versa where hinted pages could be pulled directly into the balloon without needing to notify the host.
On Thu, Jul 25, 2019 at 08:05:30AM -0700, Alexander Duyck wrote: > On Thu, 2019-07-25 at 07:35 -0400, Nitesh Narayan Lal wrote: > > On 7/24/19 6:03 PM, Alexander Duyck wrote: > > > On Wed, 2019-07-24 at 17:38 -0400, Michael S. Tsirkin wrote: > > > > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > > > > Add support for what I am referring to as "bubble hinting". Basically the > > > > > idea is to function very similar to how the balloon works in that we > > > > > basically end up madvising the page as not being used. However we don't > > > > > really need to bother with any deflate type logic since the page will be > > > > > faulted back into the guest when it is read or written to. > > > > > > > > > > This is meant to be a simplification of the existing balloon interface > > > > > to use for providing hints to what memory needs to be freed. I am assuming > > > > > this is safe to do as the deflate logic does not actually appear to do very > > > > > much other than tracking what subpages have been released and which ones > > > > > haven't. > > > > > > > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > BTW I wonder about migration here. When we migrate we lose all hints > > > > right? Well destination could be smarter, detect that page is full of > > > > 0s and just map a zero page. Then we don't need a hint as such - but I > > > > don't think it's done like that ATM. > > > I was wondering about that a bit myself. If you migrate with a balloon > > > active what currently happens with the pages in the balloon? Do you > > > actually migrate them, or do you ignore them and just assume a zero page? > > > I'm just reusing the ram_block_discard_range logic that was being used for > > > the balloon inflation so I would assume the behavior would be the same. > > I agree, however, I think it is worth investigating to see if enabling hinting > > adds some sort of overhead specifically in this kind of scenarios. What do you > > think? > > I suspect that the hinting/reporting would probably improve migration > times based on the fact that from the sound of things it would just be > migrated as a zero page. > > I don't have a good setup for testing migration though and I am not that > familiar with trying to do a live migration. That is one of the reasons > why I didn't want to stray too far from the existing balloon code as that > has already been tested with migration so I would assume as long as I am > doing almost the exact same thing to hint the pages away it should behave > exactly the same. > > > > > I also wonder about interaction with deflate. ATM deflate will add > > > > pages to the free list, then balloon will come right back and report > > > > them as free. > > > I don't know how likely it is that somebody who is getting the free page > > > reporting is likely to want to also use the balloon to take up memory. > > I think it is possible. There are two possibilities: > > 1. User has a workload running, which is allocating and freeing the pages and at > > the same time, user deflates. > > If these new pages get used by this workload, we don't have to worry as you are > > already handling that by not hinting the free pages immediately. > > 2. Guest is idle and the user adds up some memory, for this situation what you > > have explained below does seems reasonable. > > Us hinting on pages that are freed up via deflate wouldn't be too big of a > deal. I would think that is something we could look at addressing as more > of a follow-on if we ever needed to since it would just add more > complexity. > > Really what I would like to see is the balloon itself get updated first to > perhaps work with variable sized pages first so that we could then have > pages come directly out of the balloon and go back into the freelist as > hinted, or visa-versa where hinted pages could be pulled directly into the > balloon without needing to notify the host. Right, I agree. At this point the main thing I worry about is that the interfaces only support one reporter, since a page flag is used. So if we ever rewrite existing hinting to use the new mm infrastructure then we can't e.g. enable both types of hinting. FWIW Nitesh's RFC does not have this limitation. I intend to think about this over the weekend.
On Thu, 2019-07-25 at 11:16 -0400, Michael S. Tsirkin wrote: > On Thu, Jul 25, 2019 at 08:05:30AM -0700, Alexander Duyck wrote: > > On Thu, 2019-07-25 at 07:35 -0400, Nitesh Narayan Lal wrote: > > > On 7/24/19 6:03 PM, Alexander Duyck wrote: > > > > On Wed, 2019-07-24 at 17:38 -0400, Michael S. Tsirkin wrote: > > > > > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > > > > > > Add support for what I am referring to as "bubble hinting". Basically the > > > > > > idea is to function very similar to how the balloon works in that we > > > > > > basically end up madvising the page as not being used. However we don't > > > > > > really need to bother with any deflate type logic since the page will be > > > > > > faulted back into the guest when it is read or written to. > > > > > > > > > > > > This is meant to be a simplification of the existing balloon interface > > > > > > to use for providing hints to what memory needs to be freed. I am assuming > > > > > > this is safe to do as the deflate logic does not actually appear to do very > > > > > > much other than tracking what subpages have been released and which ones > > > > > > haven't. > > > > > > > > > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > BTW I wonder about migration here. When we migrate we lose all hints > > > > > right? Well destination could be smarter, detect that page is full of > > > > > 0s and just map a zero page. Then we don't need a hint as such - but I > > > > > don't think it's done like that ATM. > > > > I was wondering about that a bit myself. If you migrate with a balloon > > > > active what currently happens with the pages in the balloon? Do you > > > > actually migrate them, or do you ignore them and just assume a zero page? > > > > I'm just reusing the ram_block_discard_range logic that was being used for > > > > the balloon inflation so I would assume the behavior would be the same. > > > I agree, however, I think it is worth investigating to see if enabling hinting > > > adds some sort of overhead specifically in this kind of scenarios. What do you > > > think? > > > > I suspect that the hinting/reporting would probably improve migration > > times based on the fact that from the sound of things it would just be > > migrated as a zero page. > > > > I don't have a good setup for testing migration though and I am not that > > familiar with trying to do a live migration. That is one of the reasons > > why I didn't want to stray too far from the existing balloon code as that > > has already been tested with migration so I would assume as long as I am > > doing almost the exact same thing to hint the pages away it should behave > > exactly the same. > > > > > > > I also wonder about interaction with deflate. ATM deflate will add > > > > > pages to the free list, then balloon will come right back and report > > > > > them as free. > > > > I don't know how likely it is that somebody who is getting the free page > > > > reporting is likely to want to also use the balloon to take up memory. > > > I think it is possible. There are two possibilities: > > > 1. User has a workload running, which is allocating and freeing the pages and at > > > the same time, user deflates. > > > If these new pages get used by this workload, we don't have to worry as you are > > > already handling that by not hinting the free pages immediately. > > > 2. Guest is idle and the user adds up some memory, for this situation what you > > > have explained below does seems reasonable. > > > > Us hinting on pages that are freed up via deflate wouldn't be too big of a > > deal. I would think that is something we could look at addressing as more > > of a follow-on if we ever needed to since it would just add more > > complexity. > > > > Really what I would like to see is the balloon itself get updated first to > > perhaps work with variable sized pages first so that we could then have > > pages come directly out of the balloon and go back into the freelist as > > hinted, or visa-versa where hinted pages could be pulled directly into the > > balloon without needing to notify the host. > > Right, I agree. At this point the main thing I worry about is that > the interfaces only support one reporter, since a page flag is used. > So if we ever rewrite existing hinting to use the new mm > infrastructure then we can't e.g. enable both types of hinting. Does it make sense to have multiple types of hinting active at the same time though? That kind of seems wasteful to me. Ideally we should be able to provide the hints and have them feed whatever is supposed to be using them. So for example I could probably look at also clearing the bitmaps when migration is in process. Also, I am wonder if the free page hints would be redundant with the form of page hinting/reporting that I have since we should be migrating a much smaller footprint anyway if the pages have been madvised away before we even start the migration. > FWIW Nitesh's RFC does not have this limitation. Yes, but there are also limitations to his approach. For example the fact that the bitmap it maintains is back to being a hint rather then being very exact. As a result you could end up walking the bitmap for a while clearing bits without ever finding a free page. > I intend to think about this over the weekend. Sounds good. I'll try to get the stuff you have pointed out so far addressed and hopefully have v3 ready to go next week. Thanks. - Alex
On Thu, Jul 25, 2019 at 09:16:21AM -0700, Alexander Duyck wrote: > On Thu, 2019-07-25 at 11:16 -0400, Michael S. Tsirkin wrote: > > On Thu, Jul 25, 2019 at 08:05:30AM -0700, Alexander Duyck wrote: > > > On Thu, 2019-07-25 at 07:35 -0400, Nitesh Narayan Lal wrote: > > > > On 7/24/19 6:03 PM, Alexander Duyck wrote: > > > > > On Wed, 2019-07-24 at 17:38 -0400, Michael S. Tsirkin wrote: > > > > > > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > > > > > > > > Add support for what I am referring to as "bubble hinting". Basically the > > > > > > > idea is to function very similar to how the balloon works in that we > > > > > > > basically end up madvising the page as not being used. However we don't > > > > > > > really need to bother with any deflate type logic since the page will be > > > > > > > faulted back into the guest when it is read or written to. > > > > > > > > > > > > > > This is meant to be a simplification of the existing balloon interface > > > > > > > to use for providing hints to what memory needs to be freed. I am assuming > > > > > > > this is safe to do as the deflate logic does not actually appear to do very > > > > > > > much other than tracking what subpages have been released and which ones > > > > > > > haven't. > > > > > > > > > > > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > BTW I wonder about migration here. When we migrate we lose all hints > > > > > > right? Well destination could be smarter, detect that page is full of > > > > > > 0s and just map a zero page. Then we don't need a hint as such - but I > > > > > > don't think it's done like that ATM. > > > > > I was wondering about that a bit myself. If you migrate with a balloon > > > > > active what currently happens with the pages in the balloon? Do you > > > > > actually migrate them, or do you ignore them and just assume a zero page? > > > > > I'm just reusing the ram_block_discard_range logic that was being used for > > > > > the balloon inflation so I would assume the behavior would be the same. > > > > I agree, however, I think it is worth investigating to see if enabling hinting > > > > adds some sort of overhead specifically in this kind of scenarios. What do you > > > > think? > > > > > > I suspect that the hinting/reporting would probably improve migration > > > times based on the fact that from the sound of things it would just be > > > migrated as a zero page. > > > > > > I don't have a good setup for testing migration though and I am not that > > > familiar with trying to do a live migration. That is one of the reasons > > > why I didn't want to stray too far from the existing balloon code as that > > > has already been tested with migration so I would assume as long as I am > > > doing almost the exact same thing to hint the pages away it should behave > > > exactly the same. > > > > > > > > > I also wonder about interaction with deflate. ATM deflate will add > > > > > > pages to the free list, then balloon will come right back and report > > > > > > them as free. > > > > > I don't know how likely it is that somebody who is getting the free page > > > > > reporting is likely to want to also use the balloon to take up memory. > > > > I think it is possible. There are two possibilities: > > > > 1. User has a workload running, which is allocating and freeing the pages and at > > > > the same time, user deflates. > > > > If these new pages get used by this workload, we don't have to worry as you are > > > > already handling that by not hinting the free pages immediately. > > > > 2. Guest is idle and the user adds up some memory, for this situation what you > > > > have explained below does seems reasonable. > > > > > > Us hinting on pages that are freed up via deflate wouldn't be too big of a > > > deal. I would think that is something we could look at addressing as more > > > of a follow-on if we ever needed to since it would just add more > > > complexity. > > > > > > Really what I would like to see is the balloon itself get updated first to > > > perhaps work with variable sized pages first so that we could then have > > > pages come directly out of the balloon and go back into the freelist as > > > hinted, or visa-versa where hinted pages could be pulled directly into the > > > balloon without needing to notify the host. > > > > Right, I agree. At this point the main thing I worry about is that > > the interfaces only support one reporter, since a page flag is used. > > So if we ever rewrite existing hinting to use the new mm > > infrastructure then we can't e.g. enable both types of hinting. > > Does it make sense to have multiple types of hinting active at the same > time though? That kind of seems wasteful to me. Ideally we should be able > to provide the hints and have them feed whatever is supposed to be using > them. So for example I could probably look at also clearing the bitmaps > when migration is in process. > > Also, I am wonder if the free page hints would be redundant with the form > of page hinting/reporting that I have since we should be migrating a much > smaller footprint anyway if the pages have been madvised away before we > even start the migration. Good points. > > FWIW Nitesh's RFC does not have this limitation. > > Yes, but there are also limitations to his approach. For example the fact > that the bitmap it maintains is back to being a hint rather then being > very exact. As a result you could end up walking the bitmap for a while > clearing bits without ever finding a free page. For sure. > > I intend to think about this over the weekend. > > Sounds good. I'll try to get the stuff you have pointed out so far > addressed and hopefully have v3 ready to go next week. > > Thanks. > > - Alex
On 7/25/19 12:16 PM, Alexander Duyck wrote: > On Thu, 2019-07-25 at 11:16 -0400, Michael S. Tsirkin wrote: >> On Thu, Jul 25, 2019 at 08:05:30AM -0700, Alexander Duyck wrote: >>> On Thu, 2019-07-25 at 07:35 -0400, Nitesh Narayan Lal wrote: >>>> On 7/24/19 6:03 PM, Alexander Duyck wrote: >>>>> On Wed, 2019-07-24 at 17:38 -0400, Michael S. Tsirkin wrote: >>>>>> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: >>>>>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> >>>>>>> >>>>>>> Add support for what I am referring to as "bubble hinting". Basically the >>>>>>> idea is to function very similar to how the balloon works in that we >>>>>>> basically end up madvising the page as not being used. However we don't >>>>>>> really need to bother with any deflate type logic since the page will be >>>>>>> faulted back into the guest when it is read or written to. >>>>>>> >>>>>>> This is meant to be a simplification of the existing balloon interface >>>>>>> to use for providing hints to what memory needs to be freed. I am assuming >>>>>>> this is safe to do as the deflate logic does not actually appear to do very >>>>>>> much other than tracking what subpages have been released and which ones >>>>>>> haven't. >>>>>>> >>>>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> >>>>>> BTW I wonder about migration here. When we migrate we lose all hints >>>>>> right? Well destination could be smarter, detect that page is full of >>>>>> 0s and just map a zero page. Then we don't need a hint as such - but I >>>>>> don't think it's done like that ATM. >>>>> I was wondering about that a bit myself. If you migrate with a balloon >>>>> active what currently happens with the pages in the balloon? Do you >>>>> actually migrate them, or do you ignore them and just assume a zero page? >>>>> I'm just reusing the ram_block_discard_range logic that was being used for >>>>> the balloon inflation so I would assume the behavior would be the same. >>>> I agree, however, I think it is worth investigating to see if enabling hinting >>>> adds some sort of overhead specifically in this kind of scenarios. What do you >>>> think? >>> I suspect that the hinting/reporting would probably improve migration >>> times based on the fact that from the sound of things it would just be >>> migrated as a zero page. >>> >>> I don't have a good setup for testing migration though and I am not that >>> familiar with trying to do a live migration. That is one of the reasons >>> why I didn't want to stray too far from the existing balloon code as that >>> has already been tested with migration so I would assume as long as I am >>> doing almost the exact same thing to hint the pages away it should behave >>> exactly the same. >>> >>>>>> I also wonder about interaction with deflate. ATM deflate will add >>>>>> pages to the free list, then balloon will come right back and report >>>>>> them as free. >>>>> I don't know how likely it is that somebody who is getting the free page >>>>> reporting is likely to want to also use the balloon to take up memory. >>>> I think it is possible. There are two possibilities: >>>> 1. User has a workload running, which is allocating and freeing the pages and at >>>> the same time, user deflates. >>>> If these new pages get used by this workload, we don't have to worry as you are >>>> already handling that by not hinting the free pages immediately. >>>> 2. Guest is idle and the user adds up some memory, for this situation what you >>>> have explained below does seems reasonable. >>> Us hinting on pages that are freed up via deflate wouldn't be too big of a >>> deal. I would think that is something we could look at addressing as more >>> of a follow-on if we ever needed to since it would just add more >>> complexity. >>> >>> Really what I would like to see is the balloon itself get updated first to >>> perhaps work with variable sized pages first so that we could then have >>> pages come directly out of the balloon and go back into the freelist as >>> hinted, or visa-versa where hinted pages could be pulled directly into the >>> balloon without needing to notify the host. >> Right, I agree. At this point the main thing I worry about is that >> the interfaces only support one reporter, since a page flag is used. >> So if we ever rewrite existing hinting to use the new mm >> infrastructure then we can't e.g. enable both types of hinting. > Does it make sense to have multiple types of hinting active at the same > time though? That kind of seems wasteful to me. I agree. > Ideally we should be able > to provide the hints and have them feed whatever is supposed to be using > them. So for example I could probably look at also clearing the bitmaps > when migration is in process. > > Also, I am wonder if the free page hints would be redundant with the form > of page hinting/reporting that I have since we should be migrating a much > smaller footprint anyway if the pages have been madvised away before we > even start the migration. > >> FWIW Nitesh's RFC does not have this limitation. > Yes, but there are also limitations to his approach. For example the fact > that the bitmap it maintains is back to being a hint rather then being > very exact. True. > As a result you could end up walking the bitmap for a while > clearing bits without ever finding a free page. Are referring to the overhead which will be introduced due to bitmap scanning on very large guests? > >> I intend to think about this over the weekend. > Sounds good. I'll try to get the stuff you have pointed out so far > addressed and hopefully have v3 ready to go next week. > > Thanks. > > - Alex >
On Thu, 2019-07-25 at 14:25 -0400, Nitesh Narayan Lal wrote: > On 7/25/19 12:16 PM, Alexander Duyck wrote: > > On Thu, 2019-07-25 at 11:16 -0400, Michael S. Tsirkin wrote: > > > On Thu, Jul 25, 2019 at 08:05:30AM -0700, Alexander Duyck wrote: > > > > On Thu, 2019-07-25 at 07:35 -0400, Nitesh Narayan Lal wrote: > > > > > On 7/24/19 6:03 PM, Alexander Duyck wrote: > > > > > > On Wed, 2019-07-24 at 17:38 -0400, Michael S. Tsirkin wrote: > > > > > > > On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > > > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > > > > > > > > > <snip> > > Ideally we should be able > > to provide the hints and have them feed whatever is supposed to be using > > them. So for example I could probably look at also clearing the bitmaps > > when migration is in process. > > > > Also, I am wonder if the free page hints would be redundant with the form > > of page hinting/reporting that I have since we should be migrating a much > > smaller footprint anyway if the pages have been madvised away before we > > even start the migration. > > > > > FWIW Nitesh's RFC does not have this limitation. > > Yes, but there are also limitations to his approach. For example the fact > > that the bitmap it maintains is back to being a hint rather then being > > very exact. > > True. > > > > As a result you could end up walking the bitmap for a while > > clearing bits without ever finding a free page. > > Are referring to the overhead which will be introduced due to bitmap scanning on > very large guests? Yes. One concern I have had is that for large memory footprints the RFC would end up having a large number of false positives on an highly active system. I am worried it will result in a feedback loop where having more false hits slows down your processing speed, and the slower your processing speed the more likely you are to encounter more false hits.
On 7/25/19 4:00 PM, Alexander Duyck wrote: > On Thu, 2019-07-25 at 14:25 -0400, Nitesh Narayan Lal wrote: >> On 7/25/19 12:16 PM, Alexander Duyck wrote: >>> On Thu, 2019-07-25 at 11:16 -0400, Michael S. Tsirkin wrote: >>>> On Thu, Jul 25, 2019 at 08:05:30AM -0700, Alexander Duyck wrote: >>>>> On Thu, 2019-07-25 at 07:35 -0400, Nitesh Narayan Lal wrote: >>>>>> On 7/24/19 6:03 PM, Alexander Duyck wrote: >>>>>>> On Wed, 2019-07-24 at 17:38 -0400, Michael S. Tsirkin wrote: >>>>>>>> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: >>>>>>>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> >>>>>>>>> > <snip> > > >>> Ideally we should be able >>> to provide the hints and have them feed whatever is supposed to be using >>> them. So for example I could probably look at also clearing the bitmaps >>> when migration is in process. >>> >>> Also, I am wonder if the free page hints would be redundant with the form >>> of page hinting/reporting that I have since we should be migrating a much >>> smaller footprint anyway if the pages have been madvised away before we >>> even start the migration. >>> >>>> FWIW Nitesh's RFC does not have this limitation. >>> Yes, but there are also limitations to his approach. For example the fact >>> that the bitmap it maintains is back to being a hint rather then being >>> very exact. >> True. >> >> >>> As a result you could end up walking the bitmap for a while >>> clearing bits without ever finding a free page. >> Are referring to the overhead which will be introduced due to bitmap scanning on >> very large guests? > Yes. One concern I have had is that for large memory footprints the RFC > would end up having a large number of false positives on an highly active > system. I am worried it will result in a feedback loop where having more > false hits slows down your processing speed, and the slower your > processing speed the more likely you are to encounter more false hits. > > It is definitely an interesting thing to see, I intend to test such a scenario with large guest before my next posting.
On Wed, Jul 24, 2019 at 1:42 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Jul 24, 2019 at 04:29:27PM -0400, Nitesh Narayan Lal wrote: > > > > On 7/24/19 4:18 PM, Alexander Duyck wrote: > > > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: > > >> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > >>> > > >>> Add support for what I am referring to as "bubble hinting". Basically the > > >>> idea is to function very similar to how the balloon works in that we > > >>> basically end up madvising the page as not being used. However we don't > > >>> really need to bother with any deflate type logic since the page will be > > >>> faulted back into the guest when it is read or written to. > > >>> > > >>> This is meant to be a simplification of the existing balloon interface > > >>> to use for providing hints to what memory needs to be freed. I am assuming > > >>> this is safe to do as the deflate logic does not actually appear to do very > > >>> much other than tracking what subpages have been released and which ones > > >>> haven't. > > >>> > > >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > >>> --- > > >>> hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > > >>> include/hw/virtio/virtio-balloon.h | 2 + > > >>> include/standard-headers/linux/virtio_balloon.h | 1 + > > >>> 3 files changed, 42 insertions(+), 1 deletion(-) > > >>> > > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > >>> index 2112874055fb..70c0004c0f88 100644 > > >>> --- a/hw/virtio/virtio-balloon.c > > >>> +++ b/hw/virtio/virtio-balloon.c > > >>> @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > > >>> balloon_stats_change_timer(s, 0); > > >>> } > > >>> > > >>> +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > >>> +{ > > >>> + VirtQueueElement *elem; > > >>> + > > >>> + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > > >>> + unsigned int i; > > >>> + > > >>> + for (i = 0; i < elem->in_num; i++) { > > >>> + void *addr = elem->in_sg[i].iov_base; > > >>> + size_t size = elem->in_sg[i].iov_len; > > >>> + ram_addr_t ram_offset; > > >>> + size_t rb_page_size; > > >>> + RAMBlock *rb; > > >>> + > > >>> + if (qemu_balloon_is_inhibited()) > > >>> + continue; > > >>> + > > >>> + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > > >>> + rb_page_size = qemu_ram_pagesize(rb); > > >>> + > > >>> + /* For now we will simply ignore unaligned memory regions */ > > >>> + if ((ram_offset | size) & (rb_page_size - 1)) > > >>> + continue; > > >>> + > > >>> + ram_block_discard_range(rb, ram_offset, size); > > >> I suspect this needs to do like the migration type of > > >> hinting and get disabled if page poisoning is in effect. > > >> Right? > > > Shouldn't something like that end up getting handled via > > > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases > > > like that would end up setting qemu_balloon_is_inhibited to true, if that > > > isn't the case then I could add some additional conditions. I would do it > > > in about the same spot as the qemu_balloon_is_inhibited check. > > I don't think qemu_balloon_is_inhibited() will take care of the page poisoning > > situations. > > If I am not wrong we may have to look to extend VIRTIO_BALLOON_F_PAGE_POISON > > support as per Michael's suggestion. > > > BTW upstream qemu seems to ignore VIRTIO_BALLOON_F_PAGE_POISON ATM. > Which is probably a bug. > Wei, could you take a look pls? So I was looking at sorting out this for the unused page reporting that I am working on and it occurred to me that I don't think we can do the free page hinting if any sort of poison validation is present. The problem is that free page hinting simply stops the page from being migrated. As a result if there was stale data present it will just leave it there instead of zeroing it or writing it to alternating 1s and 0s. Also it looks like the VIRTIO_BALLOON_F_PAGE_POISON feature is assuming that 0 means that page poisoning is disabled, when in reality it might just mean we are using the value zero to poison pages instead of the 0xaa pattern. As such I think there are several cases where we could incorrectly flag the pages with the hint and result in the migrated guest reporting pages that contain non-poison values. The zero assumption works for unused page reporting since we will be zeroing out the page when it is faulted back into the guest, however the same doesn't work for the free page hint since it is simply skipping the migration of the recently dirtied page.
On Mon, Jul 29, 2019 at 09:58:04AM -0700, Alexander Duyck wrote: > On Wed, Jul 24, 2019 at 1:42 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Wed, Jul 24, 2019 at 04:29:27PM -0400, Nitesh Narayan Lal wrote: > > > > > > On 7/24/19 4:18 PM, Alexander Duyck wrote: > > > > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: > > > >> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > >>> > > > >>> Add support for what I am referring to as "bubble hinting". Basically the > > > >>> idea is to function very similar to how the balloon works in that we > > > >>> basically end up madvising the page as not being used. However we don't > > > >>> really need to bother with any deflate type logic since the page will be > > > >>> faulted back into the guest when it is read or written to. > > > >>> > > > >>> This is meant to be a simplification of the existing balloon interface > > > >>> to use for providing hints to what memory needs to be freed. I am assuming > > > >>> this is safe to do as the deflate logic does not actually appear to do very > > > >>> much other than tracking what subpages have been released and which ones > > > >>> haven't. > > > >>> > > > >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > >>> --- > > > >>> hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > > > >>> include/hw/virtio/virtio-balloon.h | 2 + > > > >>> include/standard-headers/linux/virtio_balloon.h | 1 + > > > >>> 3 files changed, 42 insertions(+), 1 deletion(-) > > > >>> > > > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > >>> index 2112874055fb..70c0004c0f88 100644 > > > >>> --- a/hw/virtio/virtio-balloon.c > > > >>> +++ b/hw/virtio/virtio-balloon.c > > > >>> @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > > > >>> balloon_stats_change_timer(s, 0); > > > >>> } > > > >>> > > > >>> +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > >>> +{ > > > >>> + VirtQueueElement *elem; > > > >>> + > > > >>> + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > > > >>> + unsigned int i; > > > >>> + > > > >>> + for (i = 0; i < elem->in_num; i++) { > > > >>> + void *addr = elem->in_sg[i].iov_base; > > > >>> + size_t size = elem->in_sg[i].iov_len; > > > >>> + ram_addr_t ram_offset; > > > >>> + size_t rb_page_size; > > > >>> + RAMBlock *rb; > > > >>> + > > > >>> + if (qemu_balloon_is_inhibited()) > > > >>> + continue; > > > >>> + > > > >>> + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > > > >>> + rb_page_size = qemu_ram_pagesize(rb); > > > >>> + > > > >>> + /* For now we will simply ignore unaligned memory regions */ > > > >>> + if ((ram_offset | size) & (rb_page_size - 1)) > > > >>> + continue; > > > >>> + > > > >>> + ram_block_discard_range(rb, ram_offset, size); > > > >> I suspect this needs to do like the migration type of > > > >> hinting and get disabled if page poisoning is in effect. > > > >> Right? > > > > Shouldn't something like that end up getting handled via > > > > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases > > > > like that would end up setting qemu_balloon_is_inhibited to true, if that > > > > isn't the case then I could add some additional conditions. I would do it > > > > in about the same spot as the qemu_balloon_is_inhibited check. > > > I don't think qemu_balloon_is_inhibited() will take care of the page poisoning > > > situations. > > > If I am not wrong we may have to look to extend VIRTIO_BALLOON_F_PAGE_POISON > > > support as per Michael's suggestion. > > > > > > BTW upstream qemu seems to ignore VIRTIO_BALLOON_F_PAGE_POISON ATM. > > Which is probably a bug. > > Wei, could you take a look pls? > > So I was looking at sorting out this for the unused page reporting > that I am working on and it occurred to me that I don't think we can > do the free page hinting if any sort of poison validation is present. > The problem is that free page hinting simply stops the page from being > migrated. As a result if there was stale data present it will just > leave it there instead of zeroing it or writing it to alternating 1s > and 0s. stale data where? on source or on destination? do you mean the case where memory was corrupted? > > Also it looks like the VIRTIO_BALLOON_F_PAGE_POISON feature is > assuming that 0 means that page poisoning is disabled, > when in reality > it might just mean we are using the value zero to poison pages instead > of the 0xaa pattern. As such I think there are several cases where we > could incorrectly flag the pages with the hint and result in the > migrated guest reporting pages that contain non-poison values. > Well guest has this code: static int virtballoon_validate(struct virtio_device *vdev) { if (!page_poisoning_enabled()) __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON); __virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM); return 0; } So it seems that host can figure out what is going on easily enough. What did I miss? > The zero assumption works for unused page reporting since we will be > zeroing out the page when it is faulted back into the guest, however > the same doesn't work for the free page hint since it is simply > skipping the migration of the recently dirtied page. Right but the dirtied page is normally full of 0 since that is the poison value, if we just leave it there we still get 0s, right?
On Mon, Jul 29, 2019 at 12:25 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Jul 29, 2019 at 09:58:04AM -0700, Alexander Duyck wrote: > > On Wed, Jul 24, 2019 at 1:42 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Wed, Jul 24, 2019 at 04:29:27PM -0400, Nitesh Narayan Lal wrote: > > > > > > > > On 7/24/19 4:18 PM, Alexander Duyck wrote: > > > > > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: > > > > >> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > >>> > > > > >>> Add support for what I am referring to as "bubble hinting". Basically the > > > > >>> idea is to function very similar to how the balloon works in that we > > > > >>> basically end up madvising the page as not being used. However we don't > > > > >>> really need to bother with any deflate type logic since the page will be > > > > >>> faulted back into the guest when it is read or written to. > > > > >>> > > > > >>> This is meant to be a simplification of the existing balloon interface > > > > >>> to use for providing hints to what memory needs to be freed. I am assuming > > > > >>> this is safe to do as the deflate logic does not actually appear to do very > > > > >>> much other than tracking what subpages have been released and which ones > > > > >>> haven't. > > > > >>> > > > > >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > >>> --- > > > > >>> hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > > > > >>> include/hw/virtio/virtio-balloon.h | 2 + > > > > >>> include/standard-headers/linux/virtio_balloon.h | 1 + > > > > >>> 3 files changed, 42 insertions(+), 1 deletion(-) > > > > >>> > > > > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > > >>> index 2112874055fb..70c0004c0f88 100644 > > > > >>> --- a/hw/virtio/virtio-balloon.c > > > > >>> +++ b/hw/virtio/virtio-balloon.c > > > > >>> @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > > > > >>> balloon_stats_change_timer(s, 0); > > > > >>> } > > > > >>> > > > > >>> +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > > >>> +{ > > > > >>> + VirtQueueElement *elem; > > > > >>> + > > > > >>> + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > > > > >>> + unsigned int i; > > > > >>> + > > > > >>> + for (i = 0; i < elem->in_num; i++) { > > > > >>> + void *addr = elem->in_sg[i].iov_base; > > > > >>> + size_t size = elem->in_sg[i].iov_len; > > > > >>> + ram_addr_t ram_offset; > > > > >>> + size_t rb_page_size; > > > > >>> + RAMBlock *rb; > > > > >>> + > > > > >>> + if (qemu_balloon_is_inhibited()) > > > > >>> + continue; > > > > >>> + > > > > >>> + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > > > > >>> + rb_page_size = qemu_ram_pagesize(rb); > > > > >>> + > > > > >>> + /* For now we will simply ignore unaligned memory regions */ > > > > >>> + if ((ram_offset | size) & (rb_page_size - 1)) > > > > >>> + continue; > > > > >>> + > > > > >>> + ram_block_discard_range(rb, ram_offset, size); > > > > >> I suspect this needs to do like the migration type of > > > > >> hinting and get disabled if page poisoning is in effect. > > > > >> Right? > > > > > Shouldn't something like that end up getting handled via > > > > > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases > > > > > like that would end up setting qemu_balloon_is_inhibited to true, if that > > > > > isn't the case then I could add some additional conditions. I would do it > > > > > in about the same spot as the qemu_balloon_is_inhibited check. > > > > I don't think qemu_balloon_is_inhibited() will take care of the page poisoning > > > > situations. > > > > If I am not wrong we may have to look to extend VIRTIO_BALLOON_F_PAGE_POISON > > > > support as per Michael's suggestion. > > > > > > > > > BTW upstream qemu seems to ignore VIRTIO_BALLOON_F_PAGE_POISON ATM. > > > Which is probably a bug. > > > Wei, could you take a look pls? > > > > So I was looking at sorting out this for the unused page reporting > > that I am working on and it occurred to me that I don't think we can > > do the free page hinting if any sort of poison validation is present. > > The problem is that free page hinting simply stops the page from being > > migrated. As a result if there was stale data present it will just > > leave it there instead of zeroing it or writing it to alternating 1s > > and 0s. > > stale data where? on source or on destination? > do you mean the case where memory was corrupted? > Actually I am getting my implementation and this one partially mixed up again. I was thinking that the page just gets put back. However it doesn't. Instead free_pages is called. As such it is going to dirty the page by poisoning it as soon as the hinting is complete. In some ways it is worse because I think page poisoning combined with free page hinting will make the VM nearly unusable because it will be burning cycles allocating all memory, and then poisoning all those pages on free. So it will be populating the dirty bitmap with all free memory each time it goes through and attempts to determine what memory is free. > > > > Also it looks like the VIRTIO_BALLOON_F_PAGE_POISON feature is > > assuming that 0 means that page poisoning is disabled, > > when in reality > > it might just mean we are using the value zero to poison pages instead > > of the 0xaa pattern. As such I think there are several cases where we > > could incorrectly flag the pages with the hint and result in the > > migrated guest reporting pages that contain non-poison values. > > > > > Well guest has this code: > static int virtballoon_validate(struct virtio_device *vdev) > { > if (!page_poisoning_enabled()) > __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON); > > __virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM); > return 0; > } > > So it seems that host can figure out what is going on easily enough. > What did I miss? Okay. So it is clearing that feature bit. I didn't see that part. However that leads to the question of where we should be setting that feature bit in the QEMU side of things. I was looking at setting that bit in virtio_balloon_get_features(). Would that be the appropriate place to set that so that the feature flag is reset when we are changing OSes or rebooting the guest? > > The zero assumption works for unused page reporting since we will be > > zeroing out the page when it is faulted back into the guest, however > > the same doesn't work for the free page hint since it is simply > > skipping the migration of the recently dirtied page. > > Right but the dirtied page is normally full of 0 since that is the > poison value, if we just leave it there we still get 0s, right? So for the unused page reporting which I am working on we can still hint the page away since it will be 0s, and us returning the pages doesn't alter the page data. However for the free page hinting I don't think we can. With page poisoning and free page hinting I am thinking we should just disable the free page hinting as I don't see how there can be any advantage to it if page poisoning is enbled. Basically the thing that makes the hinting "safe" will sabotage it since for every page we don't migrate we will also be marking as dirty for the next iteration through migration. As such we are just pushing the migration of any free pages until the VM has stopped since the VM will just keep dirtying the free pages until it stops hinting.
On Mon, Jul 29, 2019 at 01:21:32PM -0700, Alexander Duyck wrote: > On Mon, Jul 29, 2019 at 12:25 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Jul 29, 2019 at 09:58:04AM -0700, Alexander Duyck wrote: > > > On Wed, Jul 24, 2019 at 1:42 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Wed, Jul 24, 2019 at 04:29:27PM -0400, Nitesh Narayan Lal wrote: > > > > > > > > > > On 7/24/19 4:18 PM, Alexander Duyck wrote: > > > > > > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: > > > > > >> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > > >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > >>> > > > > > >>> Add support for what I am referring to as "bubble hinting". Basically the > > > > > >>> idea is to function very similar to how the balloon works in that we > > > > > >>> basically end up madvising the page as not being used. However we don't > > > > > >>> really need to bother with any deflate type logic since the page will be > > > > > >>> faulted back into the guest when it is read or written to. > > > > > >>> > > > > > >>> This is meant to be a simplification of the existing balloon interface > > > > > >>> to use for providing hints to what memory needs to be freed. I am assuming > > > > > >>> this is safe to do as the deflate logic does not actually appear to do very > > > > > >>> much other than tracking what subpages have been released and which ones > > > > > >>> haven't. > > > > > >>> > > > > > >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > >>> --- > > > > > >>> hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > > > > > >>> include/hw/virtio/virtio-balloon.h | 2 + > > > > > >>> include/standard-headers/linux/virtio_balloon.h | 1 + > > > > > >>> 3 files changed, 42 insertions(+), 1 deletion(-) > > > > > >>> > > > > > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > > > >>> index 2112874055fb..70c0004c0f88 100644 > > > > > >>> --- a/hw/virtio/virtio-balloon.c > > > > > >>> +++ b/hw/virtio/virtio-balloon.c > > > > > >>> @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > > > > > >>> balloon_stats_change_timer(s, 0); > > > > > >>> } > > > > > >>> > > > > > >>> +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > > > >>> +{ > > > > > >>> + VirtQueueElement *elem; > > > > > >>> + > > > > > >>> + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > > > > > >>> + unsigned int i; > > > > > >>> + > > > > > >>> + for (i = 0; i < elem->in_num; i++) { > > > > > >>> + void *addr = elem->in_sg[i].iov_base; > > > > > >>> + size_t size = elem->in_sg[i].iov_len; > > > > > >>> + ram_addr_t ram_offset; > > > > > >>> + size_t rb_page_size; > > > > > >>> + RAMBlock *rb; > > > > > >>> + > > > > > >>> + if (qemu_balloon_is_inhibited()) > > > > > >>> + continue; > > > > > >>> + > > > > > >>> + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > > > > > >>> + rb_page_size = qemu_ram_pagesize(rb); > > > > > >>> + > > > > > >>> + /* For now we will simply ignore unaligned memory regions */ > > > > > >>> + if ((ram_offset | size) & (rb_page_size - 1)) > > > > > >>> + continue; > > > > > >>> + > > > > > >>> + ram_block_discard_range(rb, ram_offset, size); > > > > > >> I suspect this needs to do like the migration type of > > > > > >> hinting and get disabled if page poisoning is in effect. > > > > > >> Right? > > > > > > Shouldn't something like that end up getting handled via > > > > > > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases > > > > > > like that would end up setting qemu_balloon_is_inhibited to true, if that > > > > > > isn't the case then I could add some additional conditions. I would do it > > > > > > in about the same spot as the qemu_balloon_is_inhibited check. > > > > > I don't think qemu_balloon_is_inhibited() will take care of the page poisoning > > > > > situations. > > > > > If I am not wrong we may have to look to extend VIRTIO_BALLOON_F_PAGE_POISON > > > > > support as per Michael's suggestion. > > > > > > > > > > > > BTW upstream qemu seems to ignore VIRTIO_BALLOON_F_PAGE_POISON ATM. > > > > Which is probably a bug. > > > > Wei, could you take a look pls? > > > > > > So I was looking at sorting out this for the unused page reporting > > > that I am working on and it occurred to me that I don't think we can > > > do the free page hinting if any sort of poison validation is present. > > > The problem is that free page hinting simply stops the page from being > > > migrated. As a result if there was stale data present it will just > > > leave it there instead of zeroing it or writing it to alternating 1s > > > and 0s. > > > > stale data where? on source or on destination? > > do you mean the case where memory was corrupted? > > > > Actually I am getting my implementation and this one partially mixed > up again. I was thinking that the page just gets put back. However it > doesn't. Instead free_pages is called. As such it is going to dirty > the page by poisoning it as soon as the hinting is complete. > > In some ways it is worse because I think page poisoning combined with > free page hinting will make the VM nearly unusable because it will be > burning cycles allocating all memory, and then poisoning all those > pages on free. So it will be populating the dirty bitmap with all free > memory each time it goes through and attempts to determine what memory > is free. Right, it does make it useless. I really consider this a bug: page should be given to hypervisor after it's poisoned. Then at least with 0 poison we could just mark it clean. For non-zero poison, we could think about adding kvm APIs for aggressively mapping all freed pages to a single non-zero one with COW. I guess it's prudent to sacrifice another feature bit if/when we fix it properly, saving a feature bit isn't worth the risk that someone shipped a guest like this. But it does show why just using alloc/free for hinting isn't as great an idea as it seems on the surface. > > > > > > Also it looks like the VIRTIO_BALLOON_F_PAGE_POISON feature is > > > assuming that 0 means that page poisoning is disabled, > > > when in reality > > > it might just mean we are using the value zero to poison pages instead > > > of the 0xaa pattern. As such I think there are several cases where we > > > could incorrectly flag the pages with the hint and result in the > > > migrated guest reporting pages that contain non-poison values. > > > > > > > > > Well guest has this code: > > static int virtballoon_validate(struct virtio_device *vdev) > > { > > if (!page_poisoning_enabled()) > > __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON); > > > > __virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM); > > return 0; > > } > > > > So it seems that host can figure out what is going on easily enough. > > What did I miss? > > Okay. So it is clearing that feature bit. I didn't see that part. > However that leads to the question of where we should be setting that > feature bit in the QEMU side of things. I was looking at setting that > bit in virtio_balloon_get_features(). Would that be the appropriate > place to set that so that the feature flag is reset when we are > changing OSes or rebooting the guest? We have DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features, VIRTIO_BALLOON_F_FREE_PAGE_HINT, false), and poison should be there, too. > > > The zero assumption works for unused page reporting since we will be > > > zeroing out the page when it is faulted back into the guest, however > > > the same doesn't work for the free page hint since it is simply > > > skipping the migration of the recently dirtied page. > > > > Right but the dirtied page is normally full of 0 since that is the > > poison value, if we just leave it there we still get 0s, right? > > So for the unused page reporting which I am working on we can still > hint the page away since it will be 0s, and us returning the pages > doesn't alter the page data. However for the free page hinting I don't > think we can. > > With page poisoning and free page hinting I am thinking we should just > disable the free page hinting as I don't see how there can be any > advantage to it if page poisoning is enbled. Basically the thing that > makes the hinting "safe" will sabotage it since for every page we > don't migrate we will also be marking as dirty for the next iteration > through migration. As such we are just pushing the migration of any > free pages until the VM has stopped since the VM will just keep > dirtying the free pages until it stops hinting. Right this was discussed at the time for non-zero poison. For hinting, my argument was simple: the reason it's useless is contained within the hypervisor. So don't put the logic in the guest: hypervisor can see poison is set and not request hints from guest. For reporting, as you say it works with 0s and we can thinkably make it work with non-0s down the road. Until we do we can always have the hypervisor do something like if (poisoning && poison != 0) return; madvise(MADV_FREE);
On Mon, Jul 29, 2019 at 1:49 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Jul 29, 2019 at 01:21:32PM -0700, Alexander Duyck wrote: > > On Mon, Jul 29, 2019 at 12:25 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Mon, Jul 29, 2019 at 09:58:04AM -0700, Alexander Duyck wrote: > > > > On Wed, Jul 24, 2019 at 1:42 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > On Wed, Jul 24, 2019 at 04:29:27PM -0400, Nitesh Narayan Lal wrote: > > > > > > > > > > > > On 7/24/19 4:18 PM, Alexander Duyck wrote: > > > > > > > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: > > > > > > >> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > > > >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > >>> > > > > > > >>> Add support for what I am referring to as "bubble hinting". Basically the > > > > > > >>> idea is to function very similar to how the balloon works in that we > > > > > > >>> basically end up madvising the page as not being used. However we don't > > > > > > >>> really need to bother with any deflate type logic since the page will be > > > > > > >>> faulted back into the guest when it is read or written to. > > > > > > >>> > > > > > > >>> This is meant to be a simplification of the existing balloon interface > > > > > > >>> to use for providing hints to what memory needs to be freed. I am assuming > > > > > > >>> this is safe to do as the deflate logic does not actually appear to do very > > > > > > >>> much other than tracking what subpages have been released and which ones > > > > > > >>> haven't. > > > > > > >>> > > > > > > >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > >>> --- > > > > > > >>> hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > > > > > > >>> include/hw/virtio/virtio-balloon.h | 2 + > > > > > > >>> include/standard-headers/linux/virtio_balloon.h | 1 + > > > > > > >>> 3 files changed, 42 insertions(+), 1 deletion(-) > > > > > > >>> > > > > > > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > > > > >>> index 2112874055fb..70c0004c0f88 100644 > > > > > > >>> --- a/hw/virtio/virtio-balloon.c > > > > > > >>> +++ b/hw/virtio/virtio-balloon.c > > > > > > >>> @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > > > > > > >>> balloon_stats_change_timer(s, 0); > > > > > > >>> } > > > > > > >>> > > > > > > >>> +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > > > > >>> +{ > > > > > > >>> + VirtQueueElement *elem; > > > > > > >>> + > > > > > > >>> + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > > > > > > >>> + unsigned int i; > > > > > > >>> + > > > > > > >>> + for (i = 0; i < elem->in_num; i++) { > > > > > > >>> + void *addr = elem->in_sg[i].iov_base; > > > > > > >>> + size_t size = elem->in_sg[i].iov_len; > > > > > > >>> + ram_addr_t ram_offset; > > > > > > >>> + size_t rb_page_size; > > > > > > >>> + RAMBlock *rb; > > > > > > >>> + > > > > > > >>> + if (qemu_balloon_is_inhibited()) > > > > > > >>> + continue; > > > > > > >>> + > > > > > > >>> + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > > > > > > >>> + rb_page_size = qemu_ram_pagesize(rb); > > > > > > >>> + > > > > > > >>> + /* For now we will simply ignore unaligned memory regions */ > > > > > > >>> + if ((ram_offset | size) & (rb_page_size - 1)) > > > > > > >>> + continue; > > > > > > >>> + > > > > > > >>> + ram_block_discard_range(rb, ram_offset, size); > > > > > > >> I suspect this needs to do like the migration type of > > > > > > >> hinting and get disabled if page poisoning is in effect. > > > > > > >> Right? > > > > > > > Shouldn't something like that end up getting handled via > > > > > > > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases > > > > > > > like that would end up setting qemu_balloon_is_inhibited to true, if that > > > > > > > isn't the case then I could add some additional conditions. I would do it > > > > > > > in about the same spot as the qemu_balloon_is_inhibited check. > > > > > > I don't think qemu_balloon_is_inhibited() will take care of the page poisoning > > > > > > situations. > > > > > > If I am not wrong we may have to look to extend VIRTIO_BALLOON_F_PAGE_POISON > > > > > > support as per Michael's suggestion. > > > > > > > > > > > > > > > BTW upstream qemu seems to ignore VIRTIO_BALLOON_F_PAGE_POISON ATM. > > > > > Which is probably a bug. > > > > > Wei, could you take a look pls? > > > > > > > > So I was looking at sorting out this for the unused page reporting > > > > that I am working on and it occurred to me that I don't think we can > > > > do the free page hinting if any sort of poison validation is present. > > > > The problem is that free page hinting simply stops the page from being > > > > migrated. As a result if there was stale data present it will just > > > > leave it there instead of zeroing it or writing it to alternating 1s > > > > and 0s. > > > > > > stale data where? on source or on destination? > > > do you mean the case where memory was corrupted? > > > > > > > Actually I am getting my implementation and this one partially mixed > > up again. I was thinking that the page just gets put back. However it > > doesn't. Instead free_pages is called. As such it is going to dirty > > the page by poisoning it as soon as the hinting is complete. > > > > In some ways it is worse because I think page poisoning combined with > > free page hinting will make the VM nearly unusable because it will be > > burning cycles allocating all memory, and then poisoning all those > > pages on free. So it will be populating the dirty bitmap with all free > > memory each time it goes through and attempts to determine what memory > > is free. > > Right, it does make it useless. > > I really consider this a bug: page should be given to hypervisor after > it's poisoned. Then at least with 0 poison we could just mark it clean. > For non-zero poison, we could think about adding kvm APIs for > aggressively mapping all freed pages to a single non-zero one with COW. > > I guess it's prudent to sacrifice another feature bit if/when > we fix it properly, saving a feature bit isn't worth > the risk that someone shipped a guest like this. > > But it does show why just using alloc/free for hinting isn't > as great an idea as it seems on the surface. What I think I can do for now is address this partially in QEMU. What I will do is just return without starting the free page hinting in virtio_balloon_free_page_start() if VIRTIO_BALLOON_F_PAGE_POISON is set. > > > > > > > > Also it looks like the VIRTIO_BALLOON_F_PAGE_POISON feature is > > > > assuming that 0 means that page poisoning is disabled, > > > > when in reality > > > > it might just mean we are using the value zero to poison pages instead > > > > of the 0xaa pattern. As such I think there are several cases where we > > > > could incorrectly flag the pages with the hint and result in the > > > > migrated guest reporting pages that contain non-poison values. > > > > > > > > > > > > > Well guest has this code: > > > static int virtballoon_validate(struct virtio_device *vdev) > > > { > > > if (!page_poisoning_enabled()) > > > __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON); > > > > > > __virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM); > > > return 0; > > > } > > > > > > So it seems that host can figure out what is going on easily enough. > > > What did I miss? > > > > Okay. So it is clearing that feature bit. I didn't see that part. > > However that leads to the question of where we should be setting that > > feature bit in the QEMU side of things. I was looking at setting that > > bit in virtio_balloon_get_features(). Would that be the appropriate > > place to set that so that the feature flag is reset when we are > > changing OSes or rebooting the guest? > > We have > DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features, > VIRTIO_BALLOON_F_FREE_PAGE_HINT, false), > and poison should be there, too. I don't think we want it to be user configurable though, do we? It seems like it should be set if either free-page-hint or unused-page-reporting are enabled. Then the guest can disable it on its end if poisoning is not present. > > > > The zero assumption works for unused page reporting since we will be > > > > zeroing out the page when it is faulted back into the guest, however > > > > the same doesn't work for the free page hint since it is simply > > > > skipping the migration of the recently dirtied page. > > > > > > Right but the dirtied page is normally full of 0 since that is the > > > poison value, if we just leave it there we still get 0s, right? > > > > So for the unused page reporting which I am working on we can still > > hint the page away since it will be 0s, and us returning the pages > > doesn't alter the page data. However for the free page hinting I don't > > think we can. > > > > With page poisoning and free page hinting I am thinking we should just > > disable the free page hinting as I don't see how there can be any > > advantage to it if page poisoning is enbled. Basically the thing that > > makes the hinting "safe" will sabotage it since for every page we > > don't migrate we will also be marking as dirty for the next iteration > > through migration. As such we are just pushing the migration of any > > free pages until the VM has stopped since the VM will just keep > > dirtying the free pages until it stops hinting. > > > Right this was discussed at the time for non-zero poison. > > For hinting, my argument was simple: the reason it's useless is contained > within the hypervisor. So don't put the logic in the guest: > hypervisor can see poison is set and not request hints from guest. Agreed. What I am going to do is store off the config value to a value local to the vdev when the config is written from the guest and the VIRTIO_BALLOON_F_PAGE_POISON value is set. Like I mentioned above I will just disable the free page hinting functionality by just not having the hypervisor request hinting if page poisoning is set. I will just drop the reports from the guest when the poison value is non-zero. > For reporting, as you say it works with 0s and we can thinkably > make it work with non-0s down the road. > Until we do we can always have the hypervisor do something like > > if (poisoning && poison != 0) > return; > madvise(MADV_FREE); Agreed.
On Mon, Jul 29, 2019 at 02:37:26PM -0700, Alexander Duyck wrote: > On Mon, Jul 29, 2019 at 1:49 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Jul 29, 2019 at 01:21:32PM -0700, Alexander Duyck wrote: > > > On Mon, Jul 29, 2019 at 12:25 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Mon, Jul 29, 2019 at 09:58:04AM -0700, Alexander Duyck wrote: > > > > > On Wed, Jul 24, 2019 at 1:42 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > On Wed, Jul 24, 2019 at 04:29:27PM -0400, Nitesh Narayan Lal wrote: > > > > > > > > > > > > > > On 7/24/19 4:18 PM, Alexander Duyck wrote: > > > > > > > > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote: > > > > > > > >> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote: > > > > > > > >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > >>> > > > > > > > >>> Add support for what I am referring to as "bubble hinting". Basically the > > > > > > > >>> idea is to function very similar to how the balloon works in that we > > > > > > > >>> basically end up madvising the page as not being used. However we don't > > > > > > > >>> really need to bother with any deflate type logic since the page will be > > > > > > > >>> faulted back into the guest when it is read or written to. > > > > > > > >>> > > > > > > > >>> This is meant to be a simplification of the existing balloon interface > > > > > > > >>> to use for providing hints to what memory needs to be freed. I am assuming > > > > > > > >>> this is safe to do as the deflate logic does not actually appear to do very > > > > > > > >>> much other than tracking what subpages have been released and which ones > > > > > > > >>> haven't. > > > > > > > >>> > > > > > > > >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > >>> --- > > > > > > > >>> hw/virtio/virtio-balloon.c | 40 +++++++++++++++++++++++ > > > > > > > >>> include/hw/virtio/virtio-balloon.h | 2 + > > > > > > > >>> include/standard-headers/linux/virtio_balloon.h | 1 + > > > > > > > >>> 3 files changed, 42 insertions(+), 1 deletion(-) > > > > > > > >>> > > > > > > > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > > > > > >>> index 2112874055fb..70c0004c0f88 100644 > > > > > > > >>> --- a/hw/virtio/virtio-balloon.c > > > > > > > >>> +++ b/hw/virtio/virtio-balloon.c > > > > > > > >>> @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, > > > > > > > >>> balloon_stats_change_timer(s, 0); > > > > > > > >>> } > > > > > > > >>> > > > > > > > >>> +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > > > > > >>> +{ > > > > > > > >>> + VirtQueueElement *elem; > > > > > > > >>> + > > > > > > > >>> + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { > > > > > > > >>> + unsigned int i; > > > > > > > >>> + > > > > > > > >>> + for (i = 0; i < elem->in_num; i++) { > > > > > > > >>> + void *addr = elem->in_sg[i].iov_base; > > > > > > > >>> + size_t size = elem->in_sg[i].iov_len; > > > > > > > >>> + ram_addr_t ram_offset; > > > > > > > >>> + size_t rb_page_size; > > > > > > > >>> + RAMBlock *rb; > > > > > > > >>> + > > > > > > > >>> + if (qemu_balloon_is_inhibited()) > > > > > > > >>> + continue; > > > > > > > >>> + > > > > > > > >>> + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > > > > > > > >>> + rb_page_size = qemu_ram_pagesize(rb); > > > > > > > >>> + > > > > > > > >>> + /* For now we will simply ignore unaligned memory regions */ > > > > > > > >>> + if ((ram_offset | size) & (rb_page_size - 1)) > > > > > > > >>> + continue; > > > > > > > >>> + > > > > > > > >>> + ram_block_discard_range(rb, ram_offset, size); > > > > > > > >> I suspect this needs to do like the migration type of > > > > > > > >> hinting and get disabled if page poisoning is in effect. > > > > > > > >> Right? > > > > > > > > Shouldn't something like that end up getting handled via > > > > > > > > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases > > > > > > > > like that would end up setting qemu_balloon_is_inhibited to true, if that > > > > > > > > isn't the case then I could add some additional conditions. I would do it > > > > > > > > in about the same spot as the qemu_balloon_is_inhibited check. > > > > > > > I don't think qemu_balloon_is_inhibited() will take care of the page poisoning > > > > > > > situations. > > > > > > > If I am not wrong we may have to look to extend VIRTIO_BALLOON_F_PAGE_POISON > > > > > > > support as per Michael's suggestion. > > > > > > > > > > > > > > > > > > BTW upstream qemu seems to ignore VIRTIO_BALLOON_F_PAGE_POISON ATM. > > > > > > Which is probably a bug. > > > > > > Wei, could you take a look pls? > > > > > > > > > > So I was looking at sorting out this for the unused page reporting > > > > > that I am working on and it occurred to me that I don't think we can > > > > > do the free page hinting if any sort of poison validation is present. > > > > > The problem is that free page hinting simply stops the page from being > > > > > migrated. As a result if there was stale data present it will just > > > > > leave it there instead of zeroing it or writing it to alternating 1s > > > > > and 0s. > > > > > > > > stale data where? on source or on destination? > > > > do you mean the case where memory was corrupted? > > > > > > > > > > Actually I am getting my implementation and this one partially mixed > > > up again. I was thinking that the page just gets put back. However it > > > doesn't. Instead free_pages is called. As such it is going to dirty > > > the page by poisoning it as soon as the hinting is complete. > > > > > > In some ways it is worse because I think page poisoning combined with > > > free page hinting will make the VM nearly unusable because it will be > > > burning cycles allocating all memory, and then poisoning all those > > > pages on free. So it will be populating the dirty bitmap with all free > > > memory each time it goes through and attempts to determine what memory > > > is free. > > > > Right, it does make it useless. > > > > I really consider this a bug: page should be given to hypervisor after > > it's poisoned. Then at least with 0 poison we could just mark it clean. > > For non-zero poison, we could think about adding kvm APIs for > > aggressively mapping all freed pages to a single non-zero one with COW. > > > > I guess it's prudent to sacrifice another feature bit if/when > > we fix it properly, saving a feature bit isn't worth > > the risk that someone shipped a guest like this. > > > > But it does show why just using alloc/free for hinting isn't > > as great an idea as it seems on the surface. > > What I think I can do for now is address this partially in QEMU. What > I will do is just return without starting the free page hinting in > virtio_balloon_free_page_start() if VIRTIO_BALLOON_F_PAGE_POISON is > set. > > > > > > > > > > > Also it looks like the VIRTIO_BALLOON_F_PAGE_POISON feature is > > > > > assuming that 0 means that page poisoning is disabled, > > > > > when in reality > > > > > it might just mean we are using the value zero to poison pages instead > > > > > of the 0xaa pattern. As such I think there are several cases where we > > > > > could incorrectly flag the pages with the hint and result in the > > > > > migrated guest reporting pages that contain non-poison values. > > > > > > > > > > > > > > > > > Well guest has this code: > > > > static int virtballoon_validate(struct virtio_device *vdev) > > > > { > > > > if (!page_poisoning_enabled()) > > > > __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON); > > > > > > > > __virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM); > > > > return 0; > > > > } > > > > > > > > So it seems that host can figure out what is going on easily enough. > > > > What did I miss? > > > > > > Okay. So it is clearing that feature bit. I didn't see that part. > > > However that leads to the question of where we should be setting that > > > feature bit in the QEMU side of things. I was looking at setting that > > > bit in virtio_balloon_get_features(). Would that be the appropriate > > > place to set that so that the feature flag is reset when we are > > > changing OSes or rebooting the guest? > > > > We have > > DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features, > > VIRTIO_BALLOON_F_FREE_PAGE_HINT, false), > > and poison should be there, too. > > I don't think we want it to be user configurable though, do we? It > seems like it should be set if either free-page-hint or > unused-page-reporting are enabled. Then the guest can disable it on > its end if poisoning is not present. Oh right. So it can be in virtio_balloon_get_features then, sorry. We still do need to make it conditional on a property (internal one, starting with "x-") for machine compat reasons. > > > > > The zero assumption works for unused page reporting since we will be > > > > > zeroing out the page when it is faulted back into the guest, however > > > > > the same doesn't work for the free page hint since it is simply > > > > > skipping the migration of the recently dirtied page. > > > > > > > > Right but the dirtied page is normally full of 0 since that is the > > > > poison value, if we just leave it there we still get 0s, right? > > > > > > So for the unused page reporting which I am working on we can still > > > hint the page away since it will be 0s, and us returning the pages > > > doesn't alter the page data. However for the free page hinting I don't > > > think we can. > > > > > > With page poisoning and free page hinting I am thinking we should just > > > disable the free page hinting as I don't see how there can be any > > > advantage to it if page poisoning is enbled. Basically the thing that > > > makes the hinting "safe" will sabotage it since for every page we > > > don't migrate we will also be marking as dirty for the next iteration > > > through migration. As such we are just pushing the migration of any > > > free pages until the VM has stopped since the VM will just keep > > > dirtying the free pages until it stops hinting. > > > > > > Right this was discussed at the time for non-zero poison. > > > > For hinting, my argument was simple: the reason it's useless is contained > > within the hypervisor. So don't put the logic in the guest: > > hypervisor can see poison is set and not request hints from guest. > > Agreed. What I am going to do is store off the config value to a value > local to the vdev when the config is written from the guest and the > VIRTIO_BALLOON_F_PAGE_POISON value is set. > > Like I mentioned above I will just disable the free page hinting > functionality by just not having the hypervisor request hinting if > page poisoning is set. I will just drop the reports from the guest > when the poison value is non-zero. Makes sense. In fact this part is a bugfix, and if you can post it quickly I'll try to fast track it into 4.1 (might be too late for that, we'll see). > > For reporting, as you say it works with 0s and we can thinkably > > make it work with non-0s down the road. > > Until we do we can always have the hypervisor do something like > > > > if (poisoning && poison != 0) > > return; > > madvise(MADV_FREE); > > Agreed.
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index 2112874055fb..70c0004c0f88 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, balloon_stats_change_timer(s, 0); } +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq) +{ + VirtQueueElement *elem; + + while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) { + unsigned int i; + + for (i = 0; i < elem->in_num; i++) { + void *addr = elem->in_sg[i].iov_base; + size_t size = elem->in_sg[i].iov_len; + ram_addr_t ram_offset; + size_t rb_page_size; + RAMBlock *rb; + + if (qemu_balloon_is_inhibited()) + continue; + + rb = qemu_ram_block_from_host(addr, false, &ram_offset); + rb_page_size = qemu_ram_pagesize(rb); + + /* For now we will simply ignore unaligned memory regions */ + if ((ram_offset | size) & (rb_page_size - 1)) + continue; + + ram_block_discard_range(rb, ram_offset, size); + } + + virtqueue_push(vq, elem, 0); + virtio_notify(vdev, vq); + g_free(elem); + } +} + static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) { VirtIOBalloon *s = VIRTIO_BALLOON(vdev); @@ -782,6 +815,11 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp) s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats); if (virtio_has_feature(s->host_features, + VIRTIO_BALLOON_F_HINTING)) { + s->hvq = virtio_add_queue(vdev, 128, virtio_bubble_handle_output); + } + + if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE, virtio_balloon_handle_free_page_vq); @@ -897,6 +935,8 @@ static Property virtio_balloon_properties[] = { VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false), DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features, VIRTIO_BALLOON_F_FREE_PAGE_HINT, false), + DEFINE_PROP_BIT("guest-page-hinting", VirtIOBalloon, host_features, + VIRTIO_BALLOON_F_HINTING, true), DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD, IOThread *), DEFINE_PROP_END_OF_LIST(), diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h index 1afafb12f6bc..a58b24fdf29d 100644 --- a/include/hw/virtio/virtio-balloon.h +++ b/include/hw/virtio/virtio-balloon.h @@ -44,7 +44,7 @@ enum virtio_balloon_free_page_report_status { typedef struct VirtIOBalloon { VirtIODevice parent_obj; - VirtQueue *ivq, *dvq, *svq, *free_page_vq; + VirtQueue *ivq, *dvq, *svq, *free_page_vq, *hvq; uint32_t free_page_report_status; uint32_t num_pages; uint32_t actual; diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h index 9375ca2a70de..f9e3e8256261 100644 --- a/include/standard-headers/linux/virtio_balloon.h +++ b/include/standard-headers/linux/virtio_balloon.h @@ -36,6 +36,7 @@ #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */ #define VIRTIO_BALLOON_F_FREE_PAGE_HINT 3 /* VQ to report free pages */ #define VIRTIO_BALLOON_F_PAGE_POISON 4 /* Guest is using page poisoning */ +#define VIRTIO_BALLOON_F_HINTING 5 /* Page hinting virtqueue */ /* Size of a PFN in the balloon interface. */ #define VIRTIO_BALLOON_PFN_SHIFT 12