Re: [PATCH v1 2/2] migration: Do not perform RAMBlock dirty sync during the first iteration

[David Hildenbrand <david@redhat.com>]

On 11.11.24 11:08, Yong Huang wrote:
> 
> 
> On Mon, Nov 11, 2024 at 5:27 PM David Hildenbrand <david@redhat.com 
> <mailto:david@redhat.com>> wrote:
> 
>     On 09.11.24 05:59, Hyman Huang wrote:
>      > The first iteration's RAMBlock dirty sync can be omitted because QEMU
>      > always initializes the RAMBlock's bmap to all 1s by default.
>      >
>      > Signed-off-by: Hyman Huang <yong.huang@smartx.com
>     <mailto:yong.huang@smartx.com>>
>      > ---
>      >   migration/cpu-throttle.c |  2 +-
>      >   migration/ram.c          | 11 ++++++++---
>      >   2 files changed, 9 insertions(+), 4 deletions(-)
>      >
>      > diff --git a/migration/cpu-throttle.c b/migration/cpu-throttle.c
>      > index 5179019e33..674dc2004e 100644
>      > --- a/migration/cpu-throttle.c
>      > +++ b/migration/cpu-throttle.c
>      > @@ -141,7 +141,7 @@ void cpu_throttle_dirty_sync_timer_tick(void
>     *opaque)
>      >        * effect on guest performance, therefore omit it to avoid
>      >        * paying extra for the sync penalty.
>      >        */
>      > -    if (sync_cnt <= 1) {
>      > +    if (!sync_cnt) {
>      >           goto end;
>      >       }
>      >
>      > diff --git a/migration/ram.c b/migration/ram.c
>      > index 05ff9eb328..571dba10b7 100644
>      > --- a/migration/ram.c
>      > +++ b/migration/ram.c
>      > @@ -2718,7 +2718,7 @@ static void ram_list_init_bitmaps(void)
>      >   {
>      >       MigrationState *ms = migrate_get_current();
>      >       RAMBlock *block;
>      > -    unsigned long pages;
>      > +    unsigned long pages, clear_bmap_pages;
>      >       uint8_t shift;
>      >
>      >       /* Skip setting bitmap if there is no RAM */
>      > @@ -2736,6 +2736,7 @@ static void ram_list_init_bitmaps(void)
>      >
>      >           RAMBLOCK_FOREACH_NOT_IGNORED(block) {
>      >               pages = block->max_length >> TARGET_PAGE_BITS;
>      > +            clear_bmap_pages = clear_bmap_size(pages, shift);
>      >               /*
>      >                * The initial dirty bitmap for migration must be
>     set with all
>      >                * ones to make sure we'll migrate every guest RAM
>     page to
>      > @@ -2751,7 +2752,12 @@ static void ram_list_init_bitmaps(void)
>      >                   block->file_bmap = bitmap_new(pages);
>      >               }
>      >               block->clear_bmap_shift = shift;
>      > -            block->clear_bmap =
>     bitmap_new(clear_bmap_size(pages, shift));
>      > +            block->clear_bmap = bitmap_new(clear_bmap_pages);
>      > +            /*
>      > +             * Set clear_bmap to 1 unconditionally, as we always
>     set bmap
>      > +             * to all 1s by default.
>      > +             */
>      > +            bitmap_set(block->clear_bmap, 0, clear_bmap_pages);
>      >           }
>      >       }
>      >   }
>      > @@ -2783,7 +2789,6 @@ static bool ram_init_bitmaps(RAMState *rs,
>     Error **errp)
>      >               if (!ret) {
>      >                   goto out_unlock;
>      >               }
>      > -            migration_bitmap_sync_precopy(false);
>      >           }
>      >       }
>      >   out_unlock:
> 
> 
>     For virtio-mem, we rely on the migration_bitmap_clear_discarded_pages()
>     call to clear all bits that correspond to unplugged memory ranges. 
> 
> 
>     If we ommit the sync, we can likely have bits of unplugged ranges still
>     set to "1", meaning we would try migrate them later, although we
>     shouldn't?
> 
> 
> 
> IIUC, migration_bitmap_clear_discarded_pagesis still called at the end of
> ram_init_bitmaps no matter if we omit the first sync.
 > > PRECOPY_NOTIFY_SETUPnotification is sent out at the end of
> ram_save_setup(ram_list_init_bitmaps),when 
> virtio_balloon_free_page_start() is
> called,migration_bitmap_clear_discarded_pages() has already completed 
> and the
> bmap has been correctly cleared.
> 
> ram_save_setup
>     -> ram_list_init_bitmaps
>         -> migration_bitmap_clear_discarded_pages
>      -> return precopy_notify(PRECOPY_NOTIFY_SETUP, errp);
> 
> You can double check it.

That's not my concern, let me clarify :)


Assume in KVM the bitmap is all 1s ("everything dirty").

In current code, we will sync the bitmap once (IIRC, clearing any dirty 
bits from KVM).

Then we call migration_bitmap_clear_discarded_pages() to clear all 
"discarded" pages that we shouldn't touch.

When we do the next bitmap sync, we will not get a "1" for discarded 
ranges, and we will never try migrating discarded ranges.


With your patch, we're omitting the first sync. Could we possibly get 
discarded ranges reported from KVM as dirty during the "now first" sync 
*after* the migration_bitmap_clear_discarded_pages() call, and try 
migrating discarded ranges?

I did not dive deep into the code, maybe 
migration_bitmap_clear_discarded_pages() ends up clearing the bits in 
KVM, but I recall that there was something special about the first 
bitmap sync.

-- 
Cheers,

David / dhildenb