Re: [Qemu-devel] [PATCH 1/3] balloon: drop old stats code & API

[Luiz Capitulino]

On Mon, 17 Dec 2012 12:23:59 +0000
Dietmar Maurer <address@hidden> wrote:

> > > I don't really see any conflicts here?
> > 
> > query-balloon resets all stats to -1. So, if it's issued after the stats 
> > have been
> > polled all values are lost.
> 
> I can't see that in the code. I also tested with my patch, and it does not 
> reset
> any stats. So I can't see that conflict?
> 
> There is no code to reset stats inside virtio_balloon_stat()

static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
{
    VirtIOBalloon *dev = opaque;

#if 0
    /* Disable guest-provided stats for now. For more details please check:
     * https://bugzilla.redhat.com/show_bug.cgi?id=623903
     *
     * If you do enable it (which is probably not going to happen as we
     * need a new command for it), remember that you also need to fill the
     * appropriate members of the BalloonInfo structure so that the stats
     * are returned to the client.
     */
    if (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ)) {
        virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);
        virtio_notify(&dev->vdev, dev->svq);
        return;
    }
#endif

    /* Stats are not supported.  Clear out any stale values that might
     * have been set by a more featureful guest kernel.
     */
    reset_stats(dev); 
<<<<========================================================
        ^^^^^^^^^^^^^^^^
        ^^^^^^^^^^^^^^^^
        ^^^^^^^^^^^^^^^^
        ^^^^^^^^^^^^^^^^

    info->actual = ram_size - ((uint64_t) dev->actual <<
                               VIRTIO_BALLOON_PFN_SHIFT);
}

> 
> > > > It's important to note that the QMP and HMP interfaces are also
> > > > dropped by this commit. That shouldn't be a problem though, because:
> > > >
> > > >  1. All QMP fields are optional
> > > >  2. This has never been really used
> > >
> > > Why don't we simply implement the missing parts - it's just a few lines of
> > code.
> > 
> > For the reasons explained in my last email. We'd need at least two
> > commands, one for to enable polling/set time interval and also query-
> > balloon-stats, as adding this to query-balloon would mix semantics (ie. one
> > field is always there and the others have polling semantics).
> 
> Yes, we need 2 commands - and what is the problem?

The problem is adding new commands when that's not required. With QOM
everything can be done with existing commands.