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

On 17.04.20 10:50, Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2020 at 09:49:03AM +0200, David Hildenbrand wrote:
>> On 17.04.20 08:28, Michael S. Tsirkin wrote:
>>> On Thu, Apr 16, 2020 at 04:52:42PM -0700, Alexander Duyck wrote:
>>>> On Thu, Apr 16, 2020 at 3:13 PM Michael S. Tsirkin <mst at
redhat.com> wrote:
>>>>>
>>>>> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck
wrote:
>>>>>> From: Alexander Duyck <alexander.h.duyck at
linux.intel.com>
>>>>>>
>>>>>> If we have free page hinting or page reporting enabled
we should disable it
>>>>>> if the pages are poisoned or initialized on free and we
cannot notify the
>>>>>> hypervisor.
>>>>>>
>>>>>> Fixes: 5d757c8d518d ("virtio-balloon: add support
for providing free page reports to host")
>>>>>>
>>>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck at
linux.intel.com>
>>>>>
>>>>>
>>>>> Why not put this logic in the hypervisor?
>>>>
>>>> I did that too. This is to cover the case where somebody is
running
>>>> the code prior to my QEMU changes where the page poison feature
wasn't
>>>> being enabled.
>>>
>>>
>>> Hmm so that one looks like a QEMU bug (does not expose poison
flag).  In
>>> the past we just said need to fix the bug where it's found
unless the
>>> issue is very widespread and major.  Let's assume QEMU learns
to always
>>> expose POISON with HINT.  Then this configuration (HINT &&
!POISON)
>>> allows us to detect old QEMU (pre your changes).
>>
>> Don't see why this is a QEMU bug. It's just a feature it does
not
>> implement. Perfectly valid.
> 
> I'll need to think about this.
> We need to remember that the whole HINT thing is not a mandate for host to
> corrupt memory. It's just some info about page use guest
> gives host. If host corrupts memory it's broken ...
I don't think that's true, and that's not what we currently
implement in
the hypervisor for speeding up migration. I still consider
VIRTIO_BALLOON_F_FREE_PAGE_HINT as an alternative technique of
temporarily inflating/deflating. E.g., we don't migrate any of these
pages in the hypervisor, so the content will be zeroed out on the
migration target. If migration fails, the ld content will remain. You
can call that "corrupting memory" - it's exactly what it has been
invented for.


IIRC we decided to glue the semantics of VIRTIO_BALLOON_F_PAGE_POISON to
VIRTIO_BALLOON_F_FREE_PAGE_HINT, which makes in my opinion perfect sense.

So I propose:

VIRTIO_BALLOON_F_PAGE_POISON not implemented in the hypervisor:
- Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will
  either have the old page content or will be filled with 0.
- This matches what we currently do.

VIRTIO_BALLOON_F_PAGE_POISON implemented in the hypervisor:
- Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will
  either have the old page content or will be filled with poison_val.
- This matches what we should do also during ordinary
  inflation/deflation and free page reporting.

Again, nothing is currently broken as we free_page() the pages and don't
care about an eventually changed page content. It's only relevant once
we ant to change that - and then we can rely on
VIRTIO_BALLOON_F_PAGE_POISON.
>>>>
>>>> The problem is we cannot communicate the full situation to the
>>>> hypervisor without the page poison feature being present. As
such I
>>>> would worry about that backfiring on us due to the hypervisor
acting
>>>> on incomplete data.
>>>
>>>
>>> I'll try to think about VIRTIO_BALLOON_F_FREE_PAGE_HINT
situation
>>> over the weekend. But for the new page reporting, why not
>>
>> I shared my thoughts about VIRTIO_BALLOON_F_FREE_PAGE_HINT in the other
>> thread and think we should simply not care at all for now.
>>
>>> assume host implementation will be sane?
>>
>> I don't think we should enforce having poison support around. See
my
>> reply to this mail for an alternative.
> 
> OK so you basically say leave this to host to handle? That's more or
> less what I'm saying too.
Yes, for now. We should at some point change the guest to avoid
re-poisoning/zeroing by not using free_page(). For now, I don't think
there is anything broken, it's just not as efficient as it could get at
this point - tolerable as we don't usually expect our guests to poison
pages or per-initialize them with zero.

-- 
Thanks,

David / dhildenb

On Fri, Apr 17, 2020 at 11:07:38AM +0200, David Hildenbrand
wrote:
> On 17.04.20 10:50, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 09:49:03AM +0200, David Hildenbrand wrote:
> >> On 17.04.20 08:28, Michael S. Tsirkin wrote:
> >>> On Thu, Apr 16, 2020 at 04:52:42PM -0700, Alexander Duyck
wrote:
> >>>> On Thu, Apr 16, 2020 at 3:13 PM Michael S. Tsirkin <mst
at redhat.com> wrote:
> >>>>>
> >>>>> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander
Duyck wrote:
> >>>>>> From: Alexander Duyck <alexander.h.duyck at
linux.intel.com>
> >>>>>>
> >>>>>> If we have free page hinting or page reporting
enabled we should disable it
> >>>>>> if the pages are poisoned or initialized on free
and we cannot notify the
> >>>>>> hypervisor.
> >>>>>>
> >>>>>> Fixes: 5d757c8d518d ("virtio-balloon: add
support for providing free page reports to host")
> >>>>>>
> >>>>>> Signed-off-by: Alexander Duyck
<alexander.h.duyck at linux.intel.com>
> >>>>>
> >>>>>
> >>>>> Why not put this logic in the hypervisor?
> >>>>
> >>>> I did that too. This is to cover the case where somebody
is running
> >>>> the code prior to my QEMU changes where the page poison
feature wasn't
> >>>> being enabled.
> >>>
> >>>
> >>> Hmm so that one looks like a QEMU bug (does not expose poison
flag).  In
> >>> the past we just said need to fix the bug where it's found
unless the
> >>> issue is very widespread and major.  Let's assume QEMU
learns to always
> >>> expose POISON with HINT.  Then this configuration (HINT
&& !POISON)
> >>> allows us to detect old QEMU (pre your changes).
> >>
> >> Don't see why this is a QEMU bug. It's just a feature it
does not
> >> implement. Perfectly valid.
> > 
> > I'll need to think about this.
> > We need to remember that the whole HINT thing is not a mandate for
host to
> > corrupt memory. It's just some info about page use guest
> > gives host. If host corrupts memory it's broken ...
> 
> I don't think that's true,
Do you refer to "If host corrupts memory it's broken"?
You think that's not true?
> and that's not what we currently implement in
> the hypervisor for speeding up migration. I still consider
> VIRTIO_BALLOON_F_FREE_PAGE_HINT as an alternative technique of
> temporarily inflating/deflating.
Reporting is like that. But hinting isn't, simply because
by the time host gets the hint page may already be in use.
Blowing it out unconditionally would lead to easily
reproducible guest crashes.
> E.g., we don't migrate any of these
> pages in the hypervisor, so the content will be zeroed out on the
> migration target.
Not exactly true. We do a delicate play with
the two dirty bits so they *are* migrated sometimes.
> If migration fails, the ld content will remain. You
> can call that "corrupting memory" - it's exactly what it has
been
> invented for.
Well no, original author repeatedly stated it's "hinting"
because page can be in use actually.
> 
> IIRC we decided to glue the semantics of VIRTIO_BALLOON_F_PAGE_POISON to
> VIRTIO_BALLOON_F_FREE_PAGE_HINT, which makes in my opinion perfect sense.
> 
> So I propose:
> 
> VIRTIO_BALLOON_F_PAGE_POISON not implemented in the hypervisor:
> - Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will
>   either have the old page content or will be filled with 0.
> - This matches what we currently do.
> 
> VIRTIO_BALLOON_F_PAGE_POISON implemented in the hypervisor:
> - Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will
>   either have the old page content or will be filled with poison_val.
> - This matches what we should do also during ordinary
>   inflation/deflation and free page reporting.
It's a reasonable option. however ...
> Again, nothing is currently broken as we free_page() the pages and
don't
> care about an eventually changed page content.
I don't see how you can say that. ATM on a host without POISON
and with HINT, with poison val != 0 and with validation,
host can blow a free page away and then guest will detect
that as corruption.

If guest crashes then either guest or host are broken ;)



> It's only relevant once
> we ant to change that - and then we can rely on
> VIRTIO_BALLOON_F_PAGE_POISON.
> >>>>
> >>>> The problem is we cannot communicate the full situation to
the
> >>>> hypervisor without the page poison feature being present.
As such I
> >>>> would worry about that backfiring on us due to the
hypervisor acting
> >>>> on incomplete data.
> >>>
> >>>
> >>> I'll try to think about VIRTIO_BALLOON_F_FREE_PAGE_HINT
situation
> >>> over the weekend. But for the new page reporting, why not
> >>
> >> I shared my thoughts about VIRTIO_BALLOON_F_FREE_PAGE_HINT in the
other
> >> thread and think we should simply not care at all for now.
> >>
> >>> assume host implementation will be sane?
> >>
> >> I don't think we should enforce having poison support around.
See my
> >> reply to this mail for an alternative.
> > 
> > OK so you basically say leave this to host to handle? That's more
or
> > less what I'm saying too.
> 
> Yes, for now. We should at some point change the guest to avoid
> re-poisoning/zeroing by not using free_page(). For now, I don't think
> there is anything broken, it's just not as efficient as it could get at
> this point - tolerable as we don't usually expect our guests to poison
> pages or per-initialize them with zero.
> 
> -- 
> Thanks,
> 
> David / dhildenb

>>> I'll need to think about this.
>>> We need to remember that the whole HINT thing is not a mandate for
host to
>>> corrupt memory. It's just some info about page use guest
>>> gives host. If host corrupts memory it's broken ...
>>
>> I don't think that's true,
> 
> Do you refer to "If host corrupts memory it's broken"?
> You think that's not true?
Let me think this through. IMHO it's a "hint with the option for the
hypervisor to assume the content is X and optimize (e.g., not migrate a
page) unless the page is reused before hinting ends". Whereby X is
currently assumed to be 0, correct?

Assume migrated starts, guest hints a page, migration ends. Guest reads
the page again.

a) It could be the original page (still migrated)
b) It could be the zero page (not migrated).

