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

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

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@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@linux.intel.com>
---
 drivers/virtio/virtio_balloon.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 95d9c2f0a7be..08bc86a6e468 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -1110,8 +1110,12 @@ static int virtballoon_validate(struct virtio_device *vdev)
 	/* Tell the host whether we care about poisoned pages. */
 	if (!want_init_on_free() &&
 	    (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||
-	     !page_poisoning_enabled()))
+	     !page_poisoning_enabled())) {
 		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
+	} else if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
+		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
+		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
+	}
 
 	__virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM);
 	return 0;

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

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@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@linux.intel.com>
---
 drivers/virtio/virtio_balloon.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 95d9c2f0a7be..08bc86a6e468 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -1110,8 +1110,12 @@ static int virtballoon_validate(struct virtio_device *vdev)
 	/* Tell the host whether we care about poisoned pages. */
 	if (!want_init_on_free() &&
 	    (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||
-	     !page_poisoning_enabled()))
+	     !page_poisoning_enabled())) {
 		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
+	} else if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
+		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
+		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
+	}
 
 	__virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM);
 	return 0;


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@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@linux.intel.com>


Why not put this logic in the hypervisor?
We don't know what hypervisor uses the hints for.
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.



> ---
>  drivers/virtio/virtio_balloon.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 95d9c2f0a7be..08bc86a6e468 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -1110,8 +1110,12 @@ static int virtballoon_validate(struct virtio_device *vdev)
>  	/* Tell the host whether we care about poisoned pages. */
>  	if (!want_init_on_free() &&
>  	    (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||
> -	     !page_poisoning_enabled()))
> +	     !page_poisoning_enabled())) {
>  		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> +	} else if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
> +	}
>  
>  	__virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM);
>  	return 0;

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

[Michael S. Tsirkin]

On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@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@linux.intel.com>


Why not put this logic in the hypervisor?
We don't know what hypervisor uses the hints for.
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.



> ---
>  drivers/virtio/virtio_balloon.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 95d9c2f0a7be..08bc86a6e468 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -1110,8 +1110,12 @@ static int virtballoon_validate(struct virtio_device *vdev)
>  	/* Tell the host whether we care about poisoned pages. */
>  	if (!want_init_on_free() &&
>  	    (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||
> -	     !page_poisoning_enabled()))
> +	     !page_poisoning_enabled())) {
>  		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> +	} else if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
> +	}
>  
>  	__virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM);
>  	return 0;


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Alexander Duyck]

On Thu, Apr 16, 2020 at 3:13 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@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@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.

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

[Alexander Duyck]

On Thu, Apr 16, 2020 at 3:13 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@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@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.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

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@redhat.com> wrote:
> >
> > On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
> > > From: Alexander Duyck <alexander.h.duyck@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@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

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

[Michael S. Tsirkin]

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@redhat.com> wrote:
> >
> > On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
> > > From: Alexander Duyck <alexander.h.duyck@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@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


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[David Hildenbrand]

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@redhat.com> wrote:
>>>
>>> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
>>>> From: Alexander Duyck <alexander.h.duyck@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@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

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

[David Hildenbrand]

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@redhat.com> wrote:
>>>
>>> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
>>>> From: Alexander Duyck <alexander.h.duyck@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@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


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

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@redhat.com> wrote:
> >>>
> >>> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
> >>>> From: Alexander Duyck <alexander.h.duyck@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@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 ...

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


> -- 
> Thanks,
> 
> David / dhildenb

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

[Michael S. Tsirkin]

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@redhat.com> wrote:
> >>>
> >>> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
> >>>> From: Alexander Duyck <alexander.h.duyck@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@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 ...

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


> -- 
> Thanks,
> 
> David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[David Hildenbrand]

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@redhat.com> wrote:
>>>>>
>>>>> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
>>>>>> From: Alexander Duyck <alexander.h.duyck@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@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

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

[David Hildenbrand]

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@redhat.com> wrote:
>>>>>
>>>>> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
>>>>>> From: Alexander Duyck <alexander.h.duyck@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@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


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

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@redhat.com> wrote:
> >>>>>
> >>>>> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
> >>>>>> From: Alexander Duyck <alexander.h.duyck@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@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

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

[Michael S. Tsirkin]

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@redhat.com> wrote:
> >>>>>
> >>>>> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
> >>>>>> From: Alexander Duyck <alexander.h.duyck@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@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


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[David Hildenbrand]

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

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

[David Hildenbrand]

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


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

On Fri, Apr 17, 2020 at 11:51:31AM +0200, David Hildenbrand wrote:
> 
> >>> 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".

What do you call "hinting ends" though? The fact we put
a page in the VQ is not a guarantee that it's been consumed
by the hypervisor.


I think a strict definition is this:
- hint includes a command ID
- hint implies "page was unused at some point after guest reading command ID"


Hypervisor can use dirty tracking tricks to get from that to
"page is unused at the moment".

> Whereby X is
> currently assumed to be 0, correct?



Now we are talking about what's safe to do with the page.

If POISON flag is set by hypervisor but clear by guest,
or poison_val is 0, then it's clear it's safe to blow
away the page if we can figure out it's unused.

Otherwise, it's much less clear :)



I'll have to come back and re-read the rest next week, this
is complex stuff and I'm too rushed with other things today.

> 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

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

[Michael S. Tsirkin]

On Fri, Apr 17, 2020 at 11:51:31AM +0200, David Hildenbrand wrote:
> 
> >>> 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".

What do you call "hinting ends" though? The fact we put
a page in the VQ is not a guarantee that it's been consumed
by the hypervisor.


I think a strict definition is this:
- hint includes a command ID
- hint implies "page was unused at some point after guest reading command ID"


Hypervisor can use dirty tracking tricks to get from that to
"page is unused at the moment".

> Whereby X is
> currently assumed to be 0, correct?



Now we are talking about what's safe to do with the page.

If POISON flag is set by hypervisor but clear by guest,
or poison_val is 0, then it's clear it's safe to blow
away the page if we can figure out it's unused.

Otherwise, it's much less clear :)



I'll have to come back and re-read the rest next week, this
is complex stuff and I'm too rushed with other things today.

> 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


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[David Hildenbrand]

 > What do you call "hinting ends" though? The fact we put
> a page in the VQ is not a guarantee that it's been consumed
> by the hypervisor.
> 

I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.

> 
> I think a strict definition is this:
> - hint includes a command ID
> - hint implies "page was unused at some point after guest reading command ID"
> 
> 
> Hypervisor can use dirty tracking tricks to get from that to
> "page is unused at the moment".
> 
>> Whereby X is
>> currently assumed to be 0, correct?
> 
> 
> 
> Now we are talking about what's safe to do with the page.
> 
> If POISON flag is set by hypervisor but clear by guest,
> or poison_val is 0, then it's clear it's safe to blow
> away the page if we can figure out it's unused.
> 
> Otherwise, it's much less clear :)

Hah! Agreed :D

> 
> 
> 
> I'll have to come back and re-read the rest next week, this
> is complex stuff and I'm too rushed with other things today.

Yeah, I'm also loaded with other stuff. Maybe Alex has time to
understand the details as well.

-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

 > What do you call "hinting ends" though? The fact we put
> a page in the VQ is not a guarantee that it's been consumed
> by the hypervisor.
> 

I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.

> 
> I think a strict definition is this:
> - hint includes a command ID
> - hint implies "page was unused at some point after guest reading command ID"
> 
> 
> Hypervisor can use dirty tracking tricks to get from that to
> "page is unused at the moment".
> 
>> Whereby X is
>> currently assumed to be 0, correct?
> 
> 
> 
> Now we are talking about what's safe to do with the page.
> 
> If POISON flag is set by hypervisor but clear by guest,
> or poison_val is 0, then it's clear it's safe to blow
> away the page if we can figure out it's unused.
> 
> Otherwise, it's much less clear :)

Hah! Agreed :D

> 
> 
> 
> I'll have to come back and re-read the rest next week, this
> is complex stuff and I'm too rushed with other things today.

Yeah, I'm also loaded with other stuff. Maybe Alex has time to
understand the details as well.

-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
>  > What do you call "hinting ends" though? The fact we put
> > a page in the VQ is not a guarantee that it's been consumed
> > by the hypervisor.
> > 
> 
> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.

