| Message ID | 20240905165146.293586-1-den@openvz.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] migration: improve migration_throttle tracepoint | expand |
On Thu, Sep 05, 2024 at 06:51:46PM +0200, Denis V. Lunev via wrote: > Right now this tracepoint is just saying that the guest has been > throttled, but this is not that good for debugging purposes. We should > also know how much the guest is throttled in order to understand > consequences for the guest behaviour. > > The patch moves the tracepoint from migration_trigger_throttle() to > mig_throttle_guest_down() where this information is really available. > This is not a problem as mig_throttle_guest_down() is called in the > only one place. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Peter Xu <peterx@redhat.com> > CC: Fabiano Rosas <farosas@suse.de> Makes sense to me, but maybe we can further move it to cpu_throttle_set()?
On 9/5/24 19:52, Peter Xu wrote: > On Thu, Sep 05, 2024 at 06:51:46PM +0200, Denis V. Lunev via wrote: >> Right now this tracepoint is just saying that the guest has been >> throttled, but this is not that good for debugging purposes. We should >> also know how much the guest is throttled in order to understand >> consequences for the guest behaviour. >> >> The patch moves the tracepoint from migration_trigger_throttle() to >> mig_throttle_guest_down() where this information is really available. >> This is not a problem as mig_throttle_guest_down() is called in the >> only one place. >> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Peter Xu <peterx@redhat.com> >> CC: Fabiano Rosas <farosas@suse.de> > Makes sense to me, but maybe we can further move it to cpu_throttle_set()? > in that case we should rename the tracepoint as in that case the module would be different :) 4 90 system/cpu-throttle.c <<cpu_throttle_set>> void cpu_throttle_set(int new_throttle_pct) Will it be OK for you? Den
On Thu, Sep 05, 2024 at 08:00:54PM +0200, Denis V. Lunev wrote: > On 9/5/24 19:52, Peter Xu wrote: > > On Thu, Sep 05, 2024 at 06:51:46PM +0200, Denis V. Lunev via wrote: > > > Right now this tracepoint is just saying that the guest has been > > > throttled, but this is not that good for debugging purposes. We should > > > also know how much the guest is throttled in order to understand > > > consequences for the guest behaviour. > > > > > > The patch moves the tracepoint from migration_trigger_throttle() to > > > mig_throttle_guest_down() where this information is really available. > > > This is not a problem as mig_throttle_guest_down() is called in the > > > only one place. > > > > > > Signed-off-by: Denis V. Lunev <den@openvz.org> > > > CC: Peter Xu <peterx@redhat.com> > > > CC: Fabiano Rosas <farosas@suse.de> > > Makes sense to me, but maybe we can further move it to cpu_throttle_set()? > > > in that case we should rename the tracepoint as in that case the > module would be different :) > > 4 90 system/cpu-throttle.c <<cpu_throttle_set>> > void cpu_throttle_set(int new_throttle_pct) > > Will it be OK for you? Yes. Thanks.
diff --git a/migration/ram.c b/migration/ram.c index edec1a2d07..e587f11120 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -548,9 +548,11 @@ static void mig_throttle_guest_down(uint64_t bytes_dirty_period, uint64_t throttle_now = cpu_throttle_get_percentage(); uint64_t cpu_now, cpu_ideal, throttle_inc; + int new_throttle_pct; + /* We have not started throttling yet. Let's start it. */ if (!cpu_throttle_active()) { - cpu_throttle_set(pct_initial); + new_throttle_pct = pct_initial; } else { /* Throttling already on, just increase the rate */ if (!pct_tailslow) { @@ -563,8 +565,10 @@ static void mig_throttle_guest_down(uint64_t bytes_dirty_period, bytes_dirty_period); throttle_inc = MIN(cpu_now - cpu_ideal, pct_increment); } - cpu_throttle_set(MIN(throttle_now + throttle_inc, pct_max)); + new_throttle_pct = MIN(throttle_now + throttle_inc, pct_max); } + trace_migration_throttle(new_throttle_pct); + cpu_throttle_set(new_throttle_pct); } void mig_throttle_counter_reset(void) @@ -1032,7 +1036,6 @@ static void migration_trigger_throttle(RAMState *rs) (++rs->dirty_rate_high_cnt >= 2)) { rs->dirty_rate_high_cnt = 0; if (migrate_auto_converge()) { - trace_migration_throttle(); mig_throttle_guest_down(bytes_dirty_period, bytes_dirty_threshold); } else if (migrate_dirty_limit()) { diff --git a/migration/trace-events b/migration/trace-events index 0b7c3324fb..099c828535 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -95,7 +95,7 @@ get_queued_page_not_dirty(const char *block_name, uint64_t tmp_offset, unsigned 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_throttle(int new_throttle_pct) "guest CPU throttled by %d%%" 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"
Right now this tracepoint is just saying that the guest has been throttled, but this is not that good for debugging purposes. We should also know how much the guest is throttled in order to understand consequences for the guest behaviour. The patch moves the tracepoint from migration_trigger_throttle() to mig_throttle_guest_down() where this information is really available. This is not a problem as mig_throttle_guest_down() is called in the only one place. Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Peter Xu <peterx@redhat.com> CC: Fabiano Rosas <farosas@suse.de> --- migration/ram.c | 9 ++++++--- migration/trace-events | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-)