From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36792) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUjQq-0003wu-JF for qemu-devel@nongnu.org; Wed, 16 May 2012 14:58:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SUjQo-00013T-E2 for qemu-devel@nongnu.org; Wed, 16 May 2012 14:58:44 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:55573) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUjQo-000136-4O for qemu-devel@nongnu.org; Wed, 16 May 2012 14:58:42 -0400 Received: by pbbro12 with SMTP id ro12so1904818pbb.4 for ; Wed, 16 May 2012 11:58:40 -0700 (PDT) Message-ID: <4FB3F8DA.4030809@codemonkey.ws> Date: Wed, 16 May 2012 13:58:34 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1337163047-6159-1-git-send-email-berrange@redhat.com> <20120516154245.0d9b0cec@doriath.home> In-Reply-To: <20120516154245.0d9b0cec@doriath.home> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] Add event notification for guest balloon changes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Amit Shah , qemu-devel@nongnu.org, Markus Armbruster On 05/16/2012 01:42 PM, Luiz Capitulino wrote: > On Wed, 16 May 2012 11:10:47 +0100 > "Daniel P. Berrange" wrote: > >> From: "Daniel P. Berrange" >> >> After setting a balloon target value, applications have to >> continually poll 'query-balloon' to determine whether the >> guest has reacted to this request. The virtio-balloon backend >> knows exactly when the guest has reacted though, and thus it >> is possible to emit a JSON event to tell the mgmt application >> whenever the guest balloon changes. >> >> This introduces a new 'qemu_balloon_change()' API which is >> to be called by balloon driver backends, whenever they have >> a change in balloon value. This takes the 'actual' balloon >> value, as would be found in the BalloonInfo struct. >> >> The qemu_balloon_change API emits a JSON monitor event which >> looks like: >> >> {"timestamp": {"seconds": 1337162462, "microseconds": 814521}, >> "event": "BALLOON_CHANGE", "data": {"actual": 944766976}} > > It's missing an entry in QMP/qmp-events.txt and I have a comment below, > but in general looks good. > > Amit, would be good to get your ack. I think it would be safer to limit this event to (1) only firing once target has been reached (2) firing if target is deviated from without a corresponding change in target. Otherwise, a guest could just flood libvirt with events. This would queue memory in QEMU indefinitely as the events got queued up to potentially serving as a DoS against other guests. Regards, Anthony LIguori > >> >> * balloon.c, balloon.h: Introduce qemu_balloon_change() for >> emitting balloon change events on the monitor >> * hw/virtio-balloon.c: Invoke qemu_balloon_change() whenever >> the guest changes the balloon actual value >> * monitor.c, monitor.h: Define QEVENT_BALLOON_CHANGE >> >> Signed-off-by: Daniel P. Berrange >> --- >> balloon.c | 14 ++++++++++++++ >> balloon.h | 2 ++ >> hw/virtio-balloon.c | 5 +++++ >> monitor.c | 3 +++ >> monitor.h | 1 + >> 5 files changed, 25 insertions(+), 0 deletions(-) >> >> diff --git a/balloon.c b/balloon.c >> index aa354f7..913862b 100644 >> --- a/balloon.c >> +++ b/balloon.c >> @@ -30,6 +30,7 @@ >> #include "balloon.h" >> #include "trace.h" >> #include "qmp-commands.h" >> +#include "qjson.h" >> >> static QEMUBalloonEvent *balloon_event_fn; >> static QEMUBalloonStatus *balloon_stat_fn; >> @@ -80,6 +81,19 @@ static int qemu_balloon_status(BalloonInfo *info) >> return 1; >> } >> >> +void qemu_balloon_change(int64_t actual) >> +{ >> + QObject *data; >> + >> + data = qobject_from_jsonf("{ 'actual': %" PRId64 " }", >> + actual); >> + >> + monitor_protocol_event(QEVENT_BALLOON_CHANGE, data); >> + >> + qobject_decref(data); >> +} >> + >> + >> BalloonInfo *qmp_query_balloon(Error **errp) >> { >> BalloonInfo *info; >> diff --git a/balloon.h b/balloon.h >> index b60fd5d..2ebac0d 100644 >> --- a/balloon.h >> +++ b/balloon.h >> @@ -24,4 +24,6 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func, >> QEMUBalloonStatus *stat_func, void *opaque); >> void qemu_remove_balloon_handler(void *opaque); >> >> +void qemu_balloon_change(int64_t actual); >> + >> #endif >> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c >> index ce9d2c9..9137573 100644 >> --- a/hw/virtio-balloon.c >> +++ b/hw/virtio-balloon.c >> @@ -146,8 +146,13 @@ static void virtio_balloon_set_config(VirtIODevice *vdev, >> { >> VirtIOBalloon *dev = to_virtio_balloon(vdev); >> struct virtio_balloon_config config; >> + uint32_t oldactual = dev->actual; >> memcpy(&config, config_data, 8); >> dev->actual = le32_to_cpu(config.actual); >> + if (dev->actual != oldactual) { >> + qemu_balloon_change(ram_size - >> + (dev->actual<< VIRTIO_BALLOON_PFN_SHIFT)); >> + } > > This can cause several events to be emitted until the memory is adjusted > to the value asked by the user. I'm undecided if this is a feature, but > if I were a client issuing the balloon command I'd expect to get the event > only when the memory is fully adjusted to the value I asked. > > Not sure if this possible to implement though, or if we really want it. > > >> } >> >> static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f) >> diff --git a/monitor.c b/monitor.c >> index 12a6fe2..ef59cd9 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -493,6 +493,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data) >> case QEVENT_WAKEUP: >> event_name = "WAKEUP"; >> break; >> + case QEVENT_BALLOON_CHANGE: >> + event_name = "BALLOON_CHANGE"; >> + break; >> default: >> abort(); >> break; >> diff --git a/monitor.h b/monitor.h >> index 0d49800..8de0160 100644 >> --- a/monitor.h >> +++ b/monitor.h >> @@ -41,6 +41,7 @@ typedef enum MonitorEvent { >> QEVENT_DEVICE_TRAY_MOVED, >> QEVENT_SUSPEND, >> QEVENT_WAKEUP, >> + QEVENT_BALLOON_CHANGE, >> QEVENT_MAX, >> } MonitorEvent; >> >