Re: [Qemu-devel] [PATCH v3 12/12] migration: Split log_clear() into smaller chunks

[Peter Xu <peterx@redhat.com>]

On Thu, May 30, 2019 at 07:58:55PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > Currently we are doing log_clear() right after log_sync() which mostly
> > keeps the old behavior when log_clear() was still part of log_sync().
> > 
> > This patch tries to further optimize the migration log_clear() code
> > path to split huge log_clear()s into smaller chunks.
> > 
> > We do this by spliting the whole guest memory region into memory
> > chunks, whose size is decided by MigrationState.clear_bitmap_shift (an
> > example will be given below).  With that, we don't do the dirty bitmap
> > clear operation on the remote node (e.g., KVM) when we fetch the dirty
> > bitmap, instead we explicitly clear the dirty bitmap for the memory
> > chunk for each of the first time we send a page in that chunk.
> > 
> > Here comes an example.
> > 
> > Assuming the guest has 64G memory, then before this patch the KVM
> > ioctl KVM_CLEAR_DIRTY_LOG will be a single one covering 64G memory.
> > If after the patch, let's assume when the clear bitmap shift is 18,
> > then the memory chunk size on x86_64 will be 1UL<<18 * 4K = 1GB.  Then
> > instead of sending a big 64G ioctl, we'll send 64 small ioctls, each
> > of the ioctl will cover 1G of the guest memory.  For each of the 64
> > small ioctls, we'll only send if any of the page in that small chunk
> > was going to be sent right away.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> > ---
> >  include/exec/ram_addr.h | 75 +++++++++++++++++++++++++++++++++++++++--
> >  migration/migration.c   |  4 +++
> >  migration/migration.h   | 27 +++++++++++++++
> >  migration/ram.c         | 44 ++++++++++++++++++++++++
> >  migration/trace-events  |  1 +
> >  5 files changed, 149 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> > index f8930914cd..c85896f4d5 100644
> > --- a/include/exec/ram_addr.h
> > +++ b/include/exec/ram_addr.h
> > @@ -50,8 +50,69 @@ struct RAMBlock {
> >      unsigned long *unsentmap;
> >      /* bitmap of already received pages in postcopy */
> >      unsigned long *receivedmap;
> > +
> > +    /*
> > +     * bitmap of already cleared dirty bitmap.  Set this up to
> 
> Is that description the right way around?  Isn't it set when
> we receive dirty info from the kernel and hence it needs clearing
> in the future?

Right.  I should say "bitmap to track ...".  I'll reword.

> 
> > +     * non-NULL to enable the capability to postpone and split
> > +     * clearing of dirty bitmap on the remote node (e.g., KVM).  The
> > +     * bitmap will be set only when doing global sync.
> > +     *
> > +     * NOTE: this bitmap is different comparing to the other bitmaps
> > +     * in that one bit can represent multiple guest pages (which is
> > +     * decided by the `clear_bmap_shift' variable below).  On
> > +     * destination side, this should always be NULL, and the variable
> > +     * `clear_bmap_shift' is meaningless.
> > +     */
> > +    unsigned long *clear_bmap;
> > +    uint8_t clear_bmap_shift;
> 
> Is this just here as a convenience rather than using the copy in
> the property?

Right.  It seems a bit awkward to use that directly.  Also note that
the other value can be different with this one when it's illegal
(e.g., bigger than CLEAR_BITMAP_SHIFT_MAX or smaller than
CLEAR_BITMAP_SHIFT_MIN).  This value will always be legal and we don't
need to check/assert it every time.

(Hmm, could bmap_shift be different for different ramblocks?  I never
 thought about that but actually, it can, of course)

Regards,

-- 
Peter Xu