Michael S. Tsirkin wrote:
> On Wed, Oct 18, 2017 at 07:59:23PM +0900, Tetsuo Handa wrote:
> > Do you see anything wrong with the patch I used for emulating
> > VIRTIO_BALLOON_F_DEFLATE_ON_OOM path (shown below) ?
> >
> > ----------------------------------------
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index f0b3a0b..a679ac2 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -164,7 +164,7 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> > }
> > set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> > vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > - if (!virtio_has_feature(vb->vdev,
> > + if (virtio_has_feature(vb->vdev,
> > VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > adjust_managed_page_count(page, -1);
> > }
> > @@ -184,7 +184,7 @@ static void release_pages_balloon(struct virtio_balloon *vb,
> > struct page *page, *next;
> >
> > list_for_each_entry_safe(page, next, pages, lru) {
> > - if (!virtio_has_feature(vb->vdev,
> > + if (virtio_has_feature(vb->vdev,
> > VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > adjust_managed_page_count(page, 1);
> > list_del(&page->lru);
> > @@ -363,7 +363,7 @@ static int virtballoon_oom_notify(struct notifier_block *self,
> > unsigned num_freed_pages;
> >
> > vb = container_of(self, struct virtio_balloon, nb);
> > - if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > return NOTIFY_OK;
> >
> > freed = parm;
> > ----------------------------------------
>
> Looks right but it's probably easier to configure qemu to set that
> feature bit. Basically you just add deflate-on-oom=on to the
> balloon device.
I'm using CentOS 7 where qemu does not recognize deflate-on-oom option. ;-)
> OK. Or if you use my patch, you can just set a flag and go
> if (vb->oom)
> msleep(1000);
> at beginning of fill_balloon.
I don't think it is a good manner to sleep for long from the point of view of
system_freezable_wq, for system_freezable_wq is expected to flush shortly
according to include/linux/workqueue.h . I think that using delayed_work is better.
> > While response was better than now, inflating again spoiled the effort.
> > Retrying to inflate until allocation fails is already too painful.
> >
> > Michael S. Tsirkin wrote:
> > > I think that's the case. Question is, when can we inflate again?
> >
> > I think that it is when the host explicitly asked again, for
> > VIRTIO_BALLOON_F_DEFLATE_ON_OOM path does not schedule for later inflation.
>
> Problem is host has no idea when it's safe.
> If we expect host to ask again after X seconds we
> might just as well do it in the guest.
To me, fill_balloon() with VIRTIO_BALLOON_F_DEFLATE_ON_OOM sounds like
doing
echo 3 > /proc/sys/vm/drop_caches
where nobody knows whether it won't impact the system.
Thus, I don't think it is a problem. It will be up to administrator
who enters that command.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
