Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"

[Alexander Duyck <alexander.duyck@gmail.com>]

On Wed, Jul 24, 2019 at 1:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jul 24, 2019 at 04:29:27PM -0400, Nitesh Narayan Lal wrote:
> >
> > On 7/24/19 4:18 PM, Alexander Duyck wrote:
> > > On Wed, 2019-07-24 at 15:02 -0400, Michael S. Tsirkin wrote:
> > >> On Wed, Jul 24, 2019 at 10:12:10AM -0700, Alexander Duyck wrote:
> > >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > >>>
> > >>> Add support for what I am referring to as "bubble hinting". Basically the
> > >>> idea is to function very similar to how the balloon works in that we
> > >>> basically end up madvising the page as not being used. However we don't
> > >>> really need to bother with any deflate type logic since the page will be
> > >>> faulted back into the guest when it is read or written to.
> > >>>
> > >>> This is meant to be a simplification of the existing balloon interface
> > >>> to use for providing hints to what memory needs to be freed. I am assuming
> > >>> this is safe to do as the deflate logic does not actually appear to do very
> > >>> much other than tracking what subpages have been released and which ones
> > >>> haven't.
> > >>>
> > >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > >>> ---
> > >>>  hw/virtio/virtio-balloon.c                      |   40 +++++++++++++++++++++++
> > >>>  include/hw/virtio/virtio-balloon.h              |    2 +
> > >>>  include/standard-headers/linux/virtio_balloon.h |    1 +
> > >>>  3 files changed, 42 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > >>> index 2112874055fb..70c0004c0f88 100644
> > >>> --- a/hw/virtio/virtio-balloon.c
> > >>> +++ b/hw/virtio/virtio-balloon.c
> > >>> @@ -328,6 +328,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
> > >>>      balloon_stats_change_timer(s, 0);
> > >>>  }
> > >>>
> > >>> +static void virtio_bubble_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > >>> +{
> > >>> +    VirtQueueElement *elem;
> > >>> +
> > >>> +    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
> > >>> +         unsigned int i;
> > >>> +
> > >>> +        for (i = 0; i < elem->in_num; i++) {
> > >>> +            void *addr = elem->in_sg[i].iov_base;
> > >>> +            size_t size = elem->in_sg[i].iov_len;
> > >>> +            ram_addr_t ram_offset;
> > >>> +            size_t rb_page_size;
> > >>> +            RAMBlock *rb;
> > >>> +
> > >>> +            if (qemu_balloon_is_inhibited())
> > >>> +                continue;
> > >>> +
> > >>> +            rb = qemu_ram_block_from_host(addr, false, &ram_offset);
> > >>> +            rb_page_size = qemu_ram_pagesize(rb);
> > >>> +
> > >>> +            /* For now we will simply ignore unaligned memory regions */
> > >>> +            if ((ram_offset | size) & (rb_page_size - 1))
> > >>> +                continue;
> > >>> +
> > >>> +            ram_block_discard_range(rb, ram_offset, size);
> > >> I suspect this needs to do like the migration type of
> > >> hinting and get disabled if page poisoning is in effect.
> > >> Right?
> > > Shouldn't something like that end up getting handled via
> > > qemu_balloon_is_inhibited, or did I miss something there? I assumed cases
> > > like that would end up setting qemu_balloon_is_inhibited to true, if that
> > > isn't the case then I could add some additional conditions. I would do it
> > > in about the same spot as the qemu_balloon_is_inhibited check.
> > I don't think qemu_balloon_is_inhibited() will take care of the page poisoning
> > situations.
> > If I am not wrong we may have to look to extend VIRTIO_BALLOON_F_PAGE_POISON
> > support as per Michael's suggestion.
>
>
> BTW upstream qemu seems to ignore VIRTIO_BALLOON_F_PAGE_POISON ATM.
> Which is probably a bug.
> Wei, could you take a look pls?

So I was looking at sorting out this for the unused page reporting
that I am working on and it occurred to me that I don't think we can
do the free page hinting if any sort of poison validation is present.
The problem is that free page hinting simply stops the page from being
migrated. As a result if there was stale data present it will just
leave it there instead of zeroing it or writing it to alternating 1s
and 0s.

Also it looks like the VIRTIO_BALLOON_F_PAGE_POISON feature is
assuming that 0 means that page poisoning is disabled, when in reality
it might just mean we are using the value zero to poison pages instead
of the 0xaa pattern. As such I think there are several cases where we
could incorrectly flag the pages with the hint and result in the
migrated guest reporting pages that contain non-poison values.

The zero assumption works for unused page reporting since we will be
zeroing out the page when it is faulted back into the guest, however
the same doesn't work for the free page hint since it is simply
skipping the migration of the recently dirtied page.