Can't find that one anywhere. what did I miss?



> > 
> > I think a strict definition is this:
> > - hint includes a command ID
> > - hint implies "page was unused at some point after guest reading command ID"
> > 
> > 
> > Hypervisor can use dirty tracking tricks to get from that to
> > "page is unused at the moment".
> > 
> >> Whereby X is
> >> currently assumed to be 0, correct?
> > 
> > 
> > 
> > Now we are talking about what's safe to do with the page.
> > 
> > If POISON flag is set by hypervisor but clear by guest,
> > or poison_val is 0, then it's clear it's safe to blow
> > away the page if we can figure out it's unused.
> > 
> > Otherwise, it's much less clear :)
> 
> Hah! Agreed :D
> 
> > 
> > 
> > 
> > I'll have to come back and re-read the rest next week, this
> > is complex stuff and I'm too rushed with other things today.
> 
> Yeah, I'm also loaded with other stuff. Maybe Alex has time to
> understand the details as well.
> 
> -- 
> Thanks,
> 
> David / dhildenb

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

[Michael S. Tsirkin]

On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
>  > What do you call "hinting ends" though? The fact we put
> > a page in the VQ is not a guarantee that it's been consumed
> > by the hypervisor.
> > 
> 
> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.

Can't find that one anywhere. what did I miss?



> > 
> > I think a strict definition is this:
> > - hint includes a command ID
> > - hint implies "page was unused at some point after guest reading command ID"
> > 
> > 
> > Hypervisor can use dirty tracking tricks to get from that to
> > "page is unused at the moment".
> > 
> >> Whereby X is
> >> currently assumed to be 0, correct?
> > 
> > 
> > 
> > Now we are talking about what's safe to do with the page.
> > 
> > If POISON flag is set by hypervisor but clear by guest,
> > or poison_val is 0, then it's clear it's safe to blow
> > away the page if we can figure out it's unused.
> > 
> > Otherwise, it's much less clear :)
> 
> Hah! Agreed :D
> 
> > 
> > 
> > 
> > I'll have to come back and re-read the rest next week, this
> > is complex stuff and I'm too rushed with other things today.
> 
> Yeah, I'm also loaded with other stuff. Maybe Alex has time to
> understand the details as well.
> 
> -- 
> Thanks,
> 
> David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[David Hildenbrand]

On 17.04.20 12:19, Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
>>  > What do you call "hinting ends" though? The fact we put
>>> a page in the VQ is not a guarantee that it's been consumed
>>> by the hypervisor.
>>>
>>
>> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.
> 
> Can't find that one anywhere. what did I miss?

Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is
translated to VIRTIO_BALLOON_CMD_ID_DONE

QEMU:

hw/virtio/virtio-balloon.c:virtio_balloon_free_page_report_notify()
-> virtio_balloon_free_page_done(dev)
-> s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
   virtio_notify_config(vdev);

When the guest reads the config
hw/virtio/virtio-balloon.c:virtio_balloon_get_config()
-> if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE)
-> config.free_page_report_cmd_id = ... VIRTIO_BALLOON_CMD_ID_DONE


Linux:

drivers/virtio/virtio_balloon.c:report_free_page_func()
-> if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
-> return_free_pages_to_mm()


So it's VIRTIO_BALLOON_CMD_ID_DONE.


-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 17.04.20 12:19, Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
>>  > What do you call "hinting ends" though? The fact we put
>>> a page in the VQ is not a guarantee that it's been consumed
>>> by the hypervisor.
>>>
>>
>> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.
> 
> Can't find that one anywhere. what did I miss?

Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is
translated to VIRTIO_BALLOON_CMD_ID_DONE

QEMU:

hw/virtio/virtio-balloon.c:virtio_balloon_free_page_report_notify()
-> virtio_balloon_free_page_done(dev)
-> s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
   virtio_notify_config(vdev);

When the guest reads the config
hw/virtio/virtio-balloon.c:virtio_balloon_get_config()
-> if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE)
-> config.free_page_report_cmd_id = ... VIRTIO_BALLOON_CMD_ID_DONE


Linux:

drivers/virtio/virtio_balloon.c:report_free_page_func()
-> if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
-> return_free_pages_to_mm()


So it's VIRTIO_BALLOON_CMD_ID_DONE.


-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

On Fri, Apr 17, 2020 at 12:26:24PM +0200, David Hildenbrand wrote:
> On 17.04.20 12:19, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
> >>  > What do you call "hinting ends" though? The fact we put
> >>> a page in the VQ is not a guarantee that it's been consumed
> >>> by the hypervisor.
> >>>
> >>
> >> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.
> > 
> > Can't find that one anywhere. what did I miss?
> 
> Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is
> translated to VIRTIO_BALLOON_CMD_ID_DONE

Well VIRTIO_BALLOON_CMD_ID_DONE just means "don't give me any
more hints, I finished migration".
Guest will stop hinting even without that once it scans all
free memory.



> QEMU:
> 
> hw/virtio/virtio-balloon.c:virtio_balloon_free_page_report_notify()
> -> virtio_balloon_free_page_done(dev)
> -> s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
>    virtio_notify_config(vdev);
> 
> When the guest reads the config
> hw/virtio/virtio-balloon.c:virtio_balloon_get_config()
> -> if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE)
> -> config.free_page_report_cmd_id = ... VIRTIO_BALLOON_CMD_ID_DONE
> 
> 
> Linux:
> 
> drivers/virtio/virtio_balloon.c:report_free_page_func()
> -> if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> -> return_free_pages_to_mm()
> 
> 
> So it's VIRTIO_BALLOON_CMD_ID_DONE.
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb

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

[Michael S. Tsirkin]

On Fri, Apr 17, 2020 at 12:26:24PM +0200, David Hildenbrand wrote:
> On 17.04.20 12:19, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
> >>  > What do you call "hinting ends" though? The fact we put
> >>> a page in the VQ is not a guarantee that it's been consumed
> >>> by the hypervisor.
> >>>
> >>
> >> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.
> > 
> > Can't find that one anywhere. what did I miss?
> 
> Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is
> translated to VIRTIO_BALLOON_CMD_ID_DONE

Well VIRTIO_BALLOON_CMD_ID_DONE just means "don't give me any
more hints, I finished migration".
Guest will stop hinting even without that once it scans all
free memory.



> QEMU:
> 
> hw/virtio/virtio-balloon.c:virtio_balloon_free_page_report_notify()
> -> virtio_balloon_free_page_done(dev)
> -> s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
>    virtio_notify_config(vdev);
> 
> When the guest reads the config
> hw/virtio/virtio-balloon.c:virtio_balloon_get_config()
> -> if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE)
> -> config.free_page_report_cmd_id = ... VIRTIO_BALLOON_CMD_ID_DONE
> 
> 
> Linux:
> 
> drivers/virtio/virtio_balloon.c:report_free_page_func()
> -> if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> -> return_free_pages_to_mm()
> 
> 
> So it's VIRTIO_BALLOON_CMD_ID_DONE.
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[David Hildenbrand]

On 17.04.20 12:29, Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2020 at 12:26:24PM +0200, David Hildenbrand wrote:
>> On 17.04.20 12:19, Michael S. Tsirkin wrote:
>>> On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
>>>>  > What do you call "hinting ends" though? The fact we put
>>>>> a page in the VQ is not a guarantee that it's been consumed
>>>>> by the hypervisor.
>>>>>
>>>>
>>>> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.
>>>
>>> Can't find that one anywhere. what did I miss?
>>
>> Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is
>> translated to VIRTIO_BALLOON_CMD_ID_DONE
> 
> Well VIRTIO_BALLOON_CMD_ID_DONE just means "don't give me any
> more hints, I finished migration".
> Guest will stop hinting even without that once it scans all
> free memory.

Yeah, that's the end of the whole process where you can be sure the host
processed all requests definetly.

-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 17.04.20 12:29, Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2020 at 12:26:24PM +0200, David Hildenbrand wrote:
>> On 17.04.20 12:19, Michael S. Tsirkin wrote:
>>> On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
>>>>  > What do you call "hinting ends" though? The fact we put
>>>>> a page in the VQ is not a guarantee that it's been consumed
>>>>> by the hypervisor.
>>>>>
>>>>
>>>> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.
>>>
>>> Can't find that one anywhere. what did I miss?
>>
>> Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is
>> translated to VIRTIO_BALLOON_CMD_ID_DONE
> 
> Well VIRTIO_BALLOON_CMD_ID_DONE just means "don't give me any
> more hints, I finished migration".
> Guest will stop hinting even without that once it scans all
> free memory.

