> Am 15.04.2020 um 21:29 schrieb Alexander Duyck <alexander.du...@gmail.com>:
>
> On Wed, Apr 15, 2020 at 11:17 AM David Hildenbrand <da...@redhat.com> wrote:
>>
>>>
>>> The comment above explains the "why". Basically poisoning a page will
>>> dirty it. So why hint a page as free when that will drop it back into
>>> the guest and result in it being dirtied again. What you end up with
>>> is all the pages that were temporarily placed in the balloon are dirty
>>> after the hinting report is finished at which point you made things
>>> worse instead of helping to improve them.
>>
>> Let me think this through. What happens on free page hinting is that
>>
>> a) we tell the guest that migration starts
>> b) it allocates pages (alloc_pages()), sends them to us and adds them to
>> a list
>> b) we exclude these pages on migration
>> c) we tell the guest that migration is over
>> d) the guest frees all allocated pages
>>
>> The "issue" with VIRTIO_BALLOON_F_PAGE_POISON is, that in d), the guest
>> will fill all pages again with a pattern (or zero).
>
> They should have already been filled with the poison pattern before we
> got to d). A bigger worry is that we at some point in the future
> update the kernel so that d) doesn't trigger re-poisoning, in which
> case the pages won't be marked as dirty, we will have skipped the
> migration, and we have no idea what will be in the pages at the end.
>
>> AFAIKs, it's either
>>
>> 1) Not performing free page hinting, migrating pages filled with a
>> poison value (or zero).
>> 2) Performing free page hinting, not migrating pages filled with a
>> poison value (or zero), letting only the guest fill them again.
>>
>> I have the feeling, that 2) is still better for migration, because we
>> don't migrate useless pages and let the guest reconstruct the content?
>> (having a poison value of zero might be debatable)
>>
>> Can you tell me what I am missing? :)
>
> The goal of the free page hinting was to reduce the number of pages
> that have to be migrated, in the second case you point out is is
> basically deferring it and will make the post-copy quite more
> expensive since all of the free memory will be pushed to the post-copy
> which I would think would be undesirable since it means the VM would
> have to be down for a greater amount of time with the poisoning
> enabled.
Postcopy is a very good point, bought!
But (what you wrote above) it sounds like that this is really what we *have to*
do, not an optimization. I‘ll double check the spec tomorrow (hopefully it was
documented). We should rephrase the comment then.
I could imagine that we also have to care about ordinary page
inflation/deflation when handling page poisoning. (Iow, don‘t inflate/deflate
if set for now).
>
> The worst case scenario would be one in which the VM was suspended for
> migration while there were still pages in the balloon that needed to
> be drained. In such a case you would have them in an indeterminate
> state since the last page poisoning for them might have been ignored
> since they were marked as "free", so they are just going to be
> whatever value they were if they had been migrated at all.
>
>>>
>>>>
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> if (s->free_page_report_cmd_id == UINT_MAX) {
>>>>> s->free_page_report_cmd_id =
>>>>> VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
>>>>
>>>> We should rename all "free_page_report" stuff we can to
>>>> "free_page_hint"/"free_page_hinting" to avoid confusion (e.g., on my
>>>> side :) ) before adding free page reporting .
>>>>
>>>> (looking at the virtio-balloon linux header, it's also confusing but
>>>> we're stuck with that - maybe we should add better comments)
>>>
>>> Are we stuck? Couldn't we just convert it to an anonymous union with
>>> free_page_hint_cmd_id and then use that where needed?
>>
>> Saw your patch already :)
>>
>>>
>>>>> @@ -618,12 +627,10 @@ static size_t
>>>>> virtio_balloon_config_size(VirtIOBalloon *s)
>>>>> if (s->qemu_4_0_config_size) {
>>>>> return sizeof(struct virtio_balloon_config);
>>>>> }
>>>>> - if (virtio_has_feature(features, VIRTIO_BALLOON_F_PAGE_POISON)) {
>>>>> + if (virtio_has_feature(features, VIRTIO_BALLOON_F_PAGE_POISON) ||
>>>>> + virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>>>>> return sizeof(struct virtio_balloon_config);
>>>>> }
>>>>> - if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>>>>> - return offsetof(struct virtio_balloon_config, poison_val);
>>>>> - }
>>>>
>>>> I am not sure this change is completely sane. Why is that necessary at all?
>>>
>>> The poison_val is stored at the end of the structure and is required
>>> in order to make free page hinting to work. What this change is doing
>>
>> Required to make it work? In the kernel,
>>
>> commit 2e991629bcf55a43681aec1ee096eeb03cf81709
>> Author: Wei Wang <wei.w.w...@intel.com>
>> Date: Mon Aug 27 09:32:19 2018 +0800
>>
>> virtio-balloon: VIRTIO_BALLOON_F_PAGE_POISON
>>
>> was merged after
>>
>> commit 86a559787e6f5cf662c081363f64a20cad654195
>> Author: Wei Wang <wei.w.w...@intel.com>
>> Date: Mon Aug 27 09:32:17 2018 +0800
>>
>> virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT
>>
>> So I assume it's perfectly fine to not have it.
>>
>> I'd say it's the responsibility of the guest to *not* use
>> VIRTIO_BALLOON_F_FREE_PAGE_HINT in case it is using page poisoning
>> without VIRTIO_BALLOON_F_PAGE_POISON. Otherwise it will shoot itself
>> into the foot.
>
> Based on the timing I am guessing the page poisoning was in the same
> patch set as the free page hinting since there is only 2 seconds
> between when the two are merged. If I recall the page poisoning logic
> was added after the issue was pointed out that it needed to address
> it.
>
Yeah, but any other OS implementing this, not caring about page poisoning
wouldn‘t need it. Maybe there is something in the spec.
Mental note: we‘ll have to migrate the poison value if not already done
(writing on my mobile).
