Alexander Duyck
2020-Apr-16 19:30 UTC
[PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
From: Alexander Duyck <alexander.h.duyck at linux.intel.com> If we have free page hinting or page reporting enabled we should disable it if the pages are poisoned or initialized on free and we cannot notify the hypervisor. Fixes: 5d757c8d518d ("virtio-balloon: add support for providing free page reports to host") Signed-off-by: Alexander Duyck <alexander.h.duyck at linux.intel.com> --- 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;
Michael S. Tsirkin
2020-Apr-16 22:13 UTC
[PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:> From: Alexander Duyck <alexander.h.duyck at linux.intel.com> > > If we have free page hinting or page reporting enabled we should disable it > if the pages are poisoned or initialized on free and we cannot notify the > hypervisor. > > Fixes: 5d757c8d518d ("virtio-balloon: add support for providing free page reports to host") > > Signed-off-by: Alexander Duyck <alexander.h.duyck at linux.intel.com>Why not put this logic in the hypervisor? 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;
Alexander Duyck
2020-Apr-16 23:52 UTC
[virtio-dev] Re: [PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
On Thu, Apr 16, 2020 at 3:13 PM Michael S. Tsirkin <mst at redhat.com> wrote:> > On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote: > > From: Alexander Duyck <alexander.h.duyck at linux.intel.com> > > > > If we have free page hinting or page reporting enabled we should disable it > > if the pages are poisoned or initialized on free and we cannot notify the > > hypervisor. > > > > Fixes: 5d757c8d518d ("virtio-balloon: add support for providing free page reports to host") > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck at linux.intel.com> > > > Why not put this logic in the hypervisor?I did that too. This is to cover the case where somebody is running the code prior to my QEMU changes where the page poison feature wasn't being enabled.> We don't know what hypervisor uses the hints for.I agree, but at the same time the way the feature was originally coded it was only checked if the FREE_PAGE_HINT feature was enabled. The assumption there is that if we have page poison data and want to use hints we need to report it. In my mind if we ever want to switch over to the page reporting style setup for page hinting in the future we will need to have it behave in a sane manner. So disabling it if we have a poison value we need to report, but have no mechanism to report it makes sense to me. The actual likelihood of us encountering this case should be pretty low anyway since it is not that common to have page poisoning or init_on_free enabled.> Yes you can not just drop them but you can maybe do > other things such as MADV_SOFT_OFFLINE. > > Finally, VIRTIO_BALLOON_F_FREE_PAGE_HINT does nothing > at all unless guest gets the command from hypervisor, > so there isn't even any overhead.The problem is we cannot communicate the full situation to the hypervisor without the page poison feature being present. As such I would worry about that backfiring on us due to the hypervisor acting on incomplete data.
David Hildenbrand
2020-Apr-17 07:46 UTC
[PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
On 16.04.20 21:30, Alexander Duyck wrote:> From: Alexander Duyck <alexander.h.duyck at linux.intel.com> > > If we have free page hinting or page reporting enabled we should disable it > if the pages are poisoned or initialized on free and we cannot notify the > hypervisor. >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 at 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
Reasonably Related Threads
- [PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
- [PATCH v2] virtio-balloon: Disable free page reporting if page poison reporting is not enabled
- [PATCH v2] virtio-balloon: Disable free page reporting if page poison reporting is not enabled
- [PATCH v2 resubmit] virtio-balloon: Disable free page reporting if page poison reporting is not enabled
- [PATCH v2 resubmit] virtio-balloon: Disable free page reporting if page poison reporting is not enabled