[PATCH] virtio_balloon: fix shrinker pages_to_free calculation

[PATCH] virtio_balloon: fix shrinker pages_to_free calculation

[Khazhismel Kumykov]

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.

There's also a unit mismatch, 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.

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)

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@intel.com>
Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
---

Tested this under memory pressure conditions and the shrinker seemed to
shrink.

 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

Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation

[Wei Wang]

On 11/16/2019 06:55 AM, 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.

Not sure about the above. But the following unit mismatch is a good 
capture, thanks!

>
> There's also a unit mismatch, 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.
>
> 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)
>
> Have both return virtio pages, and divide into system pages when
> returning from shrinker_scan()

Sounds good.

>
> Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier with shrinker")
> Cc: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
> ---
>
> Tested this under memory pressure conditions and the shrinker seemed to
> shrink.
>
>   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);

We also need a fix here then:

pages_freed = shrink_free_pages(vb, sc->nr_to_scan) * 
VIRTIO_BALLOON_PAGES_PER_PAGE;


Btw, there is another mistake, in virtio_balloon_shrinker_count:

-       count += vb->num_free_page_blocks >> VIRTIO_BALLOON_FREE_PAGE_ORDER;
+      count += vb->num_free_page_blocks << VIRTIO_BALLOON_FREE_PAGE_ORDER;

You may want to include it in this fix patch as well.

Best,
Wei

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@intel.com>
> Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
> ---
> 
> 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

Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation

[Michael S. Tsirkin]

On Mon, Nov 18, 2019 at 12:01:08PM +0800, Wei Wang wrote:
> On 11/16/2019 06:55 AM, 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.
> 
> Not sure about the above. But the following unit mismatch is a good capture,
> thanks!
> 
> > 
> > There's also a unit mismatch, 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.
> > 
> > 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)
> > 
> > Have both return virtio pages, and divide into system pages when
> > returning from shrinker_scan()
> 
> Sounds good.
> 
> > 
> > Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier with shrinker")
> > Cc: Wei Wang <wei.w.wang@intel.com>
> > Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
> > ---
> > 
> > Tested this under memory pressure conditions and the shrinker seemed to
> > shrink.
> > 
> >   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);
> 
> We also need a fix here then:
> 
> pages_freed = shrink_free_pages(vb, sc->nr_to_scan) *
> VIRTIO_BALLOON_PAGES_PER_PAGE;

No let's do accounting in pages please. virtio page is a legacy
thing we just did not fix it in time to get rid of it by now.

> 
> Btw, there is another mistake, in virtio_balloon_shrinker_count:
> 
> -       count += vb->num_free_page_blocks >> VIRTIO_BALLOON_FREE_PAGE_ORDER;
> +      count += vb->num_free_page_blocks << VIRTIO_BALLOON_FREE_PAGE_ORDER;
> 
> You may want to include it in this fix patch as well.

OMG. should be a separate patch.
But really this just shows why shifts are such a bad idea.

Let's define
VIRTIO_BALLOON_PAGES_PER_FREE_PAGE

and use it with * and / consistently instead of shifts.


> Best,
> Wei
> 

Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation

[Wei Wang]

On 11/18/2019 01:30 PM, Michael S. Tsirkin wrote:
> On Mon, Nov 18, 2019 at 12:01:08PM +0800, Wei Wang wrote:
>> On 11/16/2019 06:55 AM, 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.
>> Not sure about the above. But the following unit mismatch is a good capture,
>> thanks!
>>
>>> There's also a unit mismatch, 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.
>>>
>>> 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)
>>>
>>> Have both return virtio pages, and divide into system pages when
>>> returning from shrinker_scan()
>> Sounds good.
>>
>>> Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier with shrinker")
>>> Cc: Wei Wang <wei.w.wang@intel.com>
>>> Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
>>> ---
>>>
>>> Tested this under memory pressure conditions and the shrinker seemed to
>>> shrink.
>>>
>>>    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);
>> We also need a fix here then:
>>
>> pages_freed = shrink_free_pages(vb, sc->nr_to_scan) *
>> VIRTIO_BALLOON_PAGES_PER_PAGE;
> No let's do accounting in pages please. virtio page is a legacy
> thing we just did not fix it in time to get rid of it by now.
>
>> Btw, there is another mistake, in virtio_balloon_shrinker_count:
>>
>> -       count += vb->num_free_page_blocks >> VIRTIO_BALLOON_FREE_PAGE_ORDER;
>> +      count += vb->num_free_page_blocks << VIRTIO_BALLOON_FREE_PAGE_ORDER;
>>
>> You may want to include it in this fix patch as well.
> OMG. should be a separate patch.
> But really this just shows why shifts are such a bad idea.
>
> Let's define
> VIRTIO_BALLOON_PAGES_PER_FREE_PAGE
>
> and use it with * and / consistently instead of shifts.
>