Yeah, that's the end of the whole process where you can be sure the host
processed all requests definetly.

-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

On Fri, Apr 17, 2020 at 12:31:14PM +0200, David Hildenbrand wrote:
> On 17.04.20 12:29, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 12:26:24PM +0200, David Hildenbrand wrote:
> >> On 17.04.20 12:19, Michael S. Tsirkin wrote:
> >>> On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
> >>>>  > What do you call "hinting ends" though? The fact we put
> >>>>> a page in the VQ is not a guarantee that it's been consumed
> >>>>> by the hypervisor.
> >>>>>
> >>>>
> >>>> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.
> >>>
> >>> Can't find that one anywhere. what did I miss?
> >>
> >> Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is
> >> translated to VIRTIO_BALLOON_CMD_ID_DONE
> > 
> > Well VIRTIO_BALLOON_CMD_ID_DONE just means "don't give me any
> > more hints, I finished migration".
> > Guest will stop hinting even without that once it scans all
> > free memory.
> 
> Yeah, that's the end of the whole process where you can be sure the host
> processed all requests definetly.

It's not guaranteed to happen :) Sending an interrupt at the end
of each scan doubles the overhead ...

> -- 
> Thanks,
> 
> David / dhildenb

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

[Michael S. Tsirkin]

On Fri, Apr 17, 2020 at 12:31:14PM +0200, David Hildenbrand wrote:
> On 17.04.20 12:29, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 12:26:24PM +0200, David Hildenbrand wrote:
> >> On 17.04.20 12:19, Michael S. Tsirkin wrote:
> >>> On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
> >>>>  > What do you call "hinting ends" though? The fact we put
> >>>>> a page in the VQ is not a guarantee that it's been consumed
> >>>>> by the hypervisor.
> >>>>>
> >>>>
> >>>> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.
> >>>
> >>> Can't find that one anywhere. what did I miss?
> >>
> >> Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is
> >> translated to VIRTIO_BALLOON_CMD_ID_DONE
> > 
> > Well VIRTIO_BALLOON_CMD_ID_DONE just means "don't give me any
> > more hints, I finished migration".
> > Guest will stop hinting even without that once it scans all
> > free memory.
> 
> Yeah, that's the end of the whole process where you can be sure the host
> processed all requests definetly.

It's not guaranteed to happen :) Sending an interrupt at the end
of each scan doubles the overhead ...

> -- 
> Thanks,
> 
> David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[David Hildenbrand]

On 17.04.20 13:02, Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2020 at 12:31:14PM +0200, David Hildenbrand wrote:
>> On 17.04.20 12:29, Michael S. Tsirkin wrote:
>>> On Fri, Apr 17, 2020 at 12:26:24PM +0200, David Hildenbrand wrote:
>>>> On 17.04.20 12:19, Michael S. Tsirkin wrote:
>>>>> On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
>>>>>>  > What do you call "hinting ends" though? The fact we put
>>>>>>> a page in the VQ is not a guarantee that it's been consumed
>>>>>>> by the hypervisor.
>>>>>>>
>>>>>>
>>>>>> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.
>>>>>
>>>>> Can't find that one anywhere. what did I miss?
>>>>
>>>> Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is
>>>> translated to VIRTIO_BALLOON_CMD_ID_DONE
>>>
>>> Well VIRTIO_BALLOON_CMD_ID_DONE just means "don't give me any
>>> more hints, I finished migration".
>>> Guest will stop hinting even without that once it scans all
>>> free memory.
>>
>> Yeah, that's the end of the whole process where you can be sure the host
>> processed all requests definetly.
> 
> It's not guaranteed to happen :) Sending an interrupt at the end
> of each scan doubles the overhead ...

Yeah, AFAIKs you either get a VIRTIO_BALLOON_CMD_ID_STOP or a
VIRTIO_BALLOON_CMD_ID_DONE at the end. And AFAIK both will guarantee
that all previous hints were handled.

... but reconstructing this protocol from code is probably too much for
a friday afternoon, lol

-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 17.04.20 13:02, Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2020 at 12:31:14PM +0200, David Hildenbrand wrote:
>> On 17.04.20 12:29, Michael S. Tsirkin wrote:
>>> On Fri, Apr 17, 2020 at 12:26:24PM +0200, David Hildenbrand wrote:
>>>> On 17.04.20 12:19, Michael S. Tsirkin wrote:
>>>>> On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
>>>>>>  > What do you call "hinting ends" though? The fact we put
>>>>>>> a page in the VQ is not a guarantee that it's been consumed
>>>>>>> by the hypervisor.
>>>>>>>
>>>>>>
>>>>>> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.
>>>>>
>>>>> Can't find that one anywhere. what did I miss?
>>>>
>>>> Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is
>>>> translated to VIRTIO_BALLOON_CMD_ID_DONE
>>>
>>> Well VIRTIO_BALLOON_CMD_ID_DONE just means "don't give me any
>>> more hints, I finished migration".
>>> Guest will stop hinting even without that once it scans all
>>> free memory.
>>
>> Yeah, that's the end of the whole process where you can be sure the host
>> processed all requests definetly.
> 
> It's not guaranteed to happen :) Sending an interrupt at the end
> of each scan doubles the overhead ...

Yeah, AFAIKs you either get a VIRTIO_BALLOON_CMD_ID_STOP or a
VIRTIO_BALLOON_CMD_ID_DONE at the end. And AFAIK both will guarantee
that all previous hints were handled.

... but reconstructing this protocol from code is probably too much for
a friday afternoon, lol

-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Alexander Duyck]

On Fri, Apr 17, 2020 at 3:09 AM David Hildenbrand <david@redhat.com> wrote:
>
>  > What do you call "hinting ends" though? The fact we put
> > a page in the VQ is not a guarantee that it's been consumed
> > by the hypervisor.
> >
>
> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.

The key bit to this is that there are 4 states, and quasi unlimited
command IDs, although I believe the first 2 are matched up to the
states. So the VIRTIO_BALLOON_CMD_ID_DONE is matched up with
FREE_PAGE_REPORT_S_DONE, and CMD_ID_STOP with S_STOP, but really all
it means is that we are done with the current epoch so we need to
flush the memory and move on. The state is more important to the
hypervisor as it will switch to "STOP" while it is synching the dirty
bits, "REQUESTED" once that has been completed and it will increment
the command ID, "START" once the first hint is received with the
matching command ID, and "DONE" once the migration is complete. As
long as it is in the "START" state and the command ID in the hint
matches it will use the information to clear the dirty bits as it runs
in parallel with the migration task.

The piece I think I was missing was that the balloon is staying
(mostly) inflated until the migration is complete. If that is the case
then I suppose we could leave this enabled at least on the guest side,
assuming the balloon doesn't give back too many pages when it hits the
shrinker.

> >
> > I think a strict definition is this:
> > - hint includes a command ID
> > - hint implies "page was unused at some point after guest reading command ID"
> >
> >
> > Hypervisor can use dirty tracking tricks to get from that to
> > "page is unused at the moment".
> >
> >> Whereby X is
> >> currently assumed to be 0, correct?
> >
> >
> >
> > Now we are talking about what's safe to do with the page.
> >
> > If POISON flag is set by hypervisor but clear by guest,
> > or poison_val is 0, then it's clear it's safe to blow
> > away the page if we can figure out it's unused.
> >
> > Otherwise, it's much less clear :)
>
> Hah! Agreed :D

That isn't quite true. The problem is in the case of hinting it isn't
setting the page to 0. It is simply not migrating it. So if there is
data from an earlier pass it is stuck at that value. So the balloon
will see the poison/init on some pages cleared, however I suppose the
balloon doesn't care about the contents of the page. For the pages
that are leaked back out via the shrinker they will be dirtied so they
will end up being migrated with the correct value eventually.

> > I'll have to come back and re-read the rest next week, this
> > is complex stuff and I'm too rushed with other things today.
>
> Yeah, I'm also loaded with other stuff. Maybe Alex has time to
> understand the details as well.

