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