OK, will do (maybe call it VIRTIO_BALLOON_FREE_PAGES_PER_BLOCK).

Best,
Wei

Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation

[Michael S. Tsirkin]

On Mon, Nov 18, 2019 at 03:42:10PM +0800, Wei Wang wrote:
> On 11/18/2019 01:30 PM, Michael S. Tsirkin wrote:
> > On Mon, Nov 18, 2019 at 12:01:08PM +0800, Wei Wang wrote:
> > > On 11/16/2019 06:55 AM, 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.
> > > Not sure about the above. But the following unit mismatch is a good capture,
> > > thanks!
> > > 
> > > > There's also a unit mismatch, 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.
> > > > 
> > > > 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)
> > > > 
> > > > Have both return virtio pages, and divide into system pages when
> > > > returning from shrinker_scan()
> > > Sounds good.
> > > 
> > > > Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier with shrinker")
> > > > Cc: Wei Wang <wei.w.wang@intel.com>
> > > > Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
> > > > ---
> > > > 
> > > > Tested this under memory pressure conditions and the shrinker seemed to
> > > > shrink.
> > > > 
> > > >    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);
> > > We also need a fix here then:
> > > 
> > > pages_freed = shrink_free_pages(vb, sc->nr_to_scan) *
> > > VIRTIO_BALLOON_PAGES_PER_PAGE;
> > No let's do accounting in pages please. virtio page is a legacy
> > thing we just did not fix it in time to get rid of it by now.
> > 
> > > Btw, there is another mistake, in virtio_balloon_shrinker_count:
> > > 
> > > -       count += vb->num_free_page_blocks >> VIRTIO_BALLOON_FREE_PAGE_ORDER;
> > > +      count += vb->num_free_page_blocks << VIRTIO_BALLOON_FREE_PAGE_ORDER;
> > > 
> > > You may want to include it in this fix patch as well.
> > OMG. should be a separate patch.
> > But really this just shows why shifts are such a bad idea.
> > 
> > Let's define
> > VIRTIO_BALLOON_PAGES_PER_FREE_PAGE
> > 
> > and use it with * and / consistently instead of shifts.
> > 
> 
> OK, will do (maybe call it VIRTIO_BALLOON_FREE_PAGES_PER_BLOCK).
> 
> Best,
> Wei


The order is called VIRTIO_BALLOON_FREE_PAGE_ORDER
making it sounds like there's a free page
and that's it order.

Let's rename that hont block?
So VIRTIO_BALLOON_HINT_BLOCK_ORDER ?

VIRTIO_BALLOON_PAGES_PER_HINT_BLOCK ?

Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation

[Wei Wang]

On 11/18/2019 04:26 PM, Michael S. Tsirkin wrote:
>
> The order is called VIRTIO_BALLOON_FREE_PAGE_ORDER
> making it sounds like there's a free page
> and that's it order.
>
> Let's rename that hont block?
> So VIRTIO_BALLOON_HINT_BLOCK_ORDER ?
>
> VIRTIO_BALLOON_PAGES_PER_HINT_BLOCK ?

Sounds good.

Best,
Wei

Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation

[Khazhismel Kumykov]

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

On Sun, Nov 17, 2019 at 9:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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?
>

As far as I could tell, there wasn't any significant real impact. It
just raised an eyebrow as I was skimming over it.

SHRINK_BATCH is 128, it does look like we can override the batch size
per shrinker if we desire, but we don't so it's 128, yeah.

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

leak_balloon returns virtio pages so this would need to actually be
something like pages_freed += leak_balloon(vb, pages_to_free *
PAGES_PER_PAGE) / PAGES_PER_PAGE;, which didn't particularly sit well
with me :)

since leak_balloon is used elsewhere and seems to use "virtio pages" I
opted to have shrink_balloon accept number of "virtio pages", for
consistency.

>
> > 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@intel.com>
> > Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
> > ---
> >
> > 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?
>
nope, just a cleanup.

I'll re-send my patches split up, but it sounds like there's some more
incoming as well? I'll leave that to Wei.

khazhy

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

[PATCH 1/2] virtio_balloon: fix pages_to_free calculation

[Khazhismel Kumykov]

freed_pages was accumulating total freed pages, but was also subtracted
on each iteration from pages_to_free, which could potentially result in
attempting to free fewer pages than asked for. This change also makes
both freed_pages and pages_to_free in terms of "balloon pages", where
they were mismatched before.

Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier with shrinker")
Cc: Wei Wang <wei.w.wang@intel.com>
Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
---
 drivers/virtio/virtio_balloon.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 226fbb995fb0..7cf9540a40b8 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;
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 2/2] virtio_balloon: fix shrinker_scan return units

