Re: Balloon pressuring page cache

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Feb 04, 2020 at 03:30:19PM +0100, David Hildenbrand wrote:
> On 04.02.20 09:40, Michael S. Tsirkin wrote:
> > On Tue, Feb 04, 2020 at 09:35:21AM +0100, David Hildenbrand wrote:
> >>>>> I would say reverting probably makes sense. I'm not sure there is much
> >>>>> value to having a shrinker running deflation when you are actively trying
> >>>>> to increase the balloon. It would make more sense to wait until you are
> >>>>> actually about to start hitting oom.
> >>>>
> >>>> I think the shrinker makes sense for free page hinting feature
> >>>> (everything on free_page_list).
> >>>>
> >>>> So instead of only reverting, I think we should split it up and always
> >>>> register the shrinker for VIRTIO_BALLOON_F_FREE_PAGE_HINT and the OOM
> >>>> notifier (as before) for VIRTIO_BALLOON_F_MUST_TELL_HOST.
> >>
> >> s/VIRTIO_BALLOON_F_MUST_TELL_HOST/VIRTIO_BALLOON_F_DEFLATE_ON_OOM/
> >>
> >> :)
> > 
> > Well VIRTIO_BALLOON_F_MUST_TELL_HOST is also broken by shrinker
> > with VIRTIO_BALLOON_F_FREE_PAGE_HINT as that code adds buffers
> > but does not wait for them to be used even with VIRTIO_BALLOON_F_MUST_TELL_HOST.
> > We never noticed because QEMU does not advertize
> > VIRTIO_BALLOON_F_MUST_TELL_HOST.
> 
> So, I am trying to understand how the code is intended to work, but I
> am afraid I am missing something (or to rephrase: I think I found a BUG :) and
> there is lack of proper documentation about this feature).
> 
> a) We allocate pages and add them to the list as long as we are told to do so.
>    We send these pages to the host one by one.
> b) We free all pages once we get a STOP signal. Until then, we keep pages allocated.
> c) When called via the shrinker, we want to free pages from the list, even
> though the hypervisor did not notify us to do so.
> 
> 
> Issue 1: When we unload the balloon driver in the guest in an unlucky event,
> we won't free the pages. We are missing something like (if I am not wrong):
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index b1d2068fa2bd..e2b0925e1e83 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -929,6 +929,10 @@ static void remove_common(struct virtio_balloon *vb)
>                 leak_balloon(vb, vb->num_pages);
>         update_balloon_size(vb);
>  
> +       /* There might be free pages that are being reported: release them. */
> +       if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> +               return_free_pages_to_mm(vb, ULONG_MAX);
> +
>         /* Now we reset the device so we can clean up the queues. */
>         vb->vdev->config->reset(vb->vdev);


Indeed.

> 
> Issue 2: When called via the shrinker, (but also to fix Issue 1), it could be
> that we do have VIRTIO_BALLOON_F_MUST_TELL_HOST. I assume this means
> (-ENOCLUE) that we have to wait until the hypervisor notifies us via the STOP? Or
> for which event do we have to wait? Because there is no way to *tell host* here
> that we want to reuse a page. The hypervisor will *tell us* when we can reuse pages.
> For the shrinker it is simple: Don't use the shrinker with
> VIRTIO_BALLOON_F_MUST_TELL_HOST :) . But to fix Issue 1, we *would* have to wait
> until we get a STOP signal. That is not really possible because it might
> take an infinite amount of time.
> 
> Michael, any clue on which event we have to wait with
> VIRTIO_BALLOON_F_MUST_TELL_HOST? IMHO, I don't think
> VIRTIO_BALLOON_F_MUST_TELL_HOST applies to VIRTIO_BALLOON_F_FREE_PAGE_HINT and
> we'd better document that. It introduces complexity with no clear benefit.

I meant that we must wait for host to see the hint. Signalled via using
the buffer.  But maybe that's too far in the meaning from
VIRTIO_BALLOON_F_MUST_TELL_HOST and we need a separate new flag for
that. Then current code won't be broken (yay!) but we need to
document another flag that's pretty similar.

> 
> -- 
> Thanks,
> 
> David / dhildenb