Re: [PATCH v3 2/3] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

[Wei Wang <wei.w.wang@intel.com>]

On 03/06/2018 10:38 AM, Michael S. Tsirkin wrote:
> On Tue, Mar 06, 2018 at 09:54:49AM +0800, Wei Wang wrote:
> > On 03/05/2018 10:09 PM, Michael S. Tsirkin wrote:
> > > On Mon, Mar 05, 2018 at 11:36:15AM +0800, Wei Wang wrote:
> > > > On 03/03/2018 02:37 AM, Michael S. Tsirkin wrote:
> > > > > On Fri, Mar 02, 2018 at 04:47:29PM +0800, Wei Wang wrote:
> > > > > > diff --git a/include/sysemu/balloon.h b/include/sysemu/balloon.h
> > > > > > index af49e19..16a2aae 100644
> > > > > > --- a/include/sysemu/balloon.h
> > > > > > +++ b/include/sysemu/balloon.h
> > > > > ...
> > > > >
> > > > > > +typedef void (QEMUBalloonFreePageStart)(void *opaque);
> > > > > > +typedef void (QEMUBalloonFreePageStop)(void *opaque);
> > > > > So I think the rule is that no bitmap sync must happen
> > > > > between these two, otherwise a hint might arrive and
> > > > > override the sync output.
> > > > >
> > > > > Should be documented I think.
> > > > Yes, agree.
> > > Ideally we'd also detect violations and trigger an assert.
> > How about just invoking
> >
> > if (rs->free_page_support)
> >      balloon_free_page_stop();
> >
> > at the beginning of migration_bitmap_sync()? (balloon_free_page_stop will
> > just return if the optimization has stopped.)
> >
> > In this way, we can always have the guarantee that "no bitmap sync must
> > happen between these two"
> Why not. And in fact you can do balloon_free_page_start at the
> end of sync.

Sounds good. I implemented it this way in v4. In this case, we actually 
extend the usage of this optimization beyond the bulk stage. Though it 
shows similar test results as v3 which optimizes bulk stage only, but 
still good to leave the optimization there for the 2nd stage onward as 
well. It will speed up 2nd stage onward, for example, in this scenario: 
the guest writes page A, and then free(A) soon. After QEMU syncs bitmap, 
it sees bit of A is set, but now A is free page, the optimization can 
help to clear A from the bitmap.

Best,
Wei