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

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Nitesh Narayan Lal <nitesh@redhat.com>
Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>,
	Alexander Duyck <alexander.duyck@gmail.com>,
	kvm@vger.kernel.org, david@redhat.com, dave.hansen@intel.com,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	akpm@linux-foundation.org, yang.zhang.wz@gmail.com,
	pagupta@redhat.com, riel@surriel.com, konrad.wilk@oracle.com,
	lcapitulino@redhat.com, wei.w.wang@intel.com,
	aarcange@redhat.com, pbonzini@redhat.com,
	dan.j.williams@intel.com
Subject: Re: [PATCH v2 QEMU] virtio-balloon: Provide a interface for "bubble hinting"
Date: Wed, 24 Jul 2019 16:42:29 -0400	[thread overview]
Message-ID: <20190724164023-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <ee5387b1-89af-daf4-8492-8139216c6dcf@redhat.com>

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

BTW upstream qemu seems to ignore VIRTIO_BALLOON_F_PAGE_POISON ATM.
Which is probably a bug.
Wei, could you take a look pls?

> >
> >
> -- 
> Thanks
> Nitesh