From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53330) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xtx0o-00083C-Tn for qemu-devel@nongnu.org; Thu, 27 Nov 2014 06:13:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xtx0k-0002Jf-E7 for qemu-devel@nongnu.org; Thu, 27 Nov 2014 06:13:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33661) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xtx0k-0002J6-6I for qemu-devel@nongnu.org; Thu, 27 Nov 2014 06:13:22 -0500 Date: Thu, 27 Nov 2014 13:08:18 +0200 From: "Michael S. Tsirkin" Message-ID: <20141127110818.GC8961@redhat.com> References: <1416996685-15115-1-git-send-email-den@openvz.org> <1416996685-15115-3-git-send-email-den@openvz.org> <20141126111603.GB4222@redhat.com> <54770557.6010303@openvz.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54770557.6010303@openvz.org> Subject: Re: [Qemu-devel] [PATCH 2/2] balloon: add a feature bit to let Guest OS deflate balloon on oom List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: qemu-devel@nongnu.org, Raushaniya Maksudova , Anthony Liguori On Thu, Nov 27, 2014 at 02:04:55PM +0300, Denis V. Lunev wrote: > On 26/11/14 14:16, Michael S. Tsirkin wrote: > >On Wed, Nov 26, 2014 at 01:11:25PM +0300, Denis V. Lunev wrote: > >>From: Raushaniya Maksudova > >> > >>Excessive virtio_balloon inflation can cause invocation of OOM-killer, > >>when Linux is under severe memory pressure. Various mechanisms are > >>responsible for correct virtio_balloon memory management. Nevertheless it > >>is often the case that these control tools does not have enough time to > >>react on fast changing memory load. As a result OS runs out of memory and > >>invokes OOM-killer. The balancing of memory by use of the virtio balloon > >>should not cause the termination of processes while there are pages in the > >>balloon. Now there is no way for virtio balloon driver to free memory at > >>the last moment before some process get killed by OOM-killer. > >> > >>This does not provide a security breach as balloon itself is running > >>inside Guest OS and is working in the cooperation with the host. Thus > >>some improvements from Guest side should be considered as normal. > >> > >>To solve the problem, introduce a virtio_balloon callback which is > >>expected to be called from the oom notifier call chain in out_of_memory() > >>function. If virtio balloon could release some memory, it will make the > >>system to return and retry the allocation that forced the out of memory > >>killer to run. > >> > >>This behavior should be enabled if and only if appropriate feature bit > >>is set on the device. It is off by default. > >> > >>This functionality was recently merged into vanilla Linux (actually in > >>linux-next at the moment) > >> > >> commit 5a10b7dbf904bfe01bb9fcc6298f7df09eed77d5 > >> Author: Raushaniya Maksudova > >> Date: Mon Nov 10 09:36:29 2014 +1030 > >> > >>This patch adds respective control bits into QEMU. It introduces > >>deflate-on-oom option for baloon device which do the trick. > >> > >>Signed-off-by: Raushaniya Maksudova > >>Signed-off-by: Denis V. Lunev > >>CC: Anthony Liguori > >>CC: Michael S. Tsirkin > >>--- > >> hw/virtio/virtio-balloon.c | 7 +++++++ > >> include/hw/virtio/virtio-balloon.h | 2 ++ > >> qemu-options.hx | 6 +++++- > >> 3 files changed, 14 insertions(+), 1 deletion(-) > >> > >>diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > >>index 7bfbb75..9d145fa 100644 > >>--- a/hw/virtio/virtio-balloon.c > >>+++ b/hw/virtio/virtio-balloon.c > >>@@ -305,7 +305,12 @@ static void virtio_balloon_set_config(VirtIODevice *vdev, > >> static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f) > >> { > >>+ VirtIOBalloon *dev = VIRTIO_BALLOON(vdev); > >> f |= (1 << VIRTIO_BALLOON_F_STATS_VQ); > >>+ if (dev->deflate_on_oom) { > >>+ f |= (1 << VIRTIO_BALLOON_F_DEFLATE_ON_OOM); > >>+ } > >>+ > >> return f; > >> } > >>@@ -409,6 +414,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp) > >> } > >> static Property virtio_balloon_properties[] = { > >>+ DEFINE_PROP_BOOL("deflate-on-oom", VirtIOBalloon, deflate_on_oom, false), > >> DEFINE_PROP_END_OF_LIST(), > >> }; > >>diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h > >>index f863bfe..45cc55a 100644 > >>--- a/include/hw/virtio/virtio-balloon.h > >>+++ b/include/hw/virtio/virtio-balloon.h > >>@@ -30,6 +30,7 @@ > >> /* The feature bitmap for virtio balloon */ > >> #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */ > >> #define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory stats virtqueue */ > >>+#define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */ > >> /* Size of a PFN in the balloon interface. */ > >> #define VIRTIO_BALLOON_PFN_SHIFT 12 > >>@@ -67,6 +68,7 @@ typedef struct VirtIOBalloon { > >> QEMUTimer *stats_timer; > >> int64_t stats_last_update; > >> int64_t stats_poll_interval; > >>+ bool deflate_on_oom; > >> } VirtIOBalloon; > >> #endif > >You don't need an extra bool, and open-coding. > >Do it same as we do for other features please, > >set bit in feature mask directly. > there is no host_features placeholder at this level. > > We could add it and propagate proper bit in get_features > callback like > return f | dev->host_features > or we have to move this stuff a little bit up into > CCW & PCI code like done in virtscsi with > #define DEFINE_VIRTIO_SCSI_FEATURES(_state, _feature_field) > > The first approach keeps bits in the same place, > the second follows current approach. If you prefer unification way, > I think that other devices could be re-written this way. > > Any opinion? Balloon is the weird one out here. Look at how feature bits are defined for e.g. DEFINE_PROP_BIT("any_layout", _state, _field, VIRTIO_F_ANY_LAYOUT, true), you should do something similar. > >>diff --git a/qemu-options.hx b/qemu-options.hx > >>index da9851d..14ede0b 100644 > >>--- a/qemu-options.hx > >>+++ b/qemu-options.hx > >>@@ -324,7 +324,8 @@ ETEXI > >> DEF("balloon", HAS_ARG, QEMU_OPTION_balloon, > >> "-balloon none disable balloon device\n" > >> "-balloon virtio[,addr=str]\n" > >>- " enable virtio balloon device (default)\n", QEMU_ARCH_ALL) > >>+ " enable virtio balloon device (default)\n" > >>+ " [,deflate-on-oom=on|off]\n", QEMU_ARCH_ALL) > >> STEXI > >> @item -balloon none > >> @findex -balloon > >>@@ -332,6 +333,9 @@ Disable balloon device. > >> @item -balloon virtio[,addr=@var{addr}] > >> Enable virtio balloon device (default), optionally with PCI address > >> @var{addr}. > >>+@item -balloon virtio[,deflate-on-oom=@var{deflate-on-oom}] > >>+@var{deflate-on-oom} is "on" or "off" and enables whether to let Guest OS > >>+to deflate virtio balloon on OOM. Default is off. > >> ETEXI > >> DEF("device", HAS_ARG, QEMU_OPTION_device, > >Please don't add stuff to legacy -balloon. > >New -device is enough, you don't need to touch qemu-options.hx > >for it. > > > sure