* [PATCH RFC 0/2] migration: Skip sync in ram_init_bitmaps() @ 2024-10-23 2:09 yong.huang 2024-10-23 2:09 ` [PATCH RFC 1/2] accel/kvm: Introduce kvm_dirty_log_manual_enabled yong.huang 2024-10-23 2:09 ` [PATCH RFC 2/2] migration: Avoid doing RAMBlock dirty sync in the initial iteration yong.huang 0 siblings, 2 replies; 7+ messages in thread From: yong.huang @ 2024-10-23 2:09 UTC (permalink / raw) To: qemu-devel; +Cc: Peter Xu, Fabiano Rosas, Paolo Bonzini, yong.huang From: Hyman Huang <yong.huang@smartx.com> As discussed in: https://lore.kernel.org/qemu-devel/ZvbQ0RQx-zxOeo4Y@x1n/ RAMBlock dirty sync in ram_init_bitmaps() appears to be unnecessary; this patchset attempts to eliminate it and asks for comments on how to do so. Please review, thanks Yong Hyman Huang (2): accel/kvm: Introduce kvm_dirty_log_manual_enabled migration: Avoid doing RAMBlock dirty sync in the initial iteration accel/kvm/kvm-all.c | 2 ++ include/sysemu/kvm.h | 8 ++++++++ migration/cpu-throttle.c | 3 ++- migration/ram.c | 30 +++++++++++++++++++++++++++--- 4 files changed, 39 insertions(+), 4 deletions(-) -- 2.27.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH RFC 1/2] accel/kvm: Introduce kvm_dirty_log_manual_enabled 2024-10-23 2:09 [PATCH RFC 0/2] migration: Skip sync in ram_init_bitmaps() yong.huang @ 2024-10-23 2:09 ` yong.huang 2024-10-23 2:09 ` [PATCH RFC 2/2] migration: Avoid doing RAMBlock dirty sync in the initial iteration yong.huang 1 sibling, 0 replies; 7+ messages in thread From: yong.huang @ 2024-10-23 2:09 UTC (permalink / raw) To: qemu-devel; +Cc: Peter Xu, Fabiano Rosas, Paolo Bonzini, yong.huang From: Hyman Huang <yong.huang@smartx.com> Introudce kvm_dirty_log_manual_enabled to indicate if dirty logging manually was enabled in KVM. The kvm_dirty_log_manual_enabled will be used in the next commit. Signed-off-by: Hyman Huang <yong.huang@smartx.com> --- accel/kvm/kvm-all.c | 2 ++ include/sysemu/kvm.h | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 801cff16a5..a0f3e6e493 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -93,6 +93,7 @@ bool kvm_allowed; bool kvm_readonly_mem_allowed; bool kvm_vm_attributes_allowed; bool kvm_msi_use_devid; +bool kvm_dirty_log_manual; static bool kvm_has_guest_debug; static int kvm_sstep_flags; static bool kvm_immediate_exit; @@ -2537,6 +2538,7 @@ static int kvm_setup_dirty_ring(KVMState *s) dirty_log_manual_caps); s->manual_dirty_log_protect = 0; } + kvm_dirty_log_manual = true; } } diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index c3a60b2890..f38f2818e1 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -42,6 +42,7 @@ extern bool kvm_gsi_routing_allowed; extern bool kvm_gsi_direct_mapping; extern bool kvm_readonly_mem_allowed; extern bool kvm_msi_use_devid; +extern bool kvm_dirty_log_manual; #define kvm_enabled() (kvm_allowed) /** @@ -143,6 +144,12 @@ extern bool kvm_msi_use_devid; */ #define kvm_msi_devid_required() (kvm_msi_use_devid) +/** + * kvm_dirty_log_manual_enabled: + * Returns: true if dirty logging manually was enabled. + */ +#define kvm_dirty_log_manual_enabled() (kvm_dirty_log_manual) + #else #define kvm_enabled() (0) @@ -157,6 +164,7 @@ extern bool kvm_msi_use_devid; #define kvm_gsi_direct_mapping() (false) #define kvm_readonly_mem_enabled() (false) #define kvm_msi_devid_required() (false) +#define kvm_dirty_log_manual_enabled() (false) #endif /* CONFIG_KVM_IS_POSSIBLE */ -- 2.27.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH RFC 2/2] migration: Avoid doing RAMBlock dirty sync in the initial iteration 2024-10-23 2:09 [PATCH RFC 0/2] migration: Skip sync in ram_init_bitmaps() yong.huang 2024-10-23 2:09 ` [PATCH RFC 1/2] accel/kvm: Introduce kvm_dirty_log_manual_enabled yong.huang @ 2024-10-23 2:09 ` yong.huang 2024-10-29 16:21 ` Peter Xu 1 sibling, 1 reply; 7+ messages in thread From: yong.huang @ 2024-10-23 2:09 UTC (permalink / raw) To: qemu-devel; +Cc: Peter Xu, Fabiano Rosas, Paolo Bonzini, yong.huang From: Hyman Huang <yong.huang@smartx.com> KVM always returns 1 when userspace retrieves a dirty bitmap for the first time when KVM_DIRTY_LOG_INITIALLY_SET is enabled; in such scenario, the RAMBlock dirty sync of the initial iteration can be skipped. Signed-off-by: Hyman Huang <yong.huang@smartx.com> --- migration/cpu-throttle.c | 3 ++- migration/ram.c | 30 +++++++++++++++++++++++++++--- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/migration/cpu-throttle.c b/migration/cpu-throttle.c index 342681cdd4..06e3b1be78 100644 --- a/migration/cpu-throttle.c +++ b/migration/cpu-throttle.c @@ -27,6 +27,7 @@ #include "hw/core/cpu.h" #include "qemu/main-loop.h" #include "sysemu/cpus.h" +#include "sysemu/kvm.h" #include "cpu-throttle.h" #include "migration.h" #include "migration-stats.h" @@ -141,7 +142,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 <= (kvm_dirty_log_manual_enabled() ? 0 : 1)) { goto end; } diff --git a/migration/ram.c b/migration/ram.c index d284f63854..b312ebd69d 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,17 @@ 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 the clear bitmap by default to enable dirty logging. + * + * Note that with KVM_DIRTY_LOG_INITIALLY_SET, dirty logging + * will be enabled gradually in small chunks using + * KVM_CLEAR_DIRTY_LOG + */ + if (kvm_dirty_log_manual_enabled()) { + bitmap_set(block->clear_bmap, 0, clear_bmap_pages); + } } } } @@ -2771,6 +2782,7 @@ static void migration_bitmap_clear_discarded_pages(RAMState *rs) static bool ram_init_bitmaps(RAMState *rs, Error **errp) { + Error *local_err = NULL; bool ret = true; qemu_mutex_lock_ramlist(); @@ -2783,7 +2795,19 @@ static bool ram_init_bitmaps(RAMState *rs, Error **errp) if (!ret) { goto out_unlock; } - migration_bitmap_sync_precopy(false); + + if (kvm_dirty_log_manual_enabled()) { + /* + * Bypass the RAMBlock dirty sync and still publish a + * notification. + */ + if (precopy_notify(PRECOPY_NOTIFY_AFTER_BITMAP_SYNC, + &local_err)) { + error_report_err(local_err); + } + } else { + migration_bitmap_sync_precopy(false); + } } } out_unlock: -- 2.27.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH RFC 2/2] migration: Avoid doing RAMBlock dirty sync in the initial iteration 2024-10-23 2:09 ` [PATCH RFC 2/2] migration: Avoid doing RAMBlock dirty sync in the initial iteration yong.huang @ 2024-10-29 16:21 ` Peter Xu 2024-10-30 2:09 ` Yong Huang 0 siblings, 1 reply; 7+ messages in thread From: Peter Xu @ 2024-10-29 16:21 UTC (permalink / raw) To: yong.huang; +Cc: qemu-devel, Fabiano Rosas, Paolo Bonzini On Wed, Oct 23, 2024 at 10:09:51AM +0800, yong.huang@smartx.com wrote: > From: Hyman Huang <yong.huang@smartx.com> > > KVM always returns 1 when userspace retrieves a dirty bitmap for > the first time when KVM_DIRTY_LOG_INITIALLY_SET is enabled; in such > scenario, the RAMBlock dirty sync of the initial iteration can be > skipped. > > Signed-off-by: Hyman Huang <yong.huang@smartx.com> > --- > migration/cpu-throttle.c | 3 ++- > migration/ram.c | 30 +++++++++++++++++++++++++++--- > 2 files changed, 29 insertions(+), 4 deletions(-) > > diff --git a/migration/cpu-throttle.c b/migration/cpu-throttle.c > index 342681cdd4..06e3b1be78 100644 > --- a/migration/cpu-throttle.c > +++ b/migration/cpu-throttle.c > @@ -27,6 +27,7 @@ > #include "hw/core/cpu.h" > #include "qemu/main-loop.h" > #include "sysemu/cpus.h" > +#include "sysemu/kvm.h" > #include "cpu-throttle.h" > #include "migration.h" > #include "migration-stats.h" > @@ -141,7 +142,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 <= (kvm_dirty_log_manual_enabled() ? 0 : 1)) { > goto end; > } > > diff --git a/migration/ram.c b/migration/ram.c > index d284f63854..b312ebd69d 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,17 @@ 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 the clear bitmap by default to enable dirty logging. > + * > + * Note that with KVM_DIRTY_LOG_INITIALLY_SET, dirty logging > + * will be enabled gradually in small chunks using > + * KVM_CLEAR_DIRTY_LOG > + */ > + if (kvm_dirty_log_manual_enabled()) { > + bitmap_set(block->clear_bmap, 0, clear_bmap_pages); > + } Why it needs to be relevant to whether DIRTY_LOG is enabled? I wonder if we should always set clear_bmap to 1 unconditionally, as we always set bmap to all 1s by default. Then we skip sync always during setup, dropping patch 1. > } > } > } > @@ -2771,6 +2782,7 @@ static void migration_bitmap_clear_discarded_pages(RAMState *rs) > > static bool ram_init_bitmaps(RAMState *rs, Error **errp) > { > + Error *local_err = NULL; > bool ret = true; > > qemu_mutex_lock_ramlist(); > @@ -2783,7 +2795,19 @@ static bool ram_init_bitmaps(RAMState *rs, Error **errp) > if (!ret) { > goto out_unlock; > } > - migration_bitmap_sync_precopy(false); > + > + if (kvm_dirty_log_manual_enabled()) { > + /* > + * Bypass the RAMBlock dirty sync and still publish a > + * notification. > + */ > + if (precopy_notify(PRECOPY_NOTIFY_AFTER_BITMAP_SYNC, > + &local_err)) { > + error_report_err(local_err); > + } > + } else { > + migration_bitmap_sync_precopy(false); > + } > } > } > out_unlock: > -- > 2.27.0 > -- Peter Xu ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RFC 2/2] migration: Avoid doing RAMBlock dirty sync in the initial iteration 2024-10-29 16:21 ` Peter Xu @ 2024-10-30 2:09 ` Yong Huang 2024-10-30 19:43 ` Peter Xu 0 siblings, 1 reply; 7+ messages in thread From: Yong Huang @ 2024-10-30 2:09 UTC (permalink / raw) To: Peter Xu; +Cc: qemu-devel, Fabiano Rosas, Paolo Bonzini [-- Attachment #1: Type: text/plain, Size: 5040 bytes --] On Wed, Oct 30, 2024 at 12:21 AM Peter Xu <peterx@redhat.com> wrote: > On Wed, Oct 23, 2024 at 10:09:51AM +0800, yong.huang@smartx.com wrote: > > From: Hyman Huang <yong.huang@smartx.com> > > > > KVM always returns 1 when userspace retrieves a dirty bitmap for > > the first time when KVM_DIRTY_LOG_INITIALLY_SET is enabled; in such > > scenario, the RAMBlock dirty sync of the initial iteration can be > > skipped. > > > > Signed-off-by: Hyman Huang <yong.huang@smartx.com> > > --- > > migration/cpu-throttle.c | 3 ++- > > migration/ram.c | 30 +++++++++++++++++++++++++++--- > > 2 files changed, 29 insertions(+), 4 deletions(-) > > > > diff --git a/migration/cpu-throttle.c b/migration/cpu-throttle.c > > index 342681cdd4..06e3b1be78 100644 > > --- a/migration/cpu-throttle.c > > +++ b/migration/cpu-throttle.c > > @@ -27,6 +27,7 @@ > > #include "hw/core/cpu.h" > > #include "qemu/main-loop.h" > > #include "sysemu/cpus.h" > > +#include "sysemu/kvm.h" > > #include "cpu-throttle.h" > > #include "migration.h" > > #include "migration-stats.h" > > @@ -141,7 +142,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 <= (kvm_dirty_log_manual_enabled() ? 0 : 1)) { > > goto end; > > } > > > > diff --git a/migration/ram.c b/migration/ram.c > > index d284f63854..b312ebd69d 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,17 @@ 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 the clear bitmap by default to enable dirty logging. > > + * > > + * Note that with KVM_DIRTY_LOG_INITIALLY_SET, dirty logging > > + * will be enabled gradually in small chunks using > > + * KVM_CLEAR_DIRTY_LOG > > + */ > > + if (kvm_dirty_log_manual_enabled()) { > > + bitmap_set(block->clear_bmap, 0, clear_bmap_pages); > > + } > > Why it needs to be relevant to whether DIRTY_LOG is enabled? > > I wonder if we should always set clear_bmap to 1 unconditionally, as we > always set bmap to all 1s by default. > OK, this works. We can drop it. > > Then we skip sync always during setup, dropping patch 1. > IIUC, KVM initializes the slot->dirty_bitmap with 1 only when KVM_DIRTY_LOG_INITIALLY_SET is enabled, 0 otherwize. This means that if KVM does not support the KVM_DIRTY_LOG_INITIALLY_SET feature, userspace should do the first sync so that KVM could set the WP bit and clear the D-bit of the PTE. Skipping first sync could handle this scenario? > > } > > } > > } > > @@ -2771,6 +2782,7 @@ static void > migration_bitmap_clear_discarded_pages(RAMState *rs) > > > > static bool ram_init_bitmaps(RAMState *rs, Error **errp) > > { > > + Error *local_err = NULL; > > bool ret = true; > > > > qemu_mutex_lock_ramlist(); > > @@ -2783,7 +2795,19 @@ static bool ram_init_bitmaps(RAMState *rs, Error > **errp) > > if (!ret) { > > goto out_unlock; > > } > > - migration_bitmap_sync_precopy(false); > > + > > + if (kvm_dirty_log_manual_enabled()) { > > + /* > > + * Bypass the RAMBlock dirty sync and still publish a > > + * notification. > > + */ > > + if (precopy_notify(PRECOPY_NOTIFY_AFTER_BITMAP_SYNC, > > + &local_err)) { > > + error_report_err(local_err); > > + } > > + } else { > > + migration_bitmap_sync_precopy(false); > > + } > > } > > } > > out_unlock: > > -- > > 2.27.0 > > > > -- > Peter Xu > > -- Best regards [-- Attachment #2: Type: text/html, Size: 8188 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RFC 2/2] migration: Avoid doing RAMBlock dirty sync in the initial iteration 2024-10-30 2:09 ` Yong Huang @ 2024-10-30 19:43 ` Peter Xu 2024-10-31 2:08 ` Yong Huang 0 siblings, 1 reply; 7+ messages in thread From: Peter Xu @ 2024-10-30 19:43 UTC (permalink / raw) To: Yong Huang; +Cc: qemu-devel, Fabiano Rosas, Paolo Bonzini On Wed, Oct 30, 2024 at 10:09:38AM +0800, Yong Huang wrote: > On Wed, Oct 30, 2024 at 12:21 AM Peter Xu <peterx@redhat.com> wrote: > > > On Wed, Oct 23, 2024 at 10:09:51AM +0800, yong.huang@smartx.com wrote: > > > From: Hyman Huang <yong.huang@smartx.com> > > > > > > KVM always returns 1 when userspace retrieves a dirty bitmap for > > > the first time when KVM_DIRTY_LOG_INITIALLY_SET is enabled; in such > > > scenario, the RAMBlock dirty sync of the initial iteration can be > > > skipped. > > > > > > Signed-off-by: Hyman Huang <yong.huang@smartx.com> > > > --- > > > migration/cpu-throttle.c | 3 ++- > > > migration/ram.c | 30 +++++++++++++++++++++++++++--- > > > 2 files changed, 29 insertions(+), 4 deletions(-) > > > > > > diff --git a/migration/cpu-throttle.c b/migration/cpu-throttle.c > > > index 342681cdd4..06e3b1be78 100644 > > > --- a/migration/cpu-throttle.c > > > +++ b/migration/cpu-throttle.c > > > @@ -27,6 +27,7 @@ > > > #include "hw/core/cpu.h" > > > #include "qemu/main-loop.h" > > > #include "sysemu/cpus.h" > > > +#include "sysemu/kvm.h" > > > #include "cpu-throttle.h" > > > #include "migration.h" > > > #include "migration-stats.h" > > > @@ -141,7 +142,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 <= (kvm_dirty_log_manual_enabled() ? 0 : 1)) { > > > goto end; > > > } > > > > > > diff --git a/migration/ram.c b/migration/ram.c > > > index d284f63854..b312ebd69d 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,17 @@ 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 the clear bitmap by default to enable dirty logging. > > > + * > > > + * Note that with KVM_DIRTY_LOG_INITIALLY_SET, dirty logging > > > + * will be enabled gradually in small chunks using > > > + * KVM_CLEAR_DIRTY_LOG > > > + */ > > > + if (kvm_dirty_log_manual_enabled()) { > > > + bitmap_set(block->clear_bmap, 0, clear_bmap_pages); > > > + } > > > > Why it needs to be relevant to whether DIRTY_LOG is enabled? > > > > I wonder if we should always set clear_bmap to 1 unconditionally, as we > > always set bmap to all 1s by default. > > > > OK, this works. We can drop it. > > > > > > Then we skip sync always during setup, dropping patch 1. > > > > IIUC, KVM initializes the slot->dirty_bitmap with 1 only when > KVM_DIRTY_LOG_INITIALLY_SET is enabled, 0 otherwize. > This means that if KVM does not support the > KVM_DIRTY_LOG_INITIALLY_SET feature, userspace should > do the first sync so that KVM could set the WP bit and clear > the D-bit of the PTE. > > Skipping first sync could handle this scenario? Yes, the old kernels could be tricky (!CLEAR_LOG support), but I hope it's also working. The thing is log_start() should also protect all pages if that's the case. For x86, that should corresponds to: kvm_mmu_slot_apply_flags(): /* * Initially-all-set does not require write protecting any page, * because they're all assumed to be dirty. */ if (kvm_dirty_log_manual_protect_and_init_set(kvm)) return; if (READ_ONCE(eager_page_split)) kvm_mmu_slot_try_split_huge_pages(kvm, new, PG_LEVEL_4K); if (kvm_x86_ops.cpu_dirty_log_size) { kvm_mmu_slot_leaf_clear_dirty(kvm, new); kvm_mmu_slot_remove_write_access(kvm, new, PG_LEVEL_2M); } else { kvm_mmu_slot_remove_write_access(kvm, new, PG_LEVEL_4K); } In general, I think even if GET_DIRTY_LOG in setup() is omitted, all pages should still be wr-protected already right after log_start(). Then follow up log_clear()s will be noop, until the next sync which will reprotect every page again. Please double check. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RFC 2/2] migration: Avoid doing RAMBlock dirty sync in the initial iteration 2024-10-30 19:43 ` Peter Xu @ 2024-10-31 2:08 ` Yong Huang 0 siblings, 0 replies; 7+ messages in thread From: Yong Huang @ 2024-10-31 2:08 UTC (permalink / raw) To: Peter Xu; +Cc: qemu-devel, Fabiano Rosas, Paolo Bonzini [-- Attachment #1: Type: text/plain, Size: 5968 bytes --] On Thu, Oct 31, 2024 at 3:43 AM Peter Xu <peterx@redhat.com> wrote: > On Wed, Oct 30, 2024 at 10:09:38AM +0800, Yong Huang wrote: > > On Wed, Oct 30, 2024 at 12:21 AM Peter Xu <peterx@redhat.com> wrote: > > > > > On Wed, Oct 23, 2024 at 10:09:51AM +0800, yong.huang@smartx.com wrote: > > > > From: Hyman Huang <yong.huang@smartx.com> > > > > > > > > KVM always returns 1 when userspace retrieves a dirty bitmap for > > > > the first time when KVM_DIRTY_LOG_INITIALLY_SET is enabled; in such > > > > scenario, the RAMBlock dirty sync of the initial iteration can be > > > > skipped. > > > > > > > > Signed-off-by: Hyman Huang <yong.huang@smartx.com> > > > > --- > > > > migration/cpu-throttle.c | 3 ++- > > > > migration/ram.c | 30 +++++++++++++++++++++++++++--- > > > > 2 files changed, 29 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/migration/cpu-throttle.c b/migration/cpu-throttle.c > > > > index 342681cdd4..06e3b1be78 100644 > > > > --- a/migration/cpu-throttle.c > > > > +++ b/migration/cpu-throttle.c > > > > @@ -27,6 +27,7 @@ > > > > #include "hw/core/cpu.h" > > > > #include "qemu/main-loop.h" > > > > #include "sysemu/cpus.h" > > > > +#include "sysemu/kvm.h" > > > > #include "cpu-throttle.h" > > > > #include "migration.h" > > > > #include "migration-stats.h" > > > > @@ -141,7 +142,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 <= (kvm_dirty_log_manual_enabled() ? 0 : 1)) { > > > > goto end; > > > > } > > > > > > > > diff --git a/migration/ram.c b/migration/ram.c > > > > index d284f63854..b312ebd69d 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,17 @@ 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 the clear bitmap by default to enable dirty > logging. > > > > + * > > > > + * Note that with KVM_DIRTY_LOG_INITIALLY_SET, dirty > logging > > > > + * will be enabled gradually in small chunks using > > > > + * KVM_CLEAR_DIRTY_LOG > > > > + */ > > > > + if (kvm_dirty_log_manual_enabled()) { > > > > + bitmap_set(block->clear_bmap, 0, clear_bmap_pages); > > > > + } > > > > > > Why it needs to be relevant to whether DIRTY_LOG is enabled? > > > > > > I wonder if we should always set clear_bmap to 1 unconditionally, as we > > > always set bmap to all 1s by default. > > > > > > > OK, this works. We can drop it. > > > > > > > > > > Then we skip sync always during setup, dropping patch 1. > > > > > > > IIUC, KVM initializes the slot->dirty_bitmap with 1 only when > > KVM_DIRTY_LOG_INITIALLY_SET is enabled, 0 otherwize. > > This means that if KVM does not support the > > KVM_DIRTY_LOG_INITIALLY_SET feature, userspace should > > do the first sync so that KVM could set the WP bit and clear > > the D-bit of the PTE. > > > > Skipping first sync could handle this scenario? > > Yes, the old kernels could be tricky (!CLEAR_LOG support), but I hope it's > also working. > > The thing is log_start() should also protect all pages if that's the case. > For x86, that should corresponds to: > > kvm_mmu_slot_apply_flags(): > > /* > * Initially-all-set does not require write protecting any > page, > * because they're all assumed to be dirty. > */ > if (kvm_dirty_log_manual_protect_and_init_set(kvm)) > return; > > if (READ_ONCE(eager_page_split)) > kvm_mmu_slot_try_split_huge_pages(kvm, new, > PG_LEVEL_4K); > > if (kvm_x86_ops.cpu_dirty_log_size) { > kvm_mmu_slot_leaf_clear_dirty(kvm, new); > kvm_mmu_slot_remove_write_access(kvm, new, > PG_LEVEL_2M); > } else { > kvm_mmu_slot_remove_write_access(kvm, new, > PG_LEVEL_4K); > } > This path does the write protecting indeed. Thanks for pointing this out. > > In general, I think even if GET_DIRTY_LOG in setup() is omitted, all pages > should still be wr-protected already right after log_start(). Then follow > up log_clear()s will be noop, until the next sync which will reprotect > every page again. > Agree. I'll drop the patch 1 and simplify the patch 2 in the next version. Thanks for the comments. > > Please double check. > > Thanks, > > -- > Peter Xu > > Yong -- Best regards [-- Attachment #2: Type: text/html, Size: 9177 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-10-31 2:10 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-23 2:09 [PATCH RFC 0/2] migration: Skip sync in ram_init_bitmaps() yong.huang 2024-10-23 2:09 ` [PATCH RFC 1/2] accel/kvm: Introduce kvm_dirty_log_manual_enabled yong.huang 2024-10-23 2:09 ` [PATCH RFC 2/2] migration: Avoid doing RAMBlock dirty sync in the initial iteration yong.huang 2024-10-29 16:21 ` Peter Xu 2024-10-30 2:09 ` Yong Huang 2024-10-30 19:43 ` Peter Xu 2024-10-31 2:08 ` Yong Huang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).