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

[Peter Xu <peterx@redhat.com>]

On Mon, Jun 04, 2018 at 04:04:51PM +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?
> 
> Peter, do you have any thought about this?
> 
> Currently, the usage of free page optimization isn't limited to the first
> stage. It is used in each stage. A global call to start the free page
> optimization is made after bitmap sync, and another global call to stop the
> optimization is made before bitmap sync. It is simple to just use global
> calls.
> 
> If we change the implementation to use notifiers, I think we will need to
> add two new MigrationStatus as above. Would you think that is worthwhile for
> some reason?

I'm a bit confused.  Could you elaborate why we need those extra
states?

Or, to ask a more general question - could you elaborate a bit on how
you order these operations?  I would be really glad if you can point
me to some documents for the feature.  Is there any latest virtio
document that I can refer to (or old cover letter links)?  It'll be
good if the document could mention about things like:

- why we need this feature? Is that purely for migration purpose?  Or
  it can be used somewhere else too?
- high level stuff about how this is implemented, e.g.:
  - the protocol of the new virtio queues
  - how we should get the free page hints (please see below)

For now, what I see is that we do:

(1) stop hinting
(2) sync bitmap
(3) start hinting

Why this order?  My understanding is that obviously there is a race
between the page hinting thread and the dirty bitmap tracking part
(which is done in KVM).  How do we make sure there is no race?

An direct question is that, do we need to make sure step (1) must be
before step (2)?  Asked since I see that currently step (1) is an
async operation (taking a lock, set status, then return).  Then would
such an async operation satisfy any ordering requirement after all?

Btw, I would appreciate if you can push your new trees (both QEMU and
kernel) to the links you mentioned in the cover letter - I noticed
that they are not the same as what you have posted on the list.

Thanks,

-- 
Peter Xu