Re: [Qemu-devel] [PATCH v7 4/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, May 31, 2018 at 10:27:00AM +0800, Wei Wang wrote:
> On 05/30/2018 08:47 PM, Michael S. Tsirkin wrote:
> > On Wed, May 30, 2018 at 05:12:09PM +0800, Wei Wang wrote:
> > > On 05/29/2018 11:24 PM, Michael S. Tsirkin wrote:
> > > > On Tue, Apr 24, 2018 at 02:13:47PM +0800, Wei Wang wrote:
> > > > > +/*
> > > > > + * Balloon will report pages which were free at the time of this call. As the
> > > > > + * reporting happens asynchronously, dirty bit logging must be enabled before
> > > > > + * this call is made.
> > > > > + */
> > > > > +void balloon_free_page_start(void)
> > > > > +{
> > > > > +    balloon_free_page_start_fn(balloon_opaque);
> > > > > +}
> > > > Please create notifier support, not a single global.
> > > OK. The start is called at the end of bitmap_sync, and the stop is called at
> > > the beginning of bitmap_sync. In this case, we will need to add two
> > > migration states, MIGRATION_STATUS_BEFORE_BITMAP_SYNC and
> > > MIGRATION_STATUS_AFTER_BITMAP_SYNC, right?
> > If that's the way you do it, you need to ask migration guys, not me.
> 
> Yeah, I know.. thanks for the virtio part.
> 
> > > > +
> > > > +static bool virtio_balloon_free_page_support(void *opaque)
> > > > +{
> > > > +    VirtIOBalloon *s = opaque;
> > > > +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > > > +
> > > > +    return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> > > > or if poison is negotiated.
> > > Will make it
> > > return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT) &&
> > > !virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)
> > 
> > I mean the reverse:
> > 	virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT) ||
> > 	virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)
> > 
> > 
> > If poison has been negotiated you must migrate the
> > guest supplied value even if you don't use it for hints.
> 
> 
> Just a little confused with the logic. Writing it that way means that we are
> taking this possibility "virtio_vdev_has_feature(vdev,
> VIRTIO_BALLOON_F_FREE_PAGE_HINT)=fasle, virtio_vdev_has_feature(vdev,
> VIRTIO_BALLOON_F_PAGE_POISON)=true" into account, and let the support
> function return true when F_FREE_PAGE_HINT isn't supported.

All I am saying is that in this configuration, you must migrate
the poison value programmed by guest even if you do not
yet use it without VIRTIO_BALLOON_F_FREE_PAGE_HINT.

Right now you have
a section:
+    .needed = virtio_balloon_free_page_support,

which includes the poison value.

So if guest migrates after writing the poison value,
it's lost. Not nice.

> If guest doesn't support F_FREE_PAGE_HINT, it doesn't support the free page
> reporting (even the free page vq). I'm not sure why we tell the migration
> thread that the free page reporting feature is supported via this support
> function. If the support function simply returns false when F_FREE_PAGE_HINT
> isn't negotiated, the legacy migration already migrates the poisoned pages
> (not skipped, but may be compressed).
> 
> I think it would be better to simply use the original "return
> virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)" here.


So maybe you should put the poison value in a separate section then.


> 
> Best,
> Wei