So after looking it over again it makes a bit more sense why this
hasn't caused any issues so far, and I can see why the poison enabled
setup and hinting can work. The problem is I am left wondering what
assumptions we are allowed to leave in place.

1. Can we assume that we don't care about the contents in the pages in
the balloon changing?
2. Can we assume that the guest will always rewrite the page after the
deflate in the case of init_on_free or poison?
3. Can we assume that free page hinting will always function as a
balloon setup, so no moving it over to a page reporting type setup?

If we assume the above 3 items then there isn't any point in worrying
about poison when it comes to free page hinting. It doesn't make sense
to since they essentially negate it. As such I could go through this
patch and the QEMU patches and clean up any associations since the to
are not really tied together in any way.

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

[Alexander Duyck]

On Fri, Apr 17, 2020 at 3:09 AM David Hildenbrand <david@redhat.com> wrote:
>
>  > What do you call "hinting ends" though? The fact we put
> > a page in the VQ is not a guarantee that it's been consumed
> > by the hypervisor.
> >
>
> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.

The key bit to this is that there are 4 states, and quasi unlimited
command IDs, although I believe the first 2 are matched up to the
states. So the VIRTIO_BALLOON_CMD_ID_DONE is matched up with
FREE_PAGE_REPORT_S_DONE, and CMD_ID_STOP with S_STOP, but really all
it means is that we are done with the current epoch so we need to
flush the memory and move on. The state is more important to the
hypervisor as it will switch to "STOP" while it is synching the dirty
bits, "REQUESTED" once that has been completed and it will increment
the command ID, "START" once the first hint is received with the
matching command ID, and "DONE" once the migration is complete. As
long as it is in the "START" state and the command ID in the hint
matches it will use the information to clear the dirty bits as it runs
in parallel with the migration task.

The piece I think I was missing was that the balloon is staying
(mostly) inflated until the migration is complete. If that is the case
then I suppose we could leave this enabled at least on the guest side,
assuming the balloon doesn't give back too many pages when it hits the
shrinker.

> >
> > I think a strict definition is this:
> > - hint includes a command ID
> > - hint implies "page was unused at some point after guest reading command ID"
> >
> >
> > Hypervisor can use dirty tracking tricks to get from that to
> > "page is unused at the moment".
> >
> >> Whereby X is
> >> currently assumed to be 0, correct?
> >
> >
> >
> > Now we are talking about what's safe to do with the page.
> >
> > If POISON flag is set by hypervisor but clear by guest,
> > or poison_val is 0, then it's clear it's safe to blow
> > away the page if we can figure out it's unused.
> >
> > Otherwise, it's much less clear :)
>
> Hah! Agreed :D

That isn't quite true. The problem is in the case of hinting it isn't
setting the page to 0. It is simply not migrating it. So if there is
data from an earlier pass it is stuck at that value. So the balloon
will see the poison/init on some pages cleared, however I suppose the
balloon doesn't care about the contents of the page. For the pages
that are leaked back out via the shrinker they will be dirtied so they
will end up being migrated with the correct value eventually.

> > I'll have to come back and re-read the rest next week, this
> > is complex stuff and I'm too rushed with other things today.
>
> Yeah, I'm also loaded with other stuff. Maybe Alex has time to
> understand the details as well.

So after looking it over again it makes a bit more sense why this
hasn't caused any issues so far, and I can see why the poison enabled
setup and hinting can work. The problem is I am left wondering what
assumptions we are allowed to leave in place.

1. Can we assume that we don't care about the contents in the pages in
the balloon changing?
2. Can we assume that the guest will always rewrite the page after the
deflate in the case of init_on_free or poison?
3. Can we assume that free page hinting will always function as a
balloon setup, so no moving it over to a page reporting type setup?

If we assume the above 3 items then there isn't any point in worrying
about poison when it comes to free page hinting. It doesn't make sense
to since they essentially negate it. As such I could go through this
patch and the QEMU patches and clean up any associations since the to
are not really tied together in any way.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[David Hildenbrand]

>>> Now we are talking about what's safe to do with the page.
>>>
>>> If POISON flag is set by hypervisor but clear by guest,
>>> or poison_val is 0, then it's clear it's safe to blow
>>> away the page if we can figure out it's unused.
>>>
>>> Otherwise, it's much less clear :)
>>
>> Hah! Agreed :D
> 
> That isn't quite true. The problem is in the case of hinting it isn't
> setting the page to 0. It is simply not migrating it. So if there is
> data from an earlier pass it is stuck at that value. So the balloon
> will see the poison/init on some pages cleared, however I suppose the
> balloon doesn't care about the contents of the page. For the pages
> that are leaked back out via the shrinker they will be dirtied so they
> will end up being migrated with the correct value eventually.

Right, I think current Linux guests are fine. The critical piece we are
talking about is

1) Guest balloon allocates and hints a page
2) Hypervisor does not process hinting request
3) Guest frees the page and reuses it (especially, writes it).
4) Hypervisor processes the hinting request.

AFAIU, as soon as the guest writes the page (e.g., zeroes it/poisons it
in the buddy, or somebody who allocated it), the page *will* get
migrated, even if 4) happens after 3). That's guaranteed by the 2-bitmap
magic.



Now, assume the following happens (in some future Linux version) (due to
your "simply not migrating it" comment):

1) Guest balloon allocates and hints a page. Assume the page is zero due
to want_init_on_free().
2) Hypervisor processes the hinting request.
3) Guest frees the page. Assume we are implementing some magic to "skip"
zeroing, as we assume it is still zero.

Due to 2), the page won't get migrated. In 3) we expect the page to be
0. QEMU would have to make sure that we always get either the original,
or a zero page on the destination. Otherwise, this smells like data
corruption.


> 
>>> I'll have to come back and re-read the rest next week, this
>>> is complex stuff and I'm too rushed with other things today.
>>
>> Yeah, I'm also loaded with other stuff. Maybe Alex has time to
>> understand the details as well.
> 
> So after looking it over again it makes a bit more sense why this
> hasn't caused any issues so far, and I can see why the poison enabled
> setup and hinting can work. The problem is I am left wondering what
> assumptions we are allowed to leave in place.
> 
> 1. Can we assume that we don't care about the contents in the pages in
> the balloon changing?

I think, we should define valid ways for the hypervisor to change it.

"Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by
a 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."

I think the debatable part is "whereby the unwritten parts might either
be from the old, or from the zero page". AFAIU, you think it could
happen in QEMU, that we have neither the old, nor the zero page, but
instead some previous content. The question is if that's valid, or if
that's a BUG in QEMU. If it's valid, we can do no optimizations in the
guest (e.g., skip zeroing in the buddy). And I agree that this smells
like "data corruption" as Michael said.


> 2. Can we assume that the guest will always rewrite the page after the
> deflate in the case of init_on_free or poison?

Depends on what we think is the right way to do - IOW if we think "some
other content" as mentioned above is a BUG or not.

> 3. Can we assume that free page hinting will always function as a
> balloon setup, so no moving it over to a page reporting type setup?

I think we have to define the valid semantics. That implies what would
be valid to do with it. Unfortunately, we have to reverse-engineer here.

> 
> If we assume the above 3 items then there isn't any point in worrying
> about poison when it comes to free page hinting. It doesn't make sense
> to since they essentially negate it. As such I could go through this
> patch and the QEMU patches and clean up any associations since the to
> are not really tied together in any way.

The big question is, if we want to support VIRTIO_BALLOON_F_PAGE_POISON
with free page hinting. e.g.,:

"Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by
a page full of X. 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 a page filled with X. Without
VIRTIO_BALLOON_F_PAGE_POISON, X is zero, otherwise it is poison_val."

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.

-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

>>> Now we are talking about what's safe to do with the page.
>>>
>>> If POISON flag is set by hypervisor but clear by guest,
>>> or poison_val is 0, then it's clear it's safe to blow
>>> away the page if we can figure out it's unused.
>>>
>>> Otherwise, it's much less clear :)
>>
>> Hah! Agreed :D
> 
> That isn't quite true. The problem is in the case of hinting it isn't
> setting the page to 0. It is simply not migrating it. So if there is
> data from an earlier pass it is stuck at that value. So the balloon
> will see the poison/init on some pages cleared, however I suppose the
> balloon doesn't care about the contents of the page. For the pages
> that are leaked back out via the shrinker they will be dirtied so they
> will end up being migrated with the correct value eventually.

