Re: [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload

[Michael S. Tsirkin]

On Tue, Sep 06, 2016 at 09:50:00AM +0300, Roman Kagan wrote:
> On Tue, Sep 06, 2016 at 04:45:13AM +0300, Michael S. Tsirkin wrote:
> > On Mon, Sep 05, 2016 at 11:02:36AM +0300, Roman Kagan wrote:
> > > On Sat, Sep 03, 2016 at 01:53:53AM +0300, Michael S. Tsirkin wrote:
> > > > On Fri, Sep 02, 2016 at 10:21:58AM +0300, Roman Kagan wrote:
> > > > > On Thu, Sep 01, 2016 at 10:26:54PM +0300, Michael S. Tsirkin wrote:
> > > > > > I'm sorry - I don't like this patch. This means that
> > > > > > virtio_balloon_receive_stats will be called and will poke
> > > > > > at the ring even if the ring was never kicked.
> > > > > 
> > > > > I'm not sure I understand what the problem is with that:
> > > > > virtio_balloon_receive_stats just returns early if 
> > > > > virtio_queue_empty(),
> > > > > which is no more poking at the ring than is already done in 
> > > > > virtio_load.
> > > > 
> > > > Generally we should not look at ring until there was a kick.
> > > 
> > > How else would you recover ->inuse?
> > > 
> > > Roman.
> > 
> > We seem to do this:
> > +            vdev->vq[i].inuse = vdev->vq[i].last_avail_idx -
> > +                                vdev->vq[i].used_idx;
> 
> Right, but .used_idx is fetched from vring two lines above:
> 
>       vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
> 
> > Is this wrong?
> 
> And no, I don't think it is wrong :)  I guess there's just no other way
> to obtain it.
> 
> Roman.

Hmm right. It actually has a comment:
/* XXX virtio-1 devices */

So we do need to fix it.
Let's not add more stuff like this though.

-- 
MST