And I think that's a valid corruption of the page content, no?


Now, it's more tricky when we have the following

Migrated starts, guest hints a page, guest reuses the page (e.g., writes
first byte), migration ends. Guest reads the page again.

Here, I (hope) always the original page content is maintained via the
2-bitmap magic.

Or am I missing something important?
> 
>> and that's not what we currently implement in
>> the hypervisor for speeding up migration. I still consider
>> VIRTIO_BALLOON_F_FREE_PAGE_HINT as an alternative technique of
>> temporarily inflating/deflating.
> 
> Reporting is like that. But hinting isn't, simply because
> by the time host gets the hint page may already be in use.
> Blowing it out unconditionally would lead to easily
> reproducible guest crashes.
Agreed that the semantics are different. But "eventually getting a zero
page after migration" was part of the whole invention, no? That's the
whole point of the optimization.
> 
>> E.g., we don't migrate any of these
>> pages in the hypervisor, so the content will be zeroed out on the
>> migration target.
> 
> Not exactly true. We do a delicate play with
> the two dirty bits so they *are* migrated sometimes.
Agreed. Will something like this catch the semantics?

"Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT will
 always have the original page content when written before hinting
 stops. However, if pages are not written before hinting stops, the
 hypervisor might replace them by a zero page."
> 
>> If migration fails, the ld content will remain. You
>> can call that "corrupting memory" - it's exactly what it
has been
>> invented for.
> 
> Well no, original author repeatedly stated it's "hinting"
> because page can be in use actually.
Agreed.
> 
>>
>> IIRC we decided to glue the semantics of VIRTIO_BALLOON_F_PAGE_POISON
to
>> VIRTIO_BALLOON_F_FREE_PAGE_HINT, which makes in my opinion perfect
sense.
>>
>> So I propose:
>>
>> VIRTIO_BALLOON_F_PAGE_POISON not implemented in the hypervisor:
>> - Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will
>>   either have the old page content or will be filled with 0.
>> - This matches what we currently do.
>>
>> VIRTIO_BALLOON_F_PAGE_POISON implemented in the hypervisor:
>> - Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will
>>   either have the old page content or will be filled with poison_val.
>> - This matches what we should do also during ordinary
>>   inflation/deflation and free page reporting.
> 
> It's a reasonable option. however ...
> 
>> Again, nothing is currently broken as we free_page() the pages and
don't
>> care about an eventually changed page content.
> 
> I don't see how you can say that. ATM on a host without POISON
> and with HINT, with poison val != 0 and with validation,
> host can blow a free page away and then guest will detect
> that as corruption.
(At this point I start to hate the whole free page hinting feature again
:D It starts messing with my brain again.)

As soon as we do the free_page(), the page will be written to and
definitely get migrated, right? If the hypervisor would blow out the
page after the free_page(), we would be in trouble. What am I missing?

-- 
Thanks,

David / dhildenb