Right, I think current Linux guests are fine. The critical piece we are
talking about is

1) Guest balloon allocates and hints a page
2) Hypervisor does not process hinting request
3) Guest frees the page and reuses it (especially, writes it).
4) Hypervisor processes the hinting request.

AFAIU, as soon as the guest writes the page (e.g., zeroes it/poisons it
in the buddy, or somebody who allocated it), the page *will* get
migrated, even if 4) happens after 3). That's guaranteed by the 2-bitmap
magic.



Now, assume the following happens (in some future Linux version) (due to
your "simply not migrating it" comment):

1) Guest balloon allocates and hints a page. Assume the page is zero due
to want_init_on_free().
2) Hypervisor processes the hinting request.
3) Guest frees the page. Assume we are implementing some magic to "skip"
zeroing, as we assume it is still zero.

Due to 2), the page won't get migrated. In 3) we expect the page to be
0. QEMU would have to make sure that we always get either the original,
or a zero page on the destination. Otherwise, this smells like data
corruption.


> 
>>> I'll have to come back and re-read the rest next week, this
>>> is complex stuff and I'm too rushed with other things today.
>>
>> Yeah, I'm also loaded with other stuff. Maybe Alex has time to
>> understand the details as well.
> 
> So after looking it over again it makes a bit more sense why this
> hasn't caused any issues so far, and I can see why the poison enabled
> setup and hinting can work. The problem is I am left wondering what
> assumptions we are allowed to leave in place.
> 
> 1. Can we assume that we don't care about the contents in the pages in
> the balloon changing?

I think, we should define valid ways for the hypervisor to change it.

"Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by
a 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."

I think the debatable part is "whereby the unwritten parts might either
be from the old, or from the zero page". AFAIU, you think it could
happen in QEMU, that we have neither the old, nor the zero page, but
instead some previous content. The question is if that's valid, or if
that's a BUG in QEMU. If it's valid, we can do no optimizations in the
guest (e.g., skip zeroing in the buddy). And I agree that this smells
like "data corruption" as Michael said.


> 2. Can we assume that the guest will always rewrite the page after the
> deflate in the case of init_on_free or poison?

Depends on what we think is the right way to do - IOW if we think "some
other content" as mentioned above is a BUG or not.

> 3. Can we assume that free page hinting will always function as a
> balloon setup, so no moving it over to a page reporting type setup?

I think we have to define the valid semantics. That implies what would
be valid to do with it. Unfortunately, we have to reverse-engineer here.

> 
> If we assume the above 3 items then there isn't any point in worrying
> about poison when it comes to free page hinting. It doesn't make sense
> to since they essentially negate it. As such I could go through this
> patch and the QEMU patches and clean up any associations since the to
> are not really tied together in any way.

The big question is, if we want to support VIRTIO_BALLOON_F_PAGE_POISON
with free page hinting. e.g.,:

"Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by
a page full of X. 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 a page filled with X. Without
VIRTIO_BALLOON_F_PAGE_POISON, X is zero, otherwise it is poison_val."

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.

-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Alexander Duyck]

On Mon, Apr 20, 2020 at 6:28 AM David Hildenbrand <david@redhat.com> wrote:
>
> >>> Now we are talking about what's safe to do with the page.
> >>>
> >>> If POISON flag is set by hypervisor but clear by guest,
> >>> or poison_val is 0, then it's clear it's safe to blow
> >>> away the page if we can figure out it's unused.
> >>>
> >>> Otherwise, it's much less clear :)
> >>
> >> Hah! Agreed :D
> >
> > That isn't quite true. The problem is in the case of hinting it isn't
> > setting the page to 0. It is simply not migrating it. So if there is
> > data from an earlier pass it is stuck at that value. So the balloon
> > will see the poison/init on some pages cleared, however I suppose the
> > balloon doesn't care about the contents of the page. For the pages
> > that are leaked back out via the shrinker they will be dirtied so they
> > will end up being migrated with the correct value eventually.
>
> Right, I think current Linux guests are fine. The critical piece we are
> talking about is
>
> 1) Guest balloon allocates and hints a page
> 2) Hypervisor does not process hinting request
> 3) Guest frees the page and reuses it (especially, writes it).
> 4) Hypervisor processes the hinting request.
>
> AFAIU, as soon as the guest writes the page (e.g., zeroes it/poisons it
> in the buddy, or somebody who allocated it), the page *will* get
> migrated, even if 4) happens after 3). That's guaranteed by the 2-bitmap
> magic.

Yes. Basically the page will be flagged as dirty as soon as it is
written to, and that will be dealt with after the current batch of
hints are processed so we should be able to guarantee that the page
will have a coherent value stored in it.

> Now, assume the following happens (in some future Linux version) (due to
> your "simply not migrating it" comment):
>
> 1) Guest balloon allocates and hints a page. Assume the page is zero due
> to want_init_on_free().
> 2) Hypervisor processes the hinting request.
> 3) Guest frees the page. Assume we are implementing some magic to "skip"
> zeroing, as we assume it is still zero.
>
> Due to 2), the page won't get migrated. In 3) we expect the page to be
> 0. QEMU would have to make sure that we always get either the original,
> or a zero page on the destination. Otherwise, this smells like data
> corruption.

I agree and that is my concern. From the time the page is hinted until
it is written to again it is in an indeterminate state. However with
the current Linux guest implementation it is being written to so
technically there isn't an issue. We would just need to make sure it
stays that way.

In addition it is already an exposed interface and this is the way it
works. I think if anything we are likely forced to document it as-is
and guarantee that the behavior is not changed.

> >
> >>> I'll have to come back and re-read the rest next week, this
> >>> is complex stuff and I'm too rushed with other things today.
> >>
> >> Yeah, I'm also loaded with other stuff. Maybe Alex has time to
> >> understand the details as well.
> >
> > So after looking it over again it makes a bit more sense why this
> > hasn't caused any issues so far, and I can see why the poison enabled
> > setup and hinting can work. The problem is I am left wondering what
> > assumptions we are allowed to leave in place.
> >
> > 1. Can we assume that we don't care about the contents in the pages in
> > the balloon changing?
>
> I think, we should define valid ways for the hypervisor to change it.
>
> "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by
> a 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."

Actually I don't think the zero page is accurate. A page that is
hinted basically implies we don't care about the content. I would
think that we could treat a hinted page as uninitialized memory.

> I think the debatable part is "whereby the unwritten parts might either
> be from the old, or from the zero page". AFAIU, you think it could
> happen in QEMU, that we have neither the old, nor the zero page, but
> instead some previous content. The question is if that's valid, or if
> that's a BUG in QEMU. If it's valid, we can do no optimizations in the
> guest (e.g., skip zeroing in the buddy). And I agree that this smells
> like "data corruption" as Michael said.

So if any part of the page is written to after it is hinted you will
be getting the modified page since it goes back to being "dirty". All
the hinting is doing is skipping the migration of dirty pages that are
"hinted" as long as they are not written to again. So the pages are
valid up until we migrate to the new system. At that point all of the
pages that are in the page hinting balloon will be stale data as we
skipped migrating them. Assuming something like page poisoning or
init_on_free are enabled they will be poisoned again when they are
transferred from the balloon back into the page allocator.

> > 2. Can we assume that the guest will always rewrite the page after the
> > deflate in the case of init_on_free or poison?
>
> Depends on what we think is the right way to do - IOW if we think "some
> other content" as mentioned above is a BUG or not.

So I wouldn't consider it a but as the zero page probably doesn't
apply. We are basically just indicating we don't care about the
contents, we aren't giving it a value. At least that is how I see it
working based on how it was implemented.

> > 3. Can we assume that free page hinting will always function as a
> > balloon setup, so no moving it over to a page reporting type setup?
>
> I think we have to define the valid semantics. That implies what would
> be valid to do with it. Unfortunately, we have to reverse-engineer here.

So at this point a "hinted" page is a page that can essentially
transition to uninitialized while it is sitting in the balloon. I
suspect that is how we are going to have to treat it since that is the
behavior that it has right now.

