On Mon, Nov 11, 2024 at 6:42 PM David Hildenbrand <david@redhat.com
<mailto:david@redhat.com>> wrote:
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>
> <mailto: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>
> <mailto: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).
For the old logic, write-protect and clear dirty bits are all done in
the KVM_GET_DIRTY_LOG API, while with
KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 feature enabled, clearing
dirty bits are postponed in the KVM_CLEAR_DIRTY_LOG API, which
is called right before page sending in the migration thread in QEMU.
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
Yes, the migration_bitmap_clear_discarded_pages clear the bits in
KVM in:
ramblock_dirty_bitmap_clear_discarded_pages
-> dirty_bitmap_clear_section
-> migration_clear_memory_region_dirty_bitmap_range
-> migration_clear_memory_region_dirty_bitmap
-> memory_region_clear_dirty_bitmap
-> KVM_CLEAR_DIRTY_LOG ioctl