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

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.
> We don't know what hypervisor uses the hints for.
I agree, but at the same time the way the feature was originally coded
it was only checked if the FREE_PAGE_HINT feature was enabled. The
assumption there is that if we have page poison data and want to use
hints we need to report it. In my mind if we ever want to switch over
to the page reporting style setup for page hinting in the future we
will need to have it behave in a sane manner. So disabling it if we
have a poison value we need to report, but have no mechanism to report
it makes sense to me.

The actual likelihood of us encountering this case should be pretty
low anyway since it is not that common to have page poisoning or
init_on_free enabled.
> Yes you can not just drop them but you can maybe do
> other things such as MADV_SOFT_OFFLINE.
>
> Finally, VIRTIO_BALLOON_F_FREE_PAGE_HINT does nothing
> at all unless guest gets the command from hypervisor,
> so there isn't even any overhead.
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.

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

I am also interested in the following question:
Is this good for anything? In particular are there any bugs in HINT
we can fix using this hack?

E.g. I know HINT does not obey MUST_TELL_HOST under OOM.  But that
unfortunately is a guest not a host bug, so this hack does not seem
useful.

> > We don't know what hypervisor uses the hints for.
> 
> I agree, but at the same time the way the feature was originally coded
> it was only checked if the FREE_PAGE_HINT feature was enabled. The
> assumption there is that if we have page poison data and want to use
> hints we need to report it. In my mind if we ever want to switch over
> to the page reporting style setup for page hinting in the future we
> will need to have it behave in a sane manner. So disabling it if we
> have a poison value we need to report, but have no mechanism to report
> it makes sense to me.
> 
> The actual likelihood of us encountering this case should be pretty
> low anyway since it is not that common to have page poisoning or
> init_on_free enabled.
> 
> > Yes you can not just drop them but you can maybe do
> > other things such as MADV_SOFT_OFFLINE.
> >
> > Finally, VIRTIO_BALLOON_F_FREE_PAGE_HINT does nothing
> > at all unless guest gets the command from hypervisor,
> > so there isn't even any overhead.
> 
> 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
assume host implementation will be sane?

-- 
MST

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.

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

-- 
Thanks,

David / dhildenb