> >
> > If we assume the above 3 items then there isn't any point in worrying
> > about poison when it comes to free page hinting. It doesn't make sense
> > to since they essentially negate it. As such I could go through this
> > patch and the QEMU patches and clean up any associations since the to
> > are not really tied together in any way.
>
> The big question is, if we want to support VIRTIO_BALLOON_F_PAGE_POISON
> with free page hinting. e.g.,:
>
> "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by
> a page full of X. 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 a page filled with X. Without
> VIRTIO_BALLOON_F_PAGE_POISON, X is zero, otherwise it is poison_val."

So the problem is, as implemented, none of the above is correct.
Basically what we get with VIRTIO_BALLOON_F_FREE_PAGE_HINT is either
no migration, or migration of some old stale state if the page made it
into the balloon. There is a chance X could be 0 in the non-migration
case as I believe that is going to be the default starting point for
memory, however it is just as likely that the page will have stale
data if it is in use during any of the bitmap sync passes. The problem
is the original code didn't take the poison flag or value into
account, so I think we are stuck treating a hinted page as
uninitialized memory as long as it is in the balloon.

With the VIRTIO_BALLOON_F_PAGE_POISON we could make it so that when
the page comes out of the balloon it is either unmodified or it is
poison_val. Without the VIRTIO_BALLOON_F_PAGE_POISON feature present
you cannot make that guarantee and are stuck with the page being
treated as either unmodified or uninitialized memory.

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

The second approach is to work on defining it such that
VIRTIO_BALLOON_F_PAGE_POISON switches us to the unmodified/poison_val
definition. However the side effect of that is that for now having the
flag set means that essentially page hinting is disabled until we
could come up with a way of guaranteeing the poison_val effect which
would be additional work. In addition it has to rely on the fact that
the existing guest solutions reinitialize the pages when they come out
of the balloon since the pre-5.7 driver was broken and only took
poison into account and didn't check on init_on_free. It would likely
be the much more error prone approach and create significantly more
work.

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

[Alexander Duyck]

On Mon, Apr 20, 2020 at 6:28 AM David Hildenbrand <david@redhat.com> wrote:
>
> >>> Now we are talking about what's safe to do with the page.
> >>>
> >>> If POISON flag is set by hypervisor but clear by guest,
> >>> or poison_val is 0, then it's clear it's safe to blow
> >>> away the page if we can figure out it's unused.
> >>>
> >>> Otherwise, it's much less clear :)
> >>
> >> Hah! Agreed :D
> >
> > That isn't quite true. The problem is in the case of hinting it isn't
> > setting the page to 0. It is simply not migrating it. So if there is
> > data from an earlier pass it is stuck at that value. So the balloon
> > will see the poison/init on some pages cleared, however I suppose the
> > balloon doesn't care about the contents of the page. For the pages
> > that are leaked back out via the shrinker they will be dirtied so they
> > will end up being migrated with the correct value eventually.
>
> Right, I think current Linux guests are fine. The critical piece we are
> talking about is
>
> 1) Guest balloon allocates and hints a page
> 2) Hypervisor does not process hinting request
> 3) Guest frees the page and reuses it (especially, writes it).
> 4) Hypervisor processes the hinting request.
>
> AFAIU, as soon as the guest writes the page (e.g., zeroes it/poisons it
> in the buddy, or somebody who allocated it), the page *will* get
> migrated, even if 4) happens after 3). That's guaranteed by the 2-bitmap
> magic.

Yes. Basically the page will be flagged as dirty as soon as it is
written to, and that will be dealt with after the current batch of
hints are processed so we should be able to guarantee that the page
will have a coherent value stored in it.

> Now, assume the following happens (in some future Linux version) (due to
> your "simply not migrating it" comment):
>
> 1) Guest balloon allocates and hints a page. Assume the page is zero due
> to want_init_on_free().
> 2) Hypervisor processes the hinting request.
> 3) Guest frees the page. Assume we are implementing some magic to "skip"
> zeroing, as we assume it is still zero.
>
> Due to 2), the page won't get migrated. In 3) we expect the page to be
> 0. QEMU would have to make sure that we always get either the original,
> or a zero page on the destination. Otherwise, this smells like data
> corruption.

I agree and that is my concern. From the time the page is hinted until
it is written to again it is in an indeterminate state. However with
the current Linux guest implementation it is being written to so
technically there isn't an issue. We would just need to make sure it
stays that way.

In addition it is already an exposed interface and this is the way it
works. I think if anything we are likely forced to document it as-is
and guarantee that the behavior is not changed.

> >
> >>> I'll have to come back and re-read the rest next week, this
> >>> is complex stuff and I'm too rushed with other things today.
> >>
> >> Yeah, I'm also loaded with other stuff. Maybe Alex has time to
> >> understand the details as well.
> >
> > So after looking it over again it makes a bit more sense why this
> > hasn't caused any issues so far, and I can see why the poison enabled
> > setup and hinting can work. The problem is I am left wondering what
> > assumptions we are allowed to leave in place.
> >
> > 1. Can we assume that we don't care about the contents in the pages in
> > the balloon changing?
>
> I think, we should define valid ways for the hypervisor to change it.
>
> "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by
> a 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."

Actually I don't think the zero page is accurate. A page that is
hinted basically implies we don't care about the content. I would
think that we could treat a hinted page as uninitialized memory.

> I think the debatable part is "whereby the unwritten parts might either
> be from the old, or from the zero page". AFAIU, you think it could
> happen in QEMU, that we have neither the old, nor the zero page, but
> instead some previous content. The question is if that's valid, or if
> that's a BUG in QEMU. If it's valid, we can do no optimizations in the
> guest (e.g., skip zeroing in the buddy). And I agree that this smells
> like "data corruption" as Michael said.

So if any part of the page is written to after it is hinted you will
be getting the modified page since it goes back to being "dirty". All
the hinting is doing is skipping the migration of dirty pages that are
"hinted" as long as they are not written to again. So the pages are
valid up until we migrate to the new system. At that point all of the
pages that are in the page hinting balloon will be stale data as we
skipped migrating them. Assuming something like page poisoning or
init_on_free are enabled they will be poisoned again when they are
transferred from the balloon back into the page allocator.

> > 2. Can we assume that the guest will always rewrite the page after the
> > deflate in the case of init_on_free or poison?
>
> Depends on what we think is the right way to do - IOW if we think "some
> other content" as mentioned above is a BUG or not.

So I wouldn't consider it a but as the zero page probably doesn't
apply. We are basically just indicating we don't care about the
contents, we aren't giving it a value. At least that is how I see it
working based on how it was implemented.

> > 3. Can we assume that free page hinting will always function as a
> > balloon setup, so no moving it over to a page reporting type setup?
>
> I think we have to define the valid semantics. That implies what would
> be valid to do with it. Unfortunately, we have to reverse-engineer here.

So at this point a "hinted" page is a page that can essentially
transition to uninitialized while it is sitting in the balloon. I
suspect that is how we are going to have to treat it since that is the
behavior that it has right now.

> >
> > If we assume the above 3 items then there isn't any point in worrying
> > about poison when it comes to free page hinting. It doesn't make sense
> > to since they essentially negate it. As such I could go through this
> > patch and the QEMU patches and clean up any associations since the to
> > are not really tied together in any way.
>
> The big question is, if we want to support VIRTIO_BALLOON_F_PAGE_POISON
> with free page hinting. e.g.,:
>
> "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by
> a page full of X. 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 a page filled with X. Without
> VIRTIO_BALLOON_F_PAGE_POISON, X is zero, otherwise it is poison_val."

So the problem is, as implemented, none of the above is correct.
Basically what we get with VIRTIO_BALLOON_F_FREE_PAGE_HINT is either
no migration, or migration of some old stale state if the page made it
into the balloon. There is a chance X could be 0 in the non-migration
case as I believe that is going to be the default starting point for
memory, however it is just as likely that the page will have stale
data if it is in use during any of the bitmap sync passes. The problem
is the original code didn't take the poison flag or value into
account, so I think we are stuck treating a hinted page as
uninitialized memory as long as it is in the balloon.

