Il 28/05/2013 16:29, Michael S. Tsirkin ha scritto:
> On Tue, May 28, 2013 at 04:06:02PM +0200, Paolo Bonzini wrote:
>> Il 28/05/2013 15:32, Michael S. Tsirkin ha scritto:
>>> At this point I am confused. I think there are two changes in your patch:
>>>
>>> 1. Handling of VIRTIO_F_GUEST_MUST_TELL_HOST
>>> Is this functionally identical to what I proposed?
>>> If yes, I am fine with either change being applied.
>>
>> Yes.
>>
>>> 2. New SILENT_DEFLATE feature
>>> Since guest can get same functionality by not acking
>>> TELL_HOST, I still don't see what good it does:
>>> Historically a host with no features supports silent
>>> deflate and guest with no features can do silent deflate.
>>> I conclude silent deflate is the default behaviour for
>>> both host and guest, and we can't change default without
>>> breaking compatibility.
>>
>> You're right that for correctness the existing feature is enough:
>> if it is not negotiated by the guest, the host ensures correctness by
>> only giving the guest a fake balloon.
>>
>> However, the new feature is about optimization, not correctness.
>> In fact, VIRTIO_BALLOON_F_SILENT_DEFLATE is the optimization
>> feature that VIRTIO_BALLOON_F_MUST_TELL_HOST was meant to be.
>>
>> What I'm interested in, is drivers that can _optionally_ use silent
>> deflation (as an optimization). These should not get a fake balloon!
>>
>> With the new feature bit, these drivers should propose both
>> VIRTIO_BALLOON_F_GUEST_TELLS_HOST and VIRTIO_BALLOON_F_SILENT_DEFLATE.
>> The driver can then use silent deflation if and only if the host
>> has negotiated VIRTIO_BALLOON_F_SILENT_DEFLATE too. Like this:
>>
>> diff --git a/drivers/virtio/virtio_balloon.c
>> b/drivers/virtio/virtio_balloon.c
>> index bd3ae32..05fe948 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -186,12 +186,8 @@ static void leak_balloon(struct virtio_balloon *vb,
>> size_t num)
>> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
>> }
>>
>> - /*
>> - * Note that if
>> - * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
>> - * is true, we *have* to do it in this order
>> - */
>> - tell_host(vb, vb->deflate_vq);
>> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SILENT_DEFLATE)
>> + tell_host(vb, vb->deflate_vq);
>> mutex_unlock(&vb->balloon_lock);
>> release_pages_by_pfn(vb->pfns, vb->num_pfns);
>> }
>> @@ -543,6 +539,7 @@ static int virtballoon_restore(struct virtio_device
>> *vdev)
>> static unsigned int features[] = {
>> VIRTIO_BALLOON_F_MUST_TELL_HOST,
>> VIRTIO_BALLOON_F_STATS_VQ,
>> + VIRTIO_BALLOON_F_SILENT_DEFLATE,
>> };
>>
>> static struct virtio_driver virtio_balloon_driver = {
>>
>>
>> Of course with the current implementation of the balloon it does not
>> matter much. But for example, with Luiz's work, releasing pages as soon
>> as the shrinker is called will increase effectiveness of the shrinker.
>> At the same time, not all is lost if the guest prefers not to allow
>> silent deflation (e.g. because there is an assigned device).
>>
>> On old hosts, a guest that can optionally use silent deflation will
>> not use it. That's the same as for any other feature bit.
>>
>>> How about splitting the patches so we can discuss them separately?
>>
>> I can do that, but I hope the above clarifies it.
>
> Maybe I'm just dense.
> Let's see the split spec patchset?
What's unclear exactly? I'm not sure the spec patchset improves things
that much, I can split it in two or three (change old feature, add new
feature, add explanation) but it's not like changing logic in a program.
Paolo
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
