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

[Alexander Duyck <alexander.h.duyck@linux.intel.com>]

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