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

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

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"


> > How about adding the following new balloon API explanation to
> > this patch's commit:
> >
> >      - balloon_free_page_start: Callers call this API to obtain guest free
> >        page hints, and clear the related bits from the migration dirty
> > bitmap.
> >        The whole process is implemented in a new thread independent of the
> >        migration thread. Free page hints imply the part of guest memory is
> >        likely to be free without a guarantee. That is, the reported free
> > pages
> >        may not be free any more when QEMU receives them, so callers are
> >        responsible for detecting those pages that are not free pages after
> > the
> >        bits are cleared from the dirty bitmap. To ensure the above, this API
> >        should be used when the migration dirty logging mechanism (e.g.
> >        guest memory write-protection) has started.
> >
> >      - balloon_free_page_stop: Callers call this API to stop the guest from
> >        reporting free page hints. Bits from the dirty bitmap are safe to
> >        be cleared on condition that the dirty logging mechanism is recording
> >        pages that the guest has written to. To avoid the case that clearing
> >        bits of free page hints overrides the dirty bits offered by the dirty
> >        logging mechanism, this API is suggested to be called before QEMU
> >        synchronizes the dirty logging bitmap.
> >
> >      - balloon_free_page_support: This API is called to check whether the
> >        balloon device supports the guest free page reporting feature. The
> >        balloon_free_page_start and balloon_free_page_stop APIs should be used
> >        only when this API returns true.
> >
> >
> > Best,
> > Wei
> I find this more confusing than explaining.
>
> Let me try
>
> balloon_free_page_start - start guest free page hint reporting.
> Note: balloon will report pages which were free at the time
> of this call. As the reporting happens asynchronously,
> we rely on dirty logging to be started before this call is made.
>
> The dirty logging bitmap must be synchronized before this call
> and then after balloon_free_page_stop.

I think it would be better to remove the above one sentence.
I agree "No dirty bitmap synchronizations are allowed between 
balloon_free_page_start and balloon_free_page_stop", but "The dirty 
logging bitmap MUST be synchronized before balloon_free_page_start" 
seems confusing, for example the bulk stage doesn't have to start with a 
bitmap sync.


> balloon_free_page_stop: stop the guest reporting
> of free pages. dirty logging bitmap can be synchronized
> after this point.
>
> No bitmap synchronizations are allowed between these two points.

Best,
Wei