* [PATCH] virtio_balloon: fix shrinker pages_to_free calculation @ 2019-11-15 22:55 Khazhismel Kumykov 2019-11-18 4:01 ` Wei Wang 2019-11-18 5:26 ` Michael S. Tsirkin 0 siblings, 2 replies; 16+ messages in thread From: Khazhismel Kumykov @ 2019-11-15 22:55 UTC (permalink / raw) To: mst, jasowang, wei.w.wang Cc: virtualization, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation 2019-11-15 22:55 [PATCH] virtio_balloon: fix shrinker pages_to_free calculation Khazhismel Kumykov @ 2019-11-18 4:01 ` Wei Wang 2019-11-18 5:30 ` Michael S. Tsirkin 2019-11-18 5:26 ` Michael S. Tsirkin 1 sibling, 1 reply; 16+ messages in thread From: Wei Wang @ 2019-11-18 4:01 UTC (permalink / raw) To: Khazhismel Kumykov, mst, jasowang; +Cc: virtualization, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation 2019-11-18 4:01 ` Wei Wang @ 2019-11-18 5:30 ` Michael S. Tsirkin 2019-11-18 7:42 ` Wei Wang 0 siblings, 1 reply; 16+ messages in thread From: Michael S. Tsirkin @ 2019-11-18 5:30 UTC (permalink / raw) To: Wei Wang; +Cc: Khazhismel Kumykov, jasowang, virtualization, linux-kernel 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation 2019-11-18 5:30 ` Michael S. Tsirkin @ 2019-11-18 7:42 ` Wei Wang 2019-11-18 8:26 ` Michael S. Tsirkin 0 siblings, 1 reply; 16+ messages in thread From: Wei Wang @ 2019-11-18 7:42 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Khazhismel Kumykov, jasowang, virtualization, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation 2019-11-18 7:42 ` Wei Wang @ 2019-11-18 8:26 ` Michael S. Tsirkin 2019-11-18 10:31 ` Wei Wang 0 siblings, 1 reply; 16+ messages in thread From: Michael S. Tsirkin @ 2019-11-18 8:26 UTC (permalink / raw) To: Wei Wang; +Cc: Khazhismel Kumykov, jasowang, virtualization, linux-kernel 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 ? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation 2019-11-18 8:26 ` Michael S. Tsirkin @ 2019-11-18 10:31 ` Wei Wang 0 siblings, 0 replies; 16+ messages in thread From: Wei Wang @ 2019-11-18 10:31 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Khazhismel Kumykov, jasowang, virtualization, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation 2019-11-15 22:55 [PATCH] virtio_balloon: fix shrinker pages_to_free calculation Khazhismel Kumykov 2019-11-18 4:01 ` Wei Wang @ 2019-11-18 5:26 ` Michael S. Tsirkin 2019-11-18 21:30 ` Khazhismel Kumykov 1 sibling, 1 reply; 16+ messages in thread From: Michael S. Tsirkin @ 2019-11-18 5:26 UTC (permalink / raw) To: Khazhismel Kumykov; +Cc: jasowang, wei.w.wang, virtualization, linux-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation 2019-11-18 5:26 ` Michael S. Tsirkin @ 2019-11-18 21:30 ` Khazhismel Kumykov 2019-11-18 21:38 ` [PATCH 1/2] virtio_balloon: fix " Khazhismel Kumykov 0 siblings, 1 reply; 16+ messages in thread From: Khazhismel Kumykov @ 2019-11-18 21:30 UTC (permalink / raw) To: Michael S. Tsirkin Cc: jasowang, wei.w.wang, virtualization, Linux Kernel Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] virtio_balloon: fix pages_to_free calculation 2019-11-18 21:30 ` Khazhismel Kumykov @ 2019-11-18 21:38 ` Khazhismel Kumykov 2019-11-18 21:38 ` [PATCH 2/2] virtio_balloon: fix shrinker_scan return units Khazhismel Kumykov ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Khazhismel Kumykov @ 2019-11-18 21:38 UTC (permalink / raw) To: mst, jasowang, wei.w.wang Cc: linux-kernel, virtualization, 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] virtio_balloon: fix shrinker_scan return units 2019-11-18 21:38 ` [PATCH 1/2] virtio_balloon: fix " Khazhismel Kumykov @ 2019-11-18 21:38 ` Khazhismel Kumykov 2019-11-18 22:56 ` Michael S. Tsirkin 2019-11-18 22:52 ` [PATCH 1/2] virtio_balloon: fix pages_to_free calculation Michael S. Tsirkin 2019-11-18 23:08 ` Michael S. Tsirkin 2 siblings, 1 reply; 16+ messages in thread From: Khazhismel Kumykov @ 2019-11-18 21:38 UTC (permalink / raw) To: mst, jasowang, wei.w.wang Cc: linux-kernel, virtualization, 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] virtio_balloon: fix shrinker_scan return units 2019-11-18 21:38 ` [PATCH 2/2] virtio_balloon: fix shrinker_scan return units Khazhismel Kumykov @ 2019-11-18 22:56 ` Michael S. Tsirkin 0 siblings, 0 replies; 16+ messages in thread From: Michael S. Tsirkin @ 2019-11-18 22:56 UTC (permalink / raw) To: Khazhismel Kumykov; +Cc: jasowang, wei.w.wang, linux-kernel, virtualization 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] virtio_balloon: fix pages_to_free calculation 2019-11-18 21:38 ` [PATCH 1/2] virtio_balloon: fix " Khazhismel Kumykov 2019-11-18 21:38 ` [PATCH 2/2] virtio_balloon: fix shrinker_scan return units Khazhismel Kumykov @ 2019-11-18 22:52 ` Michael S. Tsirkin 2019-11-18 23:08 ` Michael S. Tsirkin 2 siblings, 0 replies; 16+ messages in thread From: Michael S. Tsirkin @ 2019-11-18 22:52 UTC (permalink / raw) To: Khazhismel Kumykov; +Cc: jasowang, wei.w.wang, linux-kernel, virtualization 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] virtio_balloon: fix pages_to_free calculation 2019-11-18 21:38 ` [PATCH 1/2] virtio_balloon: fix " Khazhismel Kumykov 2019-11-18 21:38 ` [PATCH 2/2] virtio_balloon: fix shrinker_scan return units Khazhismel Kumykov 2019-11-18 22:52 ` [PATCH 1/2] virtio_balloon: fix pages_to_free calculation Michael S. Tsirkin @ 2019-11-18 23:08 ` Michael S. Tsirkin 2019-11-18 23:20 ` Khazhismel Kumykov 2019-11-19 5:38 ` Wei Wang 2 siblings, 2 replies; 16+ messages in thread From: Michael S. Tsirkin @ 2019-11-18 23:08 UTC (permalink / raw) To: Khazhismel Kumykov; +Cc: jasowang, wei.w.wang, linux-kernel, virtualization 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); ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] virtio_balloon: fix pages_to_free calculation 2019-11-18 23:08 ` Michael S. Tsirkin @ 2019-11-18 23:20 ` Khazhismel Kumykov 2019-11-19 5:38 ` Wei Wang 1 sibling, 0 replies; 16+ messages in thread From: Khazhismel Kumykov @ 2019-11-18 23:20 UTC (permalink / raw) To: Michael S. Tsirkin Cc: jasowang, wei.w.wang, Linux Kernel Mailing List, virtualization [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] virtio_balloon: fix pages_to_free calculation 2019-11-18 23:08 ` Michael S. Tsirkin 2019-11-18 23:20 ` Khazhismel Kumykov @ 2019-11-19 5:38 ` Wei Wang 2019-11-19 9:37 ` Michael S. Tsirkin 1 sibling, 1 reply; 16+ messages in thread From: Wei Wang @ 2019-11-19 5:38 UTC (permalink / raw) To: Michael S. Tsirkin, Khazhismel Kumykov Cc: jasowang, linux-kernel, virtualization 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] virtio_balloon: fix pages_to_free calculation 2019-11-19 5:38 ` Wei Wang @ 2019-11-19 9:37 ` Michael S. Tsirkin 0 siblings, 0 replies; 16+ messages in thread From: Michael S. Tsirkin @ 2019-11-19 9:37 UTC (permalink / raw) To: Wei Wang; +Cc: Khazhismel Kumykov, jasowang, linux-kernel, virtualization 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-11-19 9:37 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-15 22:55 [PATCH] virtio_balloon: fix shrinker pages_to_free calculation Khazhismel Kumykov 2019-11-18 4:01 ` Wei Wang 2019-11-18 5:30 ` Michael S. Tsirkin 2019-11-18 7:42 ` Wei Wang 2019-11-18 8:26 ` Michael S. Tsirkin 2019-11-18 10:31 ` Wei Wang 2019-11-18 5:26 ` Michael S. Tsirkin 2019-11-18 21:30 ` Khazhismel Kumykov 2019-11-18 21:38 ` [PATCH 1/2] virtio_balloon: fix " Khazhismel Kumykov 2019-11-18 21:38 ` [PATCH 2/2] virtio_balloon: fix shrinker_scan return units Khazhismel Kumykov 2019-11-18 22:56 ` Michael S. Tsirkin 2019-11-18 22:52 ` [PATCH 1/2] virtio_balloon: fix pages_to_free calculation Michael S. Tsirkin 2019-11-18 23:08 ` Michael S. Tsirkin 2019-11-18 23:20 ` Khazhismel Kumykov 2019-11-19 5:38 ` Wei Wang 2019-11-19 9:37 ` Michael S. Tsirkin
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).