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

[Alexander Duyck <alexander.duyck@gmail.com>]

On Tue, Apr 21, 2020 at 8:18 AM David Hildenbrand <david@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.