Re: [Qemu-devel] [PATCH v7 4/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

[Wei Wang <wei.w.wang@intel.com>]

On 06/01/2018 01:42 AM, Michael S. Tsirkin wrote:
> On Thu, May 31, 2018 at 10:27:00AM +0800, Wei Wang wrote:
>> On 05/30/2018 08:47 PM, Michael S. Tsirkin wrote:
>>> On Wed, May 30, 2018 at 05:12:09PM +0800, Wei Wang wrote:
>>>> On 05/29/2018 11:24 PM, Michael S. Tsirkin wrote:
>>>>> On Tue, Apr 24, 2018 at 02:13:47PM +0800, Wei Wang wrote:
>>>>>> +/*
>>>>>> + * Balloon will report pages which were free at the time of this call. As the
>>>>>> + * reporting happens asynchronously, dirty bit logging must be enabled before
>>>>>> + * this call is made.
>>>>>> + */
>>>>>> +void balloon_free_page_start(void)
>>>>>> +{
>>>>>> +    balloon_free_page_start_fn(balloon_opaque);
>>>>>> +}
>>>>> Please create notifier support, not a single global.
>>>> OK. The start is called at the end of bitmap_sync, and the stop is called at
>>>> the beginning of bitmap_sync. In this case, we will need to add two
>>>> migration states, MIGRATION_STATUS_BEFORE_BITMAP_SYNC and
>>>> MIGRATION_STATUS_AFTER_BITMAP_SYNC, right?
>>> If that's the way you do it, you need to ask migration guys, not me.
>> Yeah, I know.. thanks for the virtio part.
>>
>>>>> +
>>>>> +static bool virtio_balloon_free_page_support(void *opaque)
>>>>> +{
>>>>> +    VirtIOBalloon *s = opaque;
>>>>> +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>>>>> +
>>>>> +    return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
>>>>> or if poison is negotiated.
>>>> Will make it
>>>> return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT) &&
>>>> !virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)
>>> I mean the reverse:
>>> 	virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT) ||
>>> 	virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)
>>>
>>>
>>> If poison has been negotiated you must migrate the
>>> guest supplied value even if you don't use it for hints.
>>
>> Just a little confused with the logic. Writing it that way means that we are
>> taking this possibility "virtio_vdev_has_feature(vdev,
>> VIRTIO_BALLOON_F_FREE_PAGE_HINT)=fasle, virtio_vdev_has_feature(vdev,
>> VIRTIO_BALLOON_F_PAGE_POISON)=true" into account, and let the support
>> function return true when F_FREE_PAGE_HINT isn't supported.
> All I am saying is that in this configuration, you must migrate
> the poison value programmed by guest even if you do not
> yet use it without VIRTIO_BALLOON_F_FREE_PAGE_HINT.
>
> Right now you have
> a section:
> +    .needed = virtio_balloon_free_page_support,
>
> which includes the poison value.
>
> So if guest migrates after writing the poison value,
> it's lost. Not nice.
>
>> If guest doesn't support F_FREE_PAGE_HINT, it doesn't support the free page
>> reporting (even the free page vq). I'm not sure why we tell the migration
>> thread that the free page reporting feature is supported via this support
>> function. If the support function simply returns false when F_FREE_PAGE_HINT
>> isn't negotiated, the legacy migration already migrates the poisoned pages
>> (not skipped, but may be compressed).
>>
>> I think it would be better to simply use the original "return
>> virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)" here.
>
> So maybe you should put the poison value in a separate section then.

Yes, that looks good to me, thanks.

Best,
Wei