On Tuesday, February 4, 2020 10:30 PM, David Hildenbrand wrote: > 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. Yes. Either host sends to the guest a STOP cmd or when the guest fails to allocate a page (meaning that all the possible free pages are taken already), the reporting ends. > 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); Right, thanks! > > > 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 don't think it is an issue here. MUST_TELL_HOST is for the ballooning pages, where pages are offered to host to _USE_. For free page hint, as the name already suggests, it's just a _HINT_ , so in whatever use case, the host should not take the page to use. So the guest doesn't need to tell host and wait. Back to the implementation of virtio_balloon_shrinker_scan, which I don't see an issue so far: shrink_free_pages just return pages to mm without waiting for the ack from host shrink_balloon_pages goes through leak_balloon which tell_host before release the balloon pages. Best, Wei
