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

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

On Tue, Apr 21, 2020 at 8:18 AM David Hildenbrand <david at redhat.com>
wrote:
>
> >>
> >> 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?
Yes, that makes sense. So the bug is that we aren't doing this right now.
> >
> >>>
> >>>> 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?
Yeah, probably. I should have probably used "reported" instead of
"balloon" in the comment.
> > 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.
That is kind of what I was thinking. The problem is that once again
the current implementation works when page poisoning is enabled. Us
disabling that wouldn't make much sense.

The whole thing with the reporting is that we are essentially just
ballooning in place. What we may do at some point in the future would
be to add an additional feature bit to do that for the standard
balloon/hinting case. Then when that is set, and we know the contents
won't match we can then just skip the madvise or hinting calls. That
way it becomes an opt-in which is what the poison was supposed to be,
but wasn't because the QEMU side was never implemented.

In the meantime I still have to make more changes to my QEMU patch
set. The way the config_size logic is implemented is somewhat of a
pain when you factor in the way the host_features and poison were
handled.

>>> 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.
> 
> That is kind of what I was thinking. The problem is that once again
> the current implementation works when page poisoning is enabled. Us
> disabling that wouldn't make much sense.
> 
> The whole thing with the reporting is that we are essentially just
> ballooning in place. What we may do at some point in the future would
> be to add an additional feature bit to do that for the standard
> balloon/hinting case. Then when that is set, and we know the contents
> won't match we can then just skip the madvise or hinting calls. That
> way it becomes an opt-in which is what the poison was supposed to be,
> but wasn't because the QEMU side was never implemented.
Yeah, introducing this later makes sense.

So VIRTIO_BALLOON_F_PAGE_POISON really means:
- poison_val in the config is unlocked
- when active, the guest is using page poisoning/init on free with
  poison_val ("for you information")
- it only changes the semantic of free page reporting, nothing else.
  (when reusing reported pages in the guest, they will either have the
  old content, or will be filled with poison_val.)

Makes sense? That should be easy to document.
> 
> In the meantime I still have to make more changes to my QEMU patch
> set. The way the config_size logic is implemented is somewhat of a
> pain when you factor in the way the host_features and poison were
> handled.
Okay, I'll wait for updated QEMU patches.

-- 
Thanks,

David / dhildenb