| Message ID | 168618975839.6361.17407633874747688653-6@git.sr.ht |
|---|---|
| State | New |
| Headers | show |
| Series | migration: introduce dirtylimit capability | expand |
~hyman <hyman@git.sr.ht> wrote: > From: Hyman Huang(黄勇) <yong.huang@smartx.com> To speed thinkng up, 1-5 are included on next Migration PULL request. > Implement dirty-limit convergence algo for live migration, > which is kind of like auto-converge algo but using dirty-limit > instead of cpu throttle to make migration convergent. > > Enable dirty page limit if dirty_rate_high_cnt greater than 2 > when dirty-limit capability enabled, Disable dirty-limit if > migration be cancled. Nit: canceled. > > Note that "set_vcpu_dirty_limit", "cancel_vcpu_dirty_limit" > commands are not allowed during dirty-limit live migration. > > Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > + * Enable dirty-limit to throttle down the guest > + */ > +static void migration_dirty_limit_guest(void) > +{ > + static int64_t quota_dirtyrate; quota_dirtyrate deserves at least a comment. I guess it means the current quota_dirty_rate that is set, but no clue. > + MigrationState *s = migrate_get_current(); > + > + /* > + * If dirty limit already enabled and migration parameter > + * vcpu-dirty-limit untouched. > + */ > + if (dirtylimit_in_service() && > + quota_dirtyrate == s->parameters.vcpu_dirty_limit) { > + return; > + } > + > + quota_dirtyrate = s->parameters.vcpu_dirty_limit; > + > + /* Set or update quota dirty limit */ > + qmp_set_vcpu_dirty_limit(false, -1, quota_dirtyrate, NULL); Care to explain why do we have to "reset" the quota? Or why we can't set it when the user issues the command, only when we throttle the guest? > + trace_migration_dirty_limit_guest(quota_dirtyrate); > +} > + Split this patch in two: a - the logic change b - the introduction of dirty limit. Old code: /* During block migration the auto-converge logic incorrectly detects * that ram migration makes no progress. Avoid this by disabling the * throttling logic during the bulk phase of block migration. */ if (blk_mig_bulk_active()) { return; } if (migrate_auto_converge()) { /* The following detection logic can be refined later. For now: Check to see if the ratio between dirtied bytes and the approx. amount of bytes that just got transferred since the last time we were in this routine reaches the threshold. If that happens twice, start or increase throttling. */ if ((bytes_dirty_period > bytes_dirty_threshold) && (++rs->dirty_rate_high_cnt >= 2)) { trace_migration_throttle(); rs->dirty_rate_high_cnt = 0; mig_throttle_guest_down(bytes_dirty_period, bytes_dirty_threshold); } } New code: /* * The following detection logic can be refined later. For now: * Check to see if the ratio between dirtied bytes and the approx. * amount of bytes that just got transferred since the last time * we were in this routine reaches the threshold. If that happens * twice, start or increase throttling. */ if ((bytes_dirty_period > bytes_dirty_threshold) && (++rs->dirty_rate_high_cnt >= 2)) { rs->dirty_rate_high_cnt = 0; /* * During block migration the auto-converge logic incorrectly detects * that ram migration makes no progress. Avoid this by disabling the * throttling logic during the bulk phase of block migration */ if (blk_mig_bulk_active()) { return; } if (migrate_auto_converge()) { trace_migration_throttle(); mig_throttle_guest_down(bytes_dirty_period, bytes_dirty_threshold); } else if (migrate_dirty_limit()) { migration_dirty_limit_guest(); } } Questions: - Why are we changing blk_mig_bulk_active() position? I think that the old code have it in the right place. Additionally, you just changefd to this version a couple of patches agon. > int64_t cpu_index, > Error **errp) > { > + MigrationState *ms = migrate_get_current(); > + > if (!kvm_enabled() || !kvm_dirty_ring_enabled()) { > return; > } > @@ -453,6 +455,15 @@ void qmp_cancel_vcpu_dirty_limit(bool has_cpu_index, > return; > } > > + if (migration_is_running(ms->state) && > + (!qemu_thread_is_self(&ms->thread)) && > + migrate_dirty_limit() && > + dirtylimit_in_service()) { > + error_setg(errp, "can't cancel dirty page limit while" > + " migration is running"); Error message is bad or wrong. You can cancel the dirty page, you ust need to be on the main thread. Or I am missing something? > + return; > + } > + > dirtylimit_state_lock(); > > if (has_cpu_index) { > @@ -488,6 +499,8 @@ void qmp_set_vcpu_dirty_limit(bool has_cpu_index, > uint64_t dirty_rate, > Error **errp) > { > + MigrationState *ms = migrate_get_current(); > + > if (!kvm_enabled() || !kvm_dirty_ring_enabled()) { > error_setg(errp, "dirty page limit feature requires KVM with" > " accelerator property 'dirty-ring-size' set'") > @@ -504,6 +517,15 @@ void qmp_set_vcpu_dirty_limit(bool has_cpu_index, > return; > } > > + if (migration_is_running(ms->state) && > + (!qemu_thread_is_self(&ms->thread)) && > + migrate_dirty_limit() && > + dirtylimit_in_service()) { > + error_setg(errp, "can't cancel dirty page limit while" > + " migration is running"); > + return; > + } If you use such a complex expression twice, I think that creating a helper function is a good idea. Later, Juan.
On Wed, Jun 14, 2023 at 1:50 AM Juan Quintela <quintela@redhat.com> wrote: > ~hyman <hyman@git.sr.ht> wrote: > > From: Hyman Huang(黄勇) <yong.huang@smartx.com> > > To speed thinkng up, 1-5 are included on next Migration PULL request. > OK, I'll post the next version only contain the last 3 commits. > > Implement dirty-limit convergence algo for live migration, > > which is kind of like auto-converge algo but using dirty-limit > > instead of cpu throttle to make migration convergent. > > > > Enable dirty page limit if dirty_rate_high_cnt greater than 2 > > when dirty-limit capability enabled, Disable dirty-limit if > > migration be cancled. > > Nit: canceled. get it. > > > > Note that "set_vcpu_dirty_limit", "cancel_vcpu_dirty_limit" > > commands are not allowed during dirty-limit live migration. > > > > Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com> > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > > > > + * Enable dirty-limit to throttle down the guest > > + */ > > +static void migration_dirty_limit_guest(void) > > +{ > > + static int64_t quota_dirtyrate; > > quota_dirtyrate deserves at least a comment. > > I guess it means the current quota_dirty_rate that is set, but no clue. OK. I'll comment it next version. > > > + MigrationState *s = migrate_get_current(); > > + > > + /* > > + * If dirty limit already enabled and migration parameter > > + * vcpu-dirty-limit untouched. > > + */ > > + if (dirtylimit_in_service() && > > + quota_dirtyrate == s->parameters.vcpu_dirty_limit) { > > + return; > > + } > > + > > + quota_dirtyrate = s->parameters.vcpu_dirty_limit; > > + > > + /* Set or update quota dirty limit */ > > + qmp_set_vcpu_dirty_limit(false, -1, quota_dirtyrate, NULL); > > Care to explain why do we have to "reset" the quota? Or why we can't > set it when the user issues the command, only when we throttle the guest? Indeed, -1 is misleading, the first parameter means the set all vcpu a quota dirtyrate, and the second parameter is meaningless if the first parameter is false. The comment will be like this next version? /* Set all vCPU a quota dirtyrate, note that the second parameter will be ignored if setting all vCPU for a vm. */ > > + trace_migration_dirty_limit_guest(quota_dirtyrate); > > +} > > + > > Split this patch in two: > > a - the logic change > b - the introduction of dirty limit. > > Ok, get it. > > Old code: > > /* During block migration the auto-converge logic incorrectly detects > * that ram migration makes no progress. Avoid this by disabling the > * throttling logic during the bulk phase of block migration. */ > if (blk_mig_bulk_active()) { > return; > } > > if (migrate_auto_converge()) { > /* The following detection logic can be refined later. For now: > Check to see if the ratio between dirtied bytes and the approx. > amount of bytes that just got transferred since the last time > we were in this routine reaches the threshold. If that happens > twice, start or increase throttling. */ > > if ((bytes_dirty_period > bytes_dirty_threshold) && > (++rs->dirty_rate_high_cnt >= 2)) { > trace_migration_throttle(); > rs->dirty_rate_high_cnt = 0; > mig_throttle_guest_down(bytes_dirty_period, > bytes_dirty_threshold); > } > } > > New code: > /* > * The following detection logic can be refined later. For now: > * Check to see if the ratio between dirtied bytes and the approx. > * amount of bytes that just got transferred since the last time > * we were in this routine reaches the threshold. If that happens > * twice, start or increase throttling. > */ > > if ((bytes_dirty_period > bytes_dirty_threshold) && > (++rs->dirty_rate_high_cnt >= 2)) { > rs->dirty_rate_high_cnt = 0; > /* > * During block migration the auto-converge logic incorrectly > detects > * that ram migration makes no progress. Avoid this by disabling > the > * throttling logic during the bulk phase of block migration > */ > if (blk_mig_bulk_active()) { > return; > } > > if (migrate_auto_converge()) { > trace_migration_throttle(); > mig_throttle_guest_down(bytes_dirty_period, > bytes_dirty_threshold); > } else if (migrate_dirty_limit()) { > migration_dirty_limit_guest(); > } > } > > Questions: > > - Why are we changing blk_mig_bulk_active() position? > I think that the old code have it in the right place. Additionally, > you just changefd to this version a couple of patches agon. > Yes, indeed, this modification make no sense, i'll fix it next version. > > > > > > int64_t cpu_index, > > Error **errp) > > { > > + MigrationState *ms = migrate_get_current(); > > + > > if (!kvm_enabled() || !kvm_dirty_ring_enabled()) { > > return; > > } > > @@ -453,6 +455,15 @@ void qmp_cancel_vcpu_dirty_limit(bool has_cpu_index, > > return; > > } > > > > + if (migration_is_running(ms->state) && > > + (!qemu_thread_is_self(&ms->thread)) && > > + migrate_dirty_limit() && > > + dirtylimit_in_service()) { > > + error_setg(errp, "can't cancel dirty page limit while" > > + " migration is running"); > > Error message is bad or wrong. > You can cancel the dirty page, you ust need to be on the main thread. > > Or I am missing something? > > Migration, IMHO, shares the same quota dirty rate stored in the global variable "dirtylimit_state ", if we cancel the dirty limit, it will make the throttle not work and the migration will be affected. > > > > + return; > > + } > > + > > dirtylimit_state_lock(); > > > > if (has_cpu_index) { > > @@ -488,6 +499,8 @@ void qmp_set_vcpu_dirty_limit(bool has_cpu_index, > > uint64_t dirty_rate, > > Error **errp) > > { > > + MigrationState *ms = migrate_get_current(); > > + > > if (!kvm_enabled() || !kvm_dirty_ring_enabled()) { > > error_setg(errp, "dirty page limit feature requires KVM with" > > " accelerator property 'dirty-ring-size' set'") > > @@ -504,6 +517,15 @@ void qmp_set_vcpu_dirty_limit(bool has_cpu_index, > > return; > > } > > > > + if (migration_is_running(ms->state) && > > + (!qemu_thread_is_self(&ms->thread)) && > > + migrate_dirty_limit() && > > + dirtylimit_in_service()) { > > + error_setg(errp, "can't cancel dirty page limit while" > > + " migration is running"); > > + return; > > + } > > If you use such a complex expression twice, I think that creating a > helper function is a good idea. > Ok, get it > > Later, Juan. > > Hyman
diff --git a/migration/migration.c b/migration/migration.c index dc05c6f6ea..4278b48af0 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -165,6 +165,9 @@ void migration_cancel(const Error *error) if (error) { migrate_set_error(current_migration, error); } + if (migrate_dirty_limit()) { + qmp_cancel_vcpu_dirty_limit(false, -1, NULL); + } migrate_fd_cancel(current_migration); } diff --git a/migration/ram.c b/migration/ram.c index 132f1a81d9..d26c7a8193 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -46,6 +46,7 @@ #include "qapi/error.h" #include "qapi/qapi-types-migration.h" #include "qapi/qapi-events-migration.h" +#include "qapi/qapi-commands-migration.h" #include "qapi/qmp/qerror.h" #include "trace.h" #include "exec/ram_addr.h" @@ -59,6 +60,8 @@ #include "multifd.h" #include "sysemu/runstate.h" #include "options.h" +#include "sysemu/dirtylimit.h" +#include "sysemu/kvm.h" #include "hw/boards.h" /* for machine_dump_guest_core() */ @@ -983,6 +986,30 @@ static void migration_update_rates(RAMState *rs, int64_t end_time) } } +/* + * Enable dirty-limit to throttle down the guest + */ +static void migration_dirty_limit_guest(void) +{ + static int64_t quota_dirtyrate; + MigrationState *s = migrate_get_current(); + + /* + * If dirty limit already enabled and migration parameter + * vcpu-dirty-limit untouched. + */ + if (dirtylimit_in_service() && + quota_dirtyrate == s->parameters.vcpu_dirty_limit) { + return; + } + + quota_dirtyrate = s->parameters.vcpu_dirty_limit; + + /* Set or update quota dirty limit */ + qmp_set_vcpu_dirty_limit(false, -1, quota_dirtyrate, NULL); + trace_migration_dirty_limit_guest(quota_dirtyrate); +} + static void migration_trigger_throttle(RAMState *rs) { uint64_t threshold = migrate_throttle_trigger_threshold(); @@ -991,26 +1018,32 @@ static void migration_trigger_throttle(RAMState *rs) uint64_t bytes_dirty_period = rs->num_dirty_pages_period * TARGET_PAGE_SIZE; uint64_t bytes_dirty_threshold = bytes_xfer_period * threshold / 100; - /* During block migration the auto-converge logic incorrectly detects - * that ram migration makes no progress. Avoid this by disabling the - * throttling logic during the bulk phase of block migration. */ - if (blk_mig_bulk_active()) { - return; - } + /* + * The following detection logic can be refined later. For now: + * Check to see if the ratio between dirtied bytes and the approx. + * amount of bytes that just got transferred since the last time + * we were in this routine reaches the threshold. If that happens + * twice, start or increase throttling. + */ - if (migrate_auto_converge()) { - /* The following detection logic can be refined later. For now: - Check to see if the ratio between dirtied bytes and the approx. - amount of bytes that just got transferred since the last time - we were in this routine reaches the threshold. If that happens - twice, start or increase throttling. */ + if ((bytes_dirty_period > bytes_dirty_threshold) && + (++rs->dirty_rate_high_cnt >= 2)) { + rs->dirty_rate_high_cnt = 0; + /* + * During block migration the auto-converge logic incorrectly detects + * that ram migration makes no progress. Avoid this by disabling the + * throttling logic during the bulk phase of block migration + */ + if (blk_mig_bulk_active()) { + return; + } - if ((bytes_dirty_period > bytes_dirty_threshold) && - (++rs->dirty_rate_high_cnt >= 2)) { + if (migrate_auto_converge()) { trace_migration_throttle(); - rs->dirty_rate_high_cnt = 0; mig_throttle_guest_down(bytes_dirty_period, bytes_dirty_threshold); + } else if (migrate_dirty_limit()) { + migration_dirty_limit_guest(); } } } diff --git a/migration/trace-events b/migration/trace-events index cdaef7a1ea..c5cb280d95 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -91,6 +91,7 @@ migration_bitmap_sync_start(void) "" migration_bitmap_sync_end(uint64_t dirty_pages) "dirty_pages %" PRIu64 migration_bitmap_clear_dirty(char *str, uint64_t start, uint64_t size, unsigned long page) "rb %s start 0x%"PRIx64" size 0x%"PRIx64" page 0x%lx" migration_throttle(void) "" +migration_dirty_limit_guest(int64_t dirtyrate) "guest dirty page rate limit %" PRIi64 " MB/s" ram_discard_range(const char *rbname, uint64_t start, size_t len) "%s: start: %" PRIx64 " %zx" ram_load_loop(const char *rbname, uint64_t addr, int flags, void *host) "%s: addr: 0x%" PRIx64 " flags: 0x%x host: %p" ram_load_postcopy_loop(int channel, uint64_t addr, int flags) "chan=%d addr=0x%" PRIx64 " flags=0x%x" diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c index 3f1103b04b..ee47158986 100644 --- a/softmmu/dirtylimit.c +++ b/softmmu/dirtylimit.c @@ -440,6 +440,8 @@ void qmp_cancel_vcpu_dirty_limit(bool has_cpu_index, int64_t cpu_index, Error **errp) { + MigrationState *ms = migrate_get_current(); + if (!kvm_enabled() || !kvm_dirty_ring_enabled()) { return; } @@ -453,6 +455,15 @@ void qmp_cancel_vcpu_dirty_limit(bool has_cpu_index, return; } + if (migration_is_running(ms->state) && + (!qemu_thread_is_self(&ms->thread)) && + migrate_dirty_limit() && + dirtylimit_in_service()) { + error_setg(errp, "can't cancel dirty page limit while" + " migration is running"); + return; + } + dirtylimit_state_lock(); if (has_cpu_index) { @@ -488,6 +499,8 @@ void qmp_set_vcpu_dirty_limit(bool has_cpu_index, uint64_t dirty_rate, Error **errp) { + MigrationState *ms = migrate_get_current(); + if (!kvm_enabled() || !kvm_dirty_ring_enabled()) { error_setg(errp, "dirty page limit feature requires KVM with" " accelerator property 'dirty-ring-size' set'"); @@ -504,6 +517,15 @@ void qmp_set_vcpu_dirty_limit(bool has_cpu_index, return; } + if (migration_is_running(ms->state) && + (!qemu_thread_is_self(&ms->thread)) && + migrate_dirty_limit() && + dirtylimit_in_service()) { + error_setg(errp, "can't cancel dirty page limit while" + " migration is running"); + return; + } + dirtylimit_state_lock(); if (!dirtylimit_in_service()) {