Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 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
> > > 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
> > > > > 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
> > > > > > > >>>
> > > > > > > >>> 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
> > > > > > > >>>
> > > > > > > >>> ---
> > > > > > > >>> 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,
> > > > > > > >>> _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
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
On Mon, Jul 29, 2019 at 1:49 PM Michael S. Tsirkin 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 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
> > > > 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
> > > > > > >>>
> > > > > > >>> 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
> > > > > > >>>
> > > > > > >>> ---
> > > > > > >>> 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,
> > > > > > >>> _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?
>
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble 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 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
> > > 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
> > > > > >>>
> > > > > >>> 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
> > > > > >>> ---
> > > > > >>> 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,
> > > > > >>> _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
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
On Mon, Jul 29, 2019 at 12:25 PM Michael S. Tsirkin 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 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
> > > > >>>
> > > > >>> 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
> > > > >>> ---
> > > > >>> 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,
> > > > >>> _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
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 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
> > > >>>
> > > >>> 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
> > > >>> ---
> > > >>> 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, _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
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
On Wed, Jul 24, 2019 at 1:42 PM Michael S. Tsirkin 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
> > >>>
> > >>> 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
> > >>> ---
> > >>> 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, _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.
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 > > > > >>> 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. -- Thanks Nitesh
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 > > > > > > > > > > > > > > > > > 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.
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 >>> >>> 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 >> 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 >
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 > > > > > > > > > > > > > > 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 > > > > > > 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
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 > > > > > > > > > > > > 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 > > > > > 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
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 > > > > > > > > > > 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 > > > > 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. -- MST
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 > > > > > > > > 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 > > > 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.
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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
> > > >
> > > > 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
> > > > ---
> > > > 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, _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.
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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
>>>
>>> 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
>>> ---
>>> 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, _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.
>
>
--
Thanks
Nitesh
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 >>> >>> 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 >> 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. > -- Thanks Nitesh
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 > > > > > > > > > > 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 > > > > > > > > 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 :)
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 > > > > > > > > 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 > > > > > > 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.
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 > > > > > > 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 > > > > 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 :) -- MST
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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 > > > > 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 > > 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.
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote:
> From: Alexander Duyck
>
> 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
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, _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
> +++
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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
> > > >
> > > > 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
> > > > ---
> > > > 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, _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.
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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
> > >
> > > 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
> > > ---
> > > 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, _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.
--
MST
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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
> >>>
> >>> 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
> >>> ---
> >>> 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, _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
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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
>>>
>>> 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
>>> ---
>>> 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, _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.
>
>
--
Thanks
Nitesh
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
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
> >
> > 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
> > ---
> > 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, _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.
Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote:
> From: Alexander Duyck
>
> 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
> ---
> 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, _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
