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

[Michael S. Tsirkin]

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.

> 
> > 
> > > 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.

OK I guess "dirty logging must be enabled" would be better.
And with above we can say hinting must be disabled
before logging bitmap is synchronized.

> 
> > 
> > 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