On Thu, May 30, 2019 at 07:58:55PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@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 <pet...@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
