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

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

On Wed, Apr 22, 2020 at 3:24 AM David Hildenbrand <david@redhat.com> wrote:
>
> >>> 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.

Yep, makes sense. In theory the old content or being filled with
poison_val should be the same thing.

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

I got to the root cause of the issues I was seeing. The config size
being dependent on the page poison feature was somewhat problematic as
it affects where I can place the setting of the bit since I have to
have it done before we call virtio_init. I should be submitting the
patches this afternoon. I am just going through and making sure I have
my bases covered and testing for any corner cases.