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

[David Hildenbrand]

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.

[...]

> +
> +	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)

Apart from that

Reviewed-by: David Hildenbrand <david@xxxxxxxxxx>

Thanks!

-- 
Thanks,

David / dhildenb