[Khazhismel Kumykov]

We were returning number of virtio balloon pages, which may not be the
same as number of system pages

Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
Cc: Wei Wang <wei.w.wang@intel.com>
Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
---
 drivers/virtio/virtio_balloon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 7cf9540a40b8..7951ece3fe24 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -802,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

Re: [PATCH 1/2] virtio_balloon: fix pages_to_free calculation

[Michael S. Tsirkin]

On Mon, Nov 18, 2019 at 01:38:10PM -0800, Khazhismel Kumykov wrote:
> freed_pages was accumulating total freed pages, but was also subtracted
> on each iteration from pages_to_free, which could potentially result in
> attempting to free fewer pages than asked for. This change also makes
> both freed_pages and pages_to_free in terms of "balloon pages", where
> they were mismatched before.

And then patch 2/2 changes it back to both be regular pages.
Which is good, but why do we have to go back and forth
breaking then fixing it back?


> 
> Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier with shrinker")
> Cc: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
> ---
>  drivers/virtio/virtio_balloon.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 226fbb995fb0..7cf9540a40b8 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;
> -- 
> 2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH 2/2] virtio_balloon: fix shrinker_scan return units

[Michael S. Tsirkin]

On Mon, Nov 18, 2019 at 01:38:11PM -0800, Khazhismel Kumykov wrote:
> We were returning number of virtio balloon pages, which may not be the
> same as number of system pages
> 
> Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Cc: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
> ---
>  drivers/virtio/virtio_balloon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 7cf9540a40b8..7951ece3fe24 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -802,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;
>

I'm no seeing why is this one right. shrink_free_pages gives result
in PAGE_SIZE units, right?
  
>  	pages_freed += shrink_balloon_pages(vb, pages_to_free - pages_freed);
>  
> -	return pages_freed;
> +	return pages_freed / VIRTIO_BALLOON_PAGES_PER_PAGE;


My head hurts. Isn't this what patch 1 tweaked?



>  }
>  
>  static unsigned long virtio_balloon_shrinker_count(struct shrinker *shrinker,
> -- 
> 2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH 1/2] virtio_balloon: fix pages_to_free calculation

[Michael S. Tsirkin]

So I really think we should do something like the below instead.
Limit playing with balloon pages so we can gradually limit it to legacy.
Testing, review would be appreciated.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


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

Re: [PATCH 1/2] virtio_balloon: fix pages_to_free calculation

[Khazhismel Kumykov]

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

On Mon, Nov 18, 2019 at 3:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
>
> So I really think we should do something like the below instead.
> Limit playing with balloon pages so we can gradually limit it to legacy.
> Testing, review would be appreciated.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> 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);
>

I got confused about shrink_free_pages, since, looking at the
operations the function *should* return the same units it takes in.
However, it does look like it should be operating on system pages
(VIRTIO_BALLOON_FREE_PAGE_ORDER is the order of our real page
allocations), so the multiplication on nr_to_scan is where we messed
up. OK. Sorry for the mixup :)

Your patch looks correct to me.

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

Re: [PATCH 1/2] virtio_balloon: fix pages_to_free calculation

[Wei Wang]

On 11/19/2019 07:08 AM, Michael S. Tsirkin wrote:
> So I really think we should do something like the below instead.
> Limit playing with balloon pages so we can gradually limit it to legacy.
> Testing, review would be appreciated.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> 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;
> +}
> +

Looks good to me, too. (just a reminder that the returning type of 
leak_balloon is "unsigned int",
we may want them to be consistent).

Reviewed-by: Wei Wang <wei.w.wang@intel.com>

Best,
Wei

Re: [PATCH 1/2] virtio_balloon: fix pages_to_free calculation

[Michael S. Tsirkin]

On Tue, Nov 19, 2019 at 01:38:44PM +0800, Wei Wang wrote:
> On 11/19/2019 07:08 AM, Michael S. Tsirkin wrote:
> > So I really think we should do something like the below instead.
> > Limit playing with balloon pages so we can gradually limit it to legacy.
> > Testing, review would be appreciated.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > 
> > 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;
> > +}
> > +
> 
> Looks good to me, too. (just a reminder that the returning type of
> leak_balloon is "unsigned int",

Yea that use of 32 bit integers is another problem with the existing interfaces.

> we may want them to be consistent).
> 
> Reviewed-by: Wei Wang <wei.w.wang@intel.com>
> 
> Best,
> Wei