[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V6)

[Adam Litke]

On Wed, 2010-01-13 at 16:04 -0200, Luiz Capitulino wrote:
>  I've tried to apply this patch to play with it, but turns out it conflicts
> with recent changes in hw/virtio-balloon.

Ahh, I will continue my never-ending quest to stay current :)

>  Some comments on the QMP side of the patch follows.

Thanks for your review!

<snip>

> > +/*
> > + * complete_stats_request - Clean up and report statistics.
> > + */
> > +static void complete_stats_request(VirtIOBalloon *vb)
> > +{
> > +    QObject *stats = get_stats_qobject(vb);
> > +
> > +    if (vb->stats_request_mode == QEMU_BALLOON_MODE_SYNC) {
> > +        qemu_del_timer(vb->stats_timer);
> > +        monitor_print_balloon(cur_mon, stats);
> > +        monitor_resume(cur_mon);
> > +    } else if (vb->stats_request_mode == QEMU_BALLOON_MODE_ASYNC) {
> > +        monitor_protocol_event(QEVENT_BALLOON, stats);
> > +    }
> > +
> > +    vb->stats_request_mode = QEMU_BALLOON_MODE_NONE;
> > +}
> 
>  In the previous thread Anthony raised some issues about the 'cur_mon'
> usage that made me concerned, because some important code rely on
> it (read async events).
> 
>  As far as I could check, 'cur_mon' is guaranteed to be the default
> Monitor which is fine if you're aware of it when putting QEMU to run,
> but I'm afraid that testing your patch with two Monitors (user and qmp)
> is not going to work.
> 
>  Maybe not a big deal, but would be good to be aware of potential
> issues.

I talked to Anthony and will try to fix this up.  I just need to start
passing the monitor pointer around as well.

<snip>
      * 
> > +    if (monitor_ctrl_mode(mon))
> > +        mode = QEMU_BALLOON_MODE_ASYNC;
> > +    else
> > +        
> > +    mode = monitor_ctrl_mode(mon) ?
> > +                            QEMU_BALLOON_MODE_ASYNC : 
> > QEMU_BALLOON_MODE_SYNC;
> 
>  I think what you want is:
> 
> } else {
>  mode = QEMU_BALLOON_MODE_SYNC;
> }

Bah... Left over gunk. 

<snip>

> > -void qemu_balloon(ram_addr_t target)
> > +int qemu_balloon(ram_addr_t target)
> >  {
> > -    if (qemu_balloon_event)
> > -        qemu_balloon_event(qemu_balloon_event_opaque, target);
> > +    if (qemu_balloon_event) {
> > +        qemu_balloon_event(qemu_balloon_event_opaque, target,
> > +                           QEMU_BALLOON_MODE_SYNC);
> > +        return 1;
> > +    } else {
> > +        return 0;
> > +    }
> >  }
> 
>  This is used by do_balloon() right? Which is also used by QMP,
> shouldn't it also handle async vs. sync?

qemu_balloon always acts synchronously.  It does not wait on the guest
to do anything and it does not return data.

-- 
Thanks,
Adam