Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation

[Michael S. Tsirkin]

Question is, does all this cause any bugs?

I'm not against cleaning this up, not at all, but we need to know the
impact.

On Fri, Nov 15, 2019 at 02:55:57PM -0800, Khazhismel Kumykov wrote:
> To my reading, we're accumulating total freed pages in pages_freed, but
> subtracting it every iteration from pages_to_free, meaning we'll count
> earlier iterations multiple times, freeing fewer pages than expected.
> Just accumulate in pages_freed, and compare to pages_to_free.

For nr to scan: yes we scan less objects but that can only happen
if the first pass does not free enough. And 1st pass can pass
256 entries, and my reading of code in do_shrink_slab
is that it passes only as much as
#define SHRINK_BATCH 128

so it looks like this never triggers in practice - right?


> 
> There's also a unit mismatch,

So two unrelated issues, I think we want two patches.


> where pages_to_free seems to be virtio
> balloon pages, and pages_freed is system pages (We divide by
> VIRTIO_BALLOON_PAGES_PER_PAGE), so sutracting pages_freed from
> pages_to_free may result in freeing too much.

I am inclining to say we should pass in page units.
Free page reporting is all done in units of MAX_ORDER - 1.
Let's not ptopagate the crazy virtio page thing - we hopefully
will add a saner interface to regular balloon too.

something like the below?

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 226fbb995fb0..128440826b55 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -783,8 +783,8 @@ static unsigned long shrink_balloon_pages(struct virtio_balloon *vb,
 	 * 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_freed += leak_balloon(vb, pages_to_free *
+					    VIRTIO_BALLOON_PAGES_PER_PAGE);
 		pages_to_free -= pages_freed;
 	}
 	update_balloon_size(vb);
@@ -799,7 +799,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);


> There also seems to be a mismatch between shrink_free_pages() and
> shrink_balloon_pages(), where in both pages_to_free is given as # of
> virtio pages to free, but free_pages() returns virtio pages, and
> balloon_pages returns system pages.
> 
> (For 4K PAGE_SIZE, this mismatch wouldn't be noticed since
> VIRTIO_BALLOON_PAGES_PER_PAGE would be 1)

About return value:
The only
use for count_objects I see is:
      freeable = shrinker->count_objects(shrinker, shrinkctl);
      if (freeable == 0 || freeable == SHRINK_EMPTY)
                return freeable;

so units do not matter here.


For scan objects, IIUC they are eventually propagated to
shrink_slab. That in turn is called at two sites.
One ignores the returned value. The other does:


        do {
                struct mem_cgroup *memcg = NULL;

                freed = 0;
                memcg = mem_cgroup_iter(NULL, NULL, NULL);
                do {
                        freed += shrink_slab(GFP_KERNEL, nid, memcg, 0);
                } while ((memcg = mem_cgroup_iter(NULL, memcg, NULL)) != NULL);
        } while (freed > 10);

so returning a larger than real value because of
double accounting will just make more calls to the scan
function.




> Have both return virtio pages, and divide into system pages when
> returning from shrinker_scan()
> 
> Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier with shrinker")
> Cc: Wei Wang <wei.w.wang@xxxxxxxxx>
> Signed-off-by: Khazhismel Kumykov <khazhy@xxxxxxxxxx>
> ---
> 
> Tested this under memory pressure conditions and the shrinker seemed to
> shrink.

And to clarify, did you manage to detect it malfunctioning without the
patch?


>  drivers/virtio/virtio_balloon.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 226fbb995fb0..7951ece3fe24 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -782,11 +782,8 @@ 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_to_free > pages_freed)
> +		pages_freed += leak_balloon(vb, pages_to_free - pages_freed);
>  	update_balloon_size(vb);
>  
>  	return pages_freed;
> @@ -805,11 +802,11 @@ static unsigned long virtio_balloon_shrinker_scan(struct shrinker *shrinker,
>  		pages_freed = shrink_free_pages(vb, pages_to_free);
>  
>  	if (pages_freed >= pages_to_free)
> -		return pages_freed;
> +		return pages_freed / VIRTIO_BALLOON_PAGES_PER_PAGE;
>  
>  	pages_freed += shrink_balloon_pages(vb, pages_to_free - pages_freed);
>  
> -	return pages_freed;
> +	return pages_freed / VIRTIO_BALLOON_PAGES_PER_PAGE;
>  }
>  
>  static unsigned long virtio_balloon_shrinker_count(struct shrinker *shrinker,
> -- 
> 2.24.0.432.g9d3f5f5b63-goog