Date:   Mon, 18 Nov 2019 13:30:45 -0800
From:   Khazhismel Kumykov <khazhy@...gle.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     jasowang@...hat.com, wei.w.wang@...el.com,
        virtualization@...ts.linux-foundation.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] virtio_balloon: fix shrinker pages_to_free calculation

On Sun, Nov 17, 2019 at 9:26 PM Michael S. Tsirkin <mst@...hat.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@...el.com>
> > Signed-off-by: Khazhismel Kumykov <khazhy@...gle.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

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4843 bytes)

Powered by blists - more mailing lists