With the VIRTIO_BALLOON_F_PAGE_POISON we could make it so that when
the page comes out of the balloon it is either unmodified or it is
poison_val. Without the VIRTIO_BALLOON_F_PAGE_POISON feature present
you cannot make that guarantee and are stuck with the page being
treated as either unmodified or uninitialized memory.

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

The second approach is to work on defining it such that
VIRTIO_BALLOON_F_PAGE_POISON switches us to the unmodified/poison_val
definition. However the side effect of that is that for now having the
flag set means that essentially page hinting is disabled until we
could come up with a way of guaranteeing the poison_val effect which
would be additional work. In addition it has to rely on the fact that
the existing guest solutions reinitialize the pages when they come out
of the balloon since the pre-5.7 driver was broken and only took
poison into account and didn't check on init_on_free. It would likely
be the much more error prone approach and create significantly more
work.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[David Hildenbrand]

 >>> 2. Can we assume that the guest will always rewrite the page after the
>>> deflate in the case of init_on_free or poison?
>>
>> Depends on what we think is the right way to do - IOW if we think "some
>> other content" as mentioned above is a BUG or not.
> 
> So I wouldn't consider it a but as the zero page probably doesn't
> apply. We are basically just indicating we don't care about the
> contents, we aren't giving it a value. At least that is how I see it
> working based on how it was implemented.
> 
>>> 3. Can we assume that free page hinting will always function as a
>>> balloon setup, so no moving it over to a page reporting type setup?
>>
>> I think we have to define the valid semantics. That implies what would
>> be valid to do with it. Unfortunately, we have to reverse-engineer here.
> 
> So at this point a "hinted" page is a page that can essentially
> transition to uninitialized while it is sitting in the balloon. I
> suspect that is how we are going to have to treat it since that is the
> behavior that it has right now.

At least it's not what Michael initially thought should be done - IIRC.

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

So the question remains: Does QEMU have a BUG or do we actually allow to
corrupt guest memory.

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.

2. "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT are considered
unused and will contain an undefined/uninitialized content once hinted.
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. The
guest should reinitialized the full page in case it cares about the
actual content (e.g., page poisoning)."


I tend to favor 2 - although it basically leaves no room for future
improvement regarding skipping re-initialization in the guest after
migration.

>>>
>>> If we assume the above 3 items then there isn't any point in worrying
>>> about poison when it comes to free page hinting. It doesn't make sense
>>> to since they essentially negate it. As such I could go through this
>>> patch and the QEMU patches and clean up any associations since the to
>>> are not really tied together in any way.
>>
>> The big question is, if we want to support VIRTIO_BALLOON_F_PAGE_POISON
>> with free page hinting. e.g.,:
>>
>> "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by
>> a page full of X. 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 a page filled with X. Without
>> VIRTIO_BALLOON_F_PAGE_POISON, X is zero, otherwise it is poison_val."

[...]

> 
> With the VIRTIO_BALLOON_F_PAGE_POISON we could make it so that when
> the page comes out of the balloon it is either unmodified or it is
> poison_val. Without the VIRTIO_BALLOON_F_PAGE_POISON feature present
> you cannot make that guarantee and are stuck with the page being
> treated as either unmodified or uninitialized memory.

While it would be possible, I doubt it will be easy to implement, and I
still wonder if we should really care about optimizing an undocumented
feature that takes three people to figure out the semantics.

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

Let's see if Michael has another opinion.

-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

 >>> 2. Can we assume that the guest will always rewrite the page after the
>>> deflate in the case of init_on_free or poison?
>>
>> Depends on what we think is the right way to do - IOW if we think "some
>> other content" as mentioned above is a BUG or not.
> 
> So I wouldn't consider it a but as the zero page probably doesn't
> apply. We are basically just indicating we don't care about the
> contents, we aren't giving it a value. At least that is how I see it
> working based on how it was implemented.
> 
>>> 3. Can we assume that free page hinting will always function as a
>>> balloon setup, so no moving it over to a page reporting type setup?
>>
>> I think we have to define the valid semantics. That implies what would
>> be valid to do with it. Unfortunately, we have to reverse-engineer here.
> 
> So at this point a "hinted" page is a page that can essentially
> transition to uninitialized while it is sitting in the balloon. I
> suspect that is how we are going to have to treat it since that is the
> behavior that it has right now.

At least it's not what Michael initially thought should be done - IIRC.

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

So the question remains: Does QEMU have a BUG or do we actually allow to
corrupt guest memory.

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.

2. "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT are considered
unused and will contain an undefined/uninitialized content once hinted.
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. The
guest should reinitialized the full page in case it cares about the
actual content (e.g., page poisoning)."


I tend to favor 2 - although it basically leaves no room for future
improvement regarding skipping re-initialization in the guest after
migration.

>>>
>>> If we assume the above 3 items then there isn't any point in worrying
>>> about poison when it comes to free page hinting. It doesn't make sense
>>> to since they essentially negate it. As such I could go through this
>>> patch and the QEMU patches and clean up any associations since the to
>>> are not really tied together in any way.
>>
>> The big question is, if we want to support VIRTIO_BALLOON_F_PAGE_POISON
>> with free page hinting. e.g.,:
>>
>> "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by
>> a page full of X. 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 a page filled with X. Without
>> VIRTIO_BALLOON_F_PAGE_POISON, X is zero, otherwise it is poison_val."

[...]

> 
> With the VIRTIO_BALLOON_F_PAGE_POISON we could make it so that when
> the page comes out of the balloon it is either unmodified or it is
> poison_val. Without the VIRTIO_BALLOON_F_PAGE_POISON feature present
> you cannot make that guarantee and are stuck with the page being
> treated as either unmodified or uninitialized memory.

While it would be possible, I doubt it will be easy to implement, and I
still wonder if we should really care about optimizing an undocumented
feature that takes three people to figure out the semantics.

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

Let's see if Michael has another opinion.

-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Alexander Duyck]

On Tue, Apr 21, 2020 at 12:29 AM David Hildenbrand <david@redhat.com> wrote:
>
>  >>> 2. Can we assume that the guest will always rewrite the page after the
> >>> deflate in the case of init_on_free or poison?
> >>
> >> Depends on what we think is the right way to do - IOW if we think "some
> >> other content" as mentioned above is a BUG or not.
> >
> > So I wouldn't consider it a but as the zero page probably doesn't
> > apply. We are basically just indicating we don't care about the
> > contents, we aren't giving it a value. At least that is how I see it
> > working based on how it was implemented.
> >
> >>> 3. Can we assume that free page hinting will always function as a
> >>> balloon setup, so no moving it over to a page reporting type setup?
> >>
> >> I think we have to define the valid semantics. That implies what would
> >> be valid to do with it. Unfortunately, we have to reverse-engineer here.
> >
> > So at this point a "hinted" page is a page that can essentially
> > transition to uninitialized while it is sitting in the balloon. I
> > suspect that is how we are going to have to treat it since that is the
> > behavior that it has right now.
>
> At least it's not what Michael initially thought should be done - IIRC.
>
> "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 ...").
>
> So the question remains: Does QEMU have a BUG or do we actually allow to
> corrupt guest memory.
>
> 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.

The caveat for the hinting is that if the page is modified from the
point it is placed on the ring the dirty flag will be enforced for it
and will not be skipped as it will be captured in the next bitmap
sync.

> 2. "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT are considered
> unused and will contain an undefined/uninitialized content once hinted.
> 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. The
> guest should reinitialized the full page in case it cares about the
> actual content (e.g., page poisoning)."
>
>
> I tend to favor 2 - although it basically leaves no room for future
> improvement regarding skipping re-initialization in the guest after
> migration.

I agree. The main advantage would be that we get to keep all of the
existing functionality and wouldn't have to shut things down for
poison being enabled. However we are limited in that any future design
won't be able to skip over having to have the guest re-poison the
pages.

However making changes to behave more like 1 would break existing use
cases since right now page poisoning can be enabled and the guest can
make it work. If we refactored QEMU to make 1 work then we would lose
that.

