linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] virtio_balloon: fix shrinker scan number of pages
@ 2019-11-19 10:17 Michael S. Tsirkin
  2019-11-19 11:39 ` David Hildenbrand
  2019-11-20  0:48 ` Khazhismel Kumykov
  0 siblings, 2 replies; 4+ messages in thread
From: Michael S. Tsirkin @ 2019-11-19 10:17 UTC (permalink / raw)
  To: linux-kernel
  Cc: stable, Khazhismel Kumykov, Wei Wang, Jason Wang, virtualization

virtio_balloon_shrinker_scan should return number of system pages freed,
but because it's calling functions that deal with balloon pages, it gets
confused and sometimes returns the number of balloon pages.

It does not matter practically as the exact number isn't
used, but it seems better to be consistent in case someone
starts using this API.

Further, if we ever tried to iteratively leak pages as
virtio_balloon_shrinker_scan tries to do, we'd run into issues - this is
because freed_pages was accumulating total freed pages, but was also
subtracted on each iteration from pages_to_free, which can result in
either leaking less memory than we were supposed to free, or or more if
pages_to_free underruns.

On a system with 4K pages we are lucky that we are never asked to leak
more than 128 pages while we can leak up to 256 at a time,
but it looks like a real issue for systems with page size != 4K.

Cc: stable@vger.kernel.org
Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier with shrinker")
Reported-by: Khazhismel Kumykov <khazhy@google.com>
Reviewed-by: Wei Wang <wei.w.wang@intel.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_balloon.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 226fbb995fb0..7cee05cdf3fb 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -772,6 +772,13 @@ static unsigned long shrink_free_pages(struct virtio_balloon *vb,
 	return blocks_freed << VIRTIO_BALLOON_FREE_PAGE_ORDER;
 }
 
