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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
>
> Thanks!
>
Thanks for the review.
- Alex
