Re: [virtio-dev] Re: [PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled

[David Hildenbrand <david@redhat.com>]

>>
>> That leaves us with either
>>
>> 1. "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced
>> by zero page. However, as soon as the page is written by the guest (even
>> before the hinting request was processed by the host), the modified page
>> will stay - whereby the unwritten parts might either be from the old, or
>> from the zero page." - a QEMU BUG.
> 
> How is this a bug? This is the behavior you would see with the current
> balloon driver. When you put a page into a balloon it has the option
> to either madvise it away, defer it, or just skip it because he
> balloon is disabled. Is the "zero page" a typo? If it was
> uninitialized data that would be a bug, but I don't see how a zero
> page or the old data would be a bug.

Sorry, I meant if this was the original design idea of hinting, then we
would have a QEMU BUG as of now, as we might get get uninitialized data.
Makes sense?

[...]

> 
>>>
>>>> The current QEMU implementation would be to simply migrate all hinted
>>>> pages. In the future we could optimize. Not sure if it's worth the trouble.
>>>
>>> So the trick for me is how best to go about sorting this all out.
>>> There are two ways I see of doing it.
>>>
>>> The first approach would be to just assume that hinting should be
>>> disassociated from poisoning. If I go that route that would more
>>> closely match up with what happened in QEMU. The downside is that it
>>> locks in the unmodified/uninitialized behavior and would require pages
>>> to be rewritten when they come out of the balloon. However this is the
>>> behavior we have now so it would only require specification
>>> documentation changes.
>>
>> Right now, for simplicity, I prefer this and define
>> VIRTIO_BALLOON_F_PAGE_POISON only for explicit deflation (balloon
>> deflation via the deflate queue) and implicit deflation
>> (VIRTIO_BALLOON_F_REPORTING).
> 
> I don't recall us talking about the explicit deflation case before.

The interesting part is that drivers/virtio/virtio_balloon.c mentions:

"Let the hypervisor know that we are expecting a specific value to be
written back in balloon pages.".

But I just realized that you introduced this comment, not the original
VIRTIO_BALLOON_F_PAGE_POISON commit.

Should this have been "in reported pages when implicitly deflating them
by reusing them." or sth. like that?

> What is the expectation there? I assume we are saying either
> poison_val or unmodified? If so I would think the inflate case makes
> much more sense as that is where the madvise is called that will
> discard the data. If so it would be pretty easy to just add a check
> for the poison value to the same spot we check
> qemu_balloon_is_inhibited.

Okay, we have basically no idea what was the intention of
VIRTIO_BALLOON_F_PAGE_POISON with basic deflation/inflation as well. So
I think we can define what suits us.

On the deflate path, we could always simply fill with poison_val. But
there are nasty corner cases (esp. no VIRTIO_BALLOON_F_MUST_TELL_HOST).

What would be your suggestion? Also don't care about
VIRTIO_BALLOON_F_PAGE_POISON on ordinary inflation/deflation? At this
point, I think this makes sense.

-- 
Thanks,

David / dhildenb