On Thu, Oct 19, 2017 at 08:52:20PM +0900, Tetsuo Handa wrote:
> 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.
Well it's already using msleep, I'm fine with reworking it all to use
delayed_work. That's unrelated to the OOM issues though.
>
> > > 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.
Right now existing hypervisors do not send that interrupt.
If you suggest a new feature where hypervisors send an interrupt,
that might work but will need a new feature bit. Please send an email
to [email protected] (subscriber only, sorry about that)
so the bit can be reserved.
--
MST
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