+static unsigned long leak_balloon_pages(struct virtio_balloon *vb,
+                                          unsigned long pages_to_free)
+{
+	return leak_balloon(vb, pages_to_free * VIRTIO_BALLOON_PAGES_PER_PAGE) /
+		VIRTIO_BALLOON_PAGES_PER_PAGE;
+}
+
 static unsigned long shrink_balloon_pages(struct virtio_balloon *vb,
 					  unsigned long pages_to_free)
 {
@@ -782,11 +789,9 @@ static unsigned long shrink_balloon_pages(struct virtio_balloon *vb,
 	 * VIRTIO_BALLOON_ARRAY_PFNS_MAX balloon pages, so we call it
 	 * multiple times to deflate pages till reaching pages_to_free.
 	 */
-	while (vb->num_pages && pages_to_free) {
-		pages_freed += leak_balloon(vb, pages_to_free) /
-					VIRTIO_BALLOON_PAGES_PER_PAGE;
-		pages_to_free -= pages_freed;
-	}
+	while (vb->num_pages && pages_freed < pages_to_free)
+		pages_freed += leak_balloon_pages(vb, pages_to_free);
+
 	update_balloon_size(vb);
 
 	return pages_freed;
@@ -799,7 +804,7 @@ static unsigned long virtio_balloon_shrinker_scan(struct shrinker *shrinker,
 	struct virtio_balloon *vb = container_of(shrinker,
 					struct virtio_balloon, shrinker);
 
-	pages_to_free = sc->nr_to_scan * VIRTIO_BALLOON_PAGES_PER_PAGE;
+	pages_to_free = sc->nr_to_scan;
 
 	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		pages_freed = shrink_free_pages(vb, pages_to_free);
-- 
MST


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] virtio_balloon: fix shrinker scan number of pages
  2019-11-19 10:17 [PATCH] virtio_balloon: fix shrinker scan number of pages Michael S. Tsirkin
@ 2019-11-19 11:39 ` David Hildenbrand
  2019-11-19 13:03   ` Michael S. Tsirkin
  2019-11-20  0:48 ` Khazhismel Kumykov
  1 sibling, 1 reply; 4+ messages in thread
From: David Hildenbrand @ 2019-11-19 11:39 UTC (permalink / raw)
  To: Michael S. Tsirkin, linux-kernel
  Cc: stable, Khazhismel Kumykov, Wei Wang, Jason Wang, virtualization

On 19.11.19 11:17, Michael S. Tsirkin wrote:
> virtio_balloon_shrinker_scan should return number of system pages freed,
> but because it's calling functions that deal with balloon pages, it gets
> confused and sometimes returns the number of balloon pages.
> 
> It does not matter practically as the exact number isn't
> used, but it seems better to be consistent in case someone
> starts using this API.

If it doesn't matter, why cc: stable?

> 
> Further, if we ever tried to iteratively leak pages as
> virtio_balloon_shrinker_scan tries to do, we'd run into issues - this is
> because freed_pages was accumulating total freed pages, but was also
> subtracted on each iteration from pages_to_free, which can result in
> either leaking less memory than we were supposed to free, or or more if
> pages_to_free underruns.
> 
> On a system with 4K pages we are lucky that we are never asked to leak
> more than 128 pages while we can leak up to 256 at a time,
> but it looks like a real issue for systems with page size != 4K.
> 
> Cc: stable@vger.kernel.org
> Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier with shrinker")
> Reported-by: Khazhismel Kumykov <khazhy@google.com>
> Reviewed-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   drivers/virtio/virtio_balloon.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 226fbb995fb0..7cee05cdf3fb 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -772,6 +772,13 @@ static unsigned long shrink_free_pages(struct virtio_balloon *vb,
>   	return blocks_freed << VIRTIO_BALLOON_FREE_PAGE_ORDER;
>   }
>   
> +static unsigned long leak_balloon_pages(struct virtio_balloon *vb,
> +                                          unsigned long pages_to_free)
> +{
> +	return leak_balloon(vb, pages_to_free * VIRTIO_BALLOON_PAGES_PER_PAGE) /
> +		VIRTIO_BALLOON_PAGES_PER_PAGE;
> +}
> +
>   static unsigned long shrink_balloon_pages(struct virtio_balloon *vb,
>   					  unsigned long pages_to_free)
>   {
> @@ -782,11 +789,9 @@ static unsigned long shrink_balloon_pages(struct virtio_balloon *vb,
>   	 * VIRTIO_BALLOON_ARRAY_PFNS_MAX balloon pages, so we call it
>   	 * multiple times to deflate pages till reaching pages_to_free.
>   	 */
> -	while (vb->num_pages && pages_to_free) {
> -		pages_freed += leak_balloon(vb, pages_to_free) /
> -					VIRTIO_BALLOON_PAGES_PER_PAGE;
> -		pages_to_free -= pages_freed;
> -	}
> +	while (vb->num_pages && pages_freed < pages_to_free)
> +		pages_freed += leak_balloon_pages(vb, pages_to_free);
> +
>   	update_balloon_size(vb);
>   
>   	return pages_freed;
> @@ -799,7 +804,7 @@ static unsigned long virtio_balloon_shrinker_scan(struct shrinker *shrinker,
>   	struct virtio_balloon *vb = container_of(shrinker,
>   					struct virtio_balloon, shrinker);
>   
> -	pages_to_free = sc->nr_to_scan * VIRTIO_BALLOON_PAGES_PER_PAGE;
> +	pages_to_free = sc->nr_to_scan;
>   
>   	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
>   		pages_freed = shrink_free_pages(vb, pages_to_free);
> 


-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] virtio_balloon: fix shrinker scan number of pages
  2019-11-19 11:39 ` David Hildenbrand
@ 2019-11-19 13:03   ` Michael S. Tsirkin
  0 siblings, 0 replies; 4+ messages in thread
From: Michael S. Tsirkin @ 2019-11-19 13:03 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-kernel, stable, Khazhismel Kumykov, Wei Wang, Jason Wang,
	virtualization

On Tue, Nov 19, 2019 at 12:39:51PM +0100, David Hildenbrand wrote:
> On 19.11.19 11:17, Michael S. Tsirkin wrote:
> > virtio_balloon_shrinker_scan should return number of system pages freed,
> > but because it's calling functions that deal with balloon pages, it gets
> > confused and sometimes returns the number of balloon pages.
> > 
> > It does not matter practically as the exact number isn't
> > used, but it seems better to be consistent in case someone
> > starts using this API.
> 
> If it doesn't matter, why cc: stable?

Oh right. Sorry.

> > 
> > Further, if we ever tried to iteratively leak pages as
> > virtio_balloon_shrinker_scan tries to do, we'd run into issues - this is
> > because freed_pages was accumulating total freed pages, but was also
> > subtracted on each iteration from pages_to_free, which can result in
> > either leaking less memory than we were supposed to free, or or more if
> > pages_to_free underruns.
> > 
> > On a system with 4K pages we are lucky that we are never asked to leak
> > more than 128 pages while we can leak up to 256 at a time,
> > but it looks like a real issue for systems with page size != 4K.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier with shrinker")
> > Reported-by: Khazhismel Kumykov <khazhy@google.com>
> > Reviewed-by: Wei Wang <wei.w.wang@intel.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >   drivers/virtio/virtio_balloon.c | 17 +++++++++++------
> >   1 file changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index 226fbb995fb0..7cee05cdf3fb 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -772,6 +772,13 @@ static unsigned long shrink_free_pages(struct virtio_balloon *vb,
> >   	return blocks_freed << VIRTIO_BALLOON_FREE_PAGE_ORDER;
> >   }
> > +static unsigned long leak_balloon_pages(struct virtio_balloon *vb,
> > +                                          unsigned long pages_to_free)
> > +{
> > +	return leak_balloon(vb, pages_to_free * VIRTIO_BALLOON_PAGES_PER_PAGE) /
> > +		VIRTIO_BALLOON_PAGES_PER_PAGE;
> > +}
> > +
> >   static unsigned long shrink_balloon_pages(struct virtio_balloon *vb,
> >   					  unsigned long pages_to_free)
> >   {
> > @@ -782,11 +789,9 @@ static unsigned long shrink_balloon_pages(struct virtio_balloon *vb,
> >   	 * VIRTIO_BALLOON_ARRAY_PFNS_MAX balloon pages, so we call it
> >   	 * multiple times to deflate pages till reaching pages_to_free.
> >   	 */
> > -	while (vb->num_pages && pages_to_free) {
> > -		pages_freed += leak_balloon(vb, pages_to_free) /
> > -					VIRTIO_BALLOON_PAGES_PER_PAGE;
> > -		pages_to_free -= pages_freed;
> > -	}
> > +	while (vb->num_pages && pages_freed < pages_to_free)
> > +		pages_freed += leak_balloon_pages(vb, pages_to_free);
> > +
> >   	update_balloon_size(vb);
> >   	return pages_freed;
> > @@ -799,7 +804,7 @@ static unsigned long virtio_balloon_shrinker_scan(struct shrinker *shrinker,
> >   	struct virtio_balloon *vb = container_of(shrinker,
> >   					struct virtio_balloon, shrinker);
> > -	pages_to_free = sc->nr_to_scan * VIRTIO_BALLOON_PAGES_PER_PAGE;
> > +	pages_to_free = sc->nr_to_scan;
> >   	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> >   		pages_freed = shrink_free_pages(vb, pages_to_free);
> > 
> 
> 
> -- 
> 
> Thanks,
> 
> David / dhildenb


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] virtio_balloon: fix shrinker scan number of pages
  2019-11-19 10:17 [PATCH] virtio_balloon: fix shrinker scan number of pages Michael S. Tsirkin
  2019-11-19 11:39 ` David Hildenbrand
@ 2019-11-20  0:48 ` Khazhismel Kumykov
  1 sibling, 0 replies; 4+ messages in thread
From: Khazhismel Kumykov @ 2019-11-20  0:48 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Linux Kernel Mailing List, stable, Wei Wang, Jason Wang, virtualization

[-- Attachment #1: Type: text/plain, Size: 3457 bytes --]

On Tue, Nov 19, 2019 at 2:17 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> virtio_balloon_shrinker_scan should return number of system pages freed,
> but because it's calling functions that deal with balloon pages, it gets
> confused and sometimes returns the number of balloon pages.
>
> It does not matter practically as the exact number isn't
> used, but it seems better to be consistent in case someone
> starts using this API.
>
> Further, if we ever tried to iteratively leak pages as
> virtio_balloon_shrinker_scan tries to do, we'd run into issues - this is
> because freed_pages was accumulating total freed pages, but was also
> subtracted on each iteration from pages_to_free, which can result in
> either leaking less memory than we were supposed to free, or or more if
> pages_to_free underruns.
>
> On a system with 4K pages we are lucky that we are never asked to leak
> more than 128 pages while we can leak up to 256 at a time,
> but it looks like a real issue for systems with page size != 4K.
>
> Cc: stable@vger.kernel.org
> Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier with shrinker")
> Reported-by: Khazhismel Kumykov <khazhy@google.com>
> Reviewed-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/virtio/virtio_balloon.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 226fbb995fb0..7cee05cdf3fb 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -772,6 +772,13 @@ static unsigned long shrink_free_pages(struct virtio_balloon *vb,
>         return blocks_freed << VIRTIO_BALLOON_FREE_PAGE_ORDER;
>  }
>
> +static unsigned long leak_balloon_pages(struct virtio_balloon *vb,
> +                                          unsigned long pages_to_free)
> +{
> +       return leak_balloon(vb, pages_to_free * VIRTIO_BALLOON_PAGES_PER_PAGE) /
> +               VIRTIO_BALLOON_PAGES_PER_PAGE;
> +}
> +
>  static unsigned long shrink_balloon_pages(struct virtio_balloon *vb,
>                                           unsigned long pages_to_free)
>  {
> @@ -782,11 +789,9 @@ static unsigned long shrink_balloon_pages(struct virtio_balloon *vb,
>          * VIRTIO_BALLOON_ARRAY_PFNS_MAX balloon pages, so we call it
>          * multiple times to deflate pages till reaching pages_to_free.
>          */
> -       while (vb->num_pages && pages_to_free) {
> -               pages_freed += leak_balloon(vb, pages_to_free) /
> -                                       VIRTIO_BALLOON_PAGES_PER_PAGE;
> -               pages_to_free -= pages_freed;
> -       }
> +       while (vb->num_pages && pages_freed < pages_to_free)
> +               pages_freed += leak_balloon_pages(vb, pages_to_free);

pages_to_free - pages_freed ?

> +
>         update_balloon_size(vb);
>
>         return pages_freed;
> @@ -799,7 +804,7 @@ static unsigned long virtio_balloon_shrinker_scan(struct shrinker *shrinker,
>         struct virtio_balloon *vb = container_of(shrinker,
>                                         struct virtio_balloon, shrinker);
>
> -       pages_to_free = sc->nr_to_scan * VIRTIO_BALLOON_PAGES_PER_PAGE;
> +       pages_to_free = sc->nr_to_scan;
>
>         if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
>                 pages_freed = shrink_free_pages(vb, pages_to_free);
> --
> MST
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4843 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-11-20  0:48 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-19 10:17 [PATCH] virtio_balloon: fix shrinker scan number of pages Michael S. Tsirkin
2019-11-19 11:39 ` David Hildenbrand
2019-11-19 13:03   ` Michael S. Tsirkin
2019-11-20  0:48 ` Khazhismel Kumykov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).