Re: [PATCH v15 6/7] virtio-balloon: Add support for providing free page reports to host

Alexander Duyck
2019-12-13T20:39:24Z

On Fri, 2019-12-13 at 11:15 +0100, David Hildenbrand wrote:
> On 05.12.19 17:22, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Add support for the page reporting feature provided by virtio-balloon.
> > Reporting differs from the regular balloon functionality in that is is
> > much less durable than a standard memory balloon. Instead of creating a
> > list of pages that cannot be accessed the pages are only inaccessible
> > while they are being indicated to the virtio interface. Once the
> > interface has acknowledged them they are placed back into their respective
> > free lists and are once again accessible by the guest system.
> >
> > Unlike a standard balloon we don't inflate and deflate the pages. Instead
> > we perform the reporting, and once the reporting is completed it is
> > assumed that the page has been dropped from the guest and will be faulted
> > back in the next time the page is accessed.
> >
> > For this reason when I had originally introduced the patch set I referred
> > to this behavior as a "bubble" instead of a "balloon" since the duration
> > is short lived, and when the page is touched the "bubble" is popped and
> > the page is faulted back in.
>
> While an interesting read, I would drop that comment as it isn't really
> of value for the code/codebase itself.

Okay, I can drop the comment.

> [...]
>
> > +
> > + vb->pr_dev_info.report = virtballoon_free_page_report;
> > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING)) {
> > + unsigned int capacity;
> > +
> > + capacity = virtqueue_get_vring_size(vb->reporting_vq);
> > + if (capacity < PAGE_REPORTING_CAPACITY) {
> > + err = -ENOSPC;
> > + goto out_unregister_shrinker;
>
> It's somewhat strange to fail loading the balloon completely here.
> Wouldn't it be better to print e.g. a warning but continue without free
> page reporting?
>
> (I guess splitting up the list can be done in an addon patch if ever
> really needed for virtio-balloon)

That was kind of my thought. Odds are it probably is unlikely to come up.
At least with the code I have now I think the virtqueue size is something
like 128 and the capacity is only 32 as I wanted to limit the number of
pages that were being reported at the time.

> Apart from that
>
> Reviewed-by: David Hildenbrand <david@redhat.com>
>
> Thanks!
>

Thanks for the review.

- Alex