> >>>
> >>> If we assume the above 3 items then there isn't any point in worrying
> >>> about poison when it comes to free page hinting. It doesn't make sense
> >>> to since they essentially negate it. As such I could go through this
> >>> patch and the QEMU patches and clean up any associations since the to
> >>> are not really tied together in any way.
> >>
> >> The big question is, if we want to support VIRTIO_BALLOON_F_PAGE_POISON
> >> with free page hinting. e.g.,:
> >>
> >> "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by
> >> a page full of X. 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 a page filled with X. Without
> >> VIRTIO_BALLOON_F_PAGE_POISON, X is zero, otherwise it is poison_val."
>
> [...]
>
> >
> > With the VIRTIO_BALLOON_F_PAGE_POISON we could make it so that when
> > the page comes out of the balloon it is either unmodified or it is
> > poison_val. Without the VIRTIO_BALLOON_F_PAGE_POISON feature present
> > you cannot make that guarantee and are stuck with the page being
> > treated as either unmodified or uninitialized memory.
>
> While it would be possible, I doubt it will be easy to implement, and I
> still wonder if we should really care about optimizing an undocumented
> feature that takes three people to figure out the semantics.

Agreed. That is why I am thinking I will just disassociate
F_PAGE_POISON from the page hinting entirely since QEMU never had the
two implemented together.

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

> Let's see if Michael has another opinion.

Agreed.

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

[Alexander Duyck]

On Tue, Apr 21, 2020 at 12:29 AM David Hildenbrand <david@redhat.com> wrote:
>
>  >>> 2. Can we assume that the guest will always rewrite the page after the
> >>> deflate in the case of init_on_free or poison?
> >>
> >> Depends on what we think is the right way to do - IOW if we think "some
> >> other content" as mentioned above is a BUG or not.
> >
> > So I wouldn't consider it a but as the zero page probably doesn't
> > apply. We are basically just indicating we don't care about the
> > contents, we aren't giving it a value. At least that is how I see it
> > working based on how it was implemented.
> >
> >>> 3. Can we assume that free page hinting will always function as a
> >>> balloon setup, so no moving it over to a page reporting type setup?
> >>
> >> I think we have to define the valid semantics. That implies what would
> >> be valid to do with it. Unfortunately, we have to reverse-engineer here.
> >
> > So at this point a "hinted" page is a page that can essentially
> > transition to uninitialized while it is sitting in the balloon. I
> > suspect that is how we are going to have to treat it since that is the
> > behavior that it has right now.
>
> At least it's not what Michael initially thought should be done - IIRC.
>
> "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 ...").
>
> So the question remains: Does QEMU have a BUG or do we actually allow to
> corrupt guest memory.
>
> 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.

The caveat for the hinting is that if the page is modified from the
point it is placed on the ring the dirty flag will be enforced for it
and will not be skipped as it will be captured in the next bitmap
sync.

> 2. "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT are considered
> unused and will contain an undefined/uninitialized content once hinted.
> 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. The
> guest should reinitialized the full page in case it cares about the
> actual content (e.g., page poisoning)."
>
>
> I tend to favor 2 - although it basically leaves no room for future
> improvement regarding skipping re-initialization in the guest after
> migration.

I agree. The main advantage would be that we get to keep all of the
existing functionality and wouldn't have to shut things down for
poison being enabled. However we are limited in that any future design
won't be able to skip over having to have the guest re-poison the
pages.

However making changes to behave more like 1 would break existing use
cases since right now page poisoning can be enabled and the guest can
make it work. If we refactored QEMU to make 1 work then we would lose
that.

> >>>
> >>> If we assume the above 3 items then there isn't any point in worrying
> >>> about poison when it comes to free page hinting. It doesn't make sense
> >>> to since they essentially negate it. As such I could go through this
> >>> patch and the QEMU patches and clean up any associations since the to
> >>> are not really tied together in any way.
> >>
> >> The big question is, if we want to support VIRTIO_BALLOON_F_PAGE_POISON
> >> with free page hinting. e.g.,:
> >>
> >> "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by
> >> a page full of X. 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 a page filled with X. Without
> >> VIRTIO_BALLOON_F_PAGE_POISON, X is zero, otherwise it is poison_val."
>
> [...]
>
> >
> > With the VIRTIO_BALLOON_F_PAGE_POISON we could make it so that when
> > the page comes out of the balloon it is either unmodified or it is
> > poison_val. Without the VIRTIO_BALLOON_F_PAGE_POISON feature present
> > you cannot make that guarantee and are stuck with the page being
> > treated as either unmodified or uninitialized memory.
>
> While it would be possible, I doubt it will be easy to implement, and I
> still wonder if we should really care about optimizing an undocumented
> feature that takes three people to figure out the semantics.

Agreed. That is why I am thinking I will just disassociate
F_PAGE_POISON from the page hinting entirely since QEMU never had the
two implemented together.

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

> Let's see if Michael has another opinion.

Agreed.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[David Hildenbrand]

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

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

[David Hildenbrand]

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


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Alexander Duyck]

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.

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

[Alexander Duyck]

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.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[David Hildenbrand]

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

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

[David Hildenbrand]

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


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Alexander Duyck]

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.

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

[Alexander Duyck]

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.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[David Hildenbrand]

On 16.04.20 21:30, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@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.
> 

Please split that up into an actual fix (Fixes:) for free page reporting
and an optimization for free page hinting. Also, please document why we
are doing stuff.

Regarding the free page reporting part, I propose something like this instead


diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0ef16566c3f3..9b1e56da1e29 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -1107,6 +1107,20 @@ static int virtballoon_restore(struct virtio_device *vdev)
 
 static int virtballoon_validate(struct virtio_device *vdev)
 {
+       /*
+        * Free page reporting relies on the fact that any access after
+        * pages were reported (esp. from the buddy) will result in them getting
+        * deflated automatically. In case we care about the page content, we
+        * need support from the hypervisor to provide us with the same page
+        * (poisoned) content we originally had in the page. Without
+        * VIRTIO_BALLOON_F_PAGE_POISON, we will receive either the original
+        * page or a zeroed page.
+        */
+       if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON) &&
+           !IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) &&
+           page_poisoning_enabled())
+               __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
+
        /* Tell the host whether we care about poisoned pages. */
        if (!want_init_on_free() &&
            (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||


> Fixes: 5d757c8d518d ("virtio-balloon: add support for providing free page reports to host")
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  drivers/virtio/virtio_balloon.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 95d9c2f0a7be..08bc86a6e468 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -1110,8 +1110,12 @@ static int virtballoon_validate(struct virtio_device *vdev)
>  	/* Tell the host whether we care about poisoned pages. */
>  	if (!want_init_on_free() &&
>  	    (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||
> -	     !page_poisoning_enabled()))
> +	     !page_poisoning_enabled())) {
>  		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> +	} else if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
> +	}
>  
>  	__virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM);
>  	return 0;
> 


-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 16.04.20 21:30, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@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.
> 

Please split that up into an actual fix (Fixes:) for free page reporting
and an optimization for free page hinting. Also, please document why we
are doing stuff.

Regarding the free page reporting part, I propose something like this instead


diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0ef16566c3f3..9b1e56da1e29 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -1107,6 +1107,20 @@ static int virtballoon_restore(struct virtio_device *vdev)
 
 static int virtballoon_validate(struct virtio_device *vdev)
 {
+       /*
+        * Free page reporting relies on the fact that any access after
+        * pages were reported (esp. from the buddy) will result in them getting
+        * deflated automatically. In case we care about the page content, we
+        * need support from the hypervisor to provide us with the same page
+        * (poisoned) content we originally had in the page. Without
+        * VIRTIO_BALLOON_F_PAGE_POISON, we will receive either the original
+        * page or a zeroed page.
+        */
+       if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON) &&
+           !IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) &&
+           page_poisoning_enabled())
+               __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
+
        /* Tell the host whether we care about poisoned pages. */
        if (!want_init_on_free() &&
            (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||


> Fixes: 5d757c8d518d ("virtio-balloon: add support for providing free page reports to host")
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  drivers/virtio/virtio_balloon.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 95d9c2f0a7be..08bc86a6e468 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -1110,8 +1110,12 @@ static int virtballoon_validate(struct virtio_device *vdev)
>  	/* Tell the host whether we care about poisoned pages. */
>  	if (!want_init_on_free() &&
>  	    (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||
> -	     !page_poisoning_enabled()))
> +	     !page_poisoning_enabled())) {
>  		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> +	} else if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
> +	}
>  
>  	__virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM);
>  	return 0;
> 


-- 
Thanks,

David / dhildenb


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org