| Message ID | 1495642203-12702-2-git-send-email-felipe@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 24, 2017 at 05:10:00PM +0100, Felipe Franciosi wrote: > The first time migration_bitmap_sync() is called, bytes_xfer_prev is set > to ram_state.bytes_transferred which is, at this point, zero. The next > time migration_bitmap_sync() is called, an iteration has happened and > bytes_xfer_prev is set to 'x' bytes. Most likely, more than one second > has passed, so the auto converge logic will be triggered and > bytes_xfer_now will also be set to 'x' bytes. > > This condition is currently masked by dirty_rate_high_cnt, which will > wait for a few iterations before throttling. It would otherwise always > assume zero bytes have been copied and therefore throttle the guest > (possibly) prematurely. > > Given bytes_xfer_prev is only used by the auto convergence logic, it > makes sense to only set its value after a check has been made against > bytes_xfer_now. > > Signed-off-by: Felipe Franciosi <felipe@nutanix.com> Yes, with patch 3, I think it should be accurate. Reviewed-by: Peter Xu <peterx@redhat.com> > --- > migration/ram.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index c07a9c0..36bf720 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -673,10 +673,6 @@ static void migration_bitmap_sync(RAMState *rs) > > rs->bitmap_sync_count++; > > - if (!rs->bytes_xfer_prev) { > - rs->bytes_xfer_prev = ram_bytes_transferred(); > - } > - > if (!rs->time_last_bitmap_sync) { > rs->time_last_bitmap_sync = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > } > -- > 1.9.5 >
Felipe Franciosi <felipe@nutanix.com> wrote: > The first time migration_bitmap_sync() is called, bytes_xfer_prev is set > to ram_state.bytes_transferred which is, at this point, zero. The next > time migration_bitmap_sync() is called, an iteration has happened and > bytes_xfer_prev is set to 'x' bytes. Most likely, more than one second > has passed, so the auto converge logic will be triggered and > bytes_xfer_now will also be set to 'x' bytes. > > This condition is currently masked by dirty_rate_high_cnt, which will > wait for a few iterations before throttling. It would otherwise always > assume zero bytes have been copied and therefore throttle the guest > (possibly) prematurely. > > Given bytes_xfer_prev is only used by the auto convergence logic, it > makes sense to only set its value after a check has been made against > bytes_xfer_now. > > Signed-off-by: Felipe Franciosi <felipe@nutanix.com> Reviewed-by: Juan Quintela <quintela@redhat.com>
diff --git a/migration/ram.c b/migration/ram.c index c07a9c0..36bf720 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -673,10 +673,6 @@ static void migration_bitmap_sync(RAMState *rs) rs->bitmap_sync_count++; - if (!rs->bytes_xfer_prev) { - rs->bytes_xfer_prev = ram_bytes_transferred(); - } - if (!rs->time_last_bitmap_sync) { rs->time_last_bitmap_sync = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); }
The first time migration_bitmap_sync() is called, bytes_xfer_prev is set to ram_state.bytes_transferred which is, at this point, zero. The next time migration_bitmap_sync() is called, an iteration has happened and bytes_xfer_prev is set to 'x' bytes. Most likely, more than one second has passed, so the auto converge logic will be triggered and bytes_xfer_now will also be set to 'x' bytes. This condition is currently masked by dirty_rate_high_cnt, which will wait for a few iterations before throttling. It would otherwise always assume zero bytes have been copied and therefore throttle the guest (possibly) prematurely. Given bytes_xfer_prev is only used by the auto convergence logic, it makes sense to only set its value after a check has been made against bytes_xfer_now. Signed-off-by: Felipe Franciosi <felipe@nutanix.com> --- migration/ram.c | 4 ---- 1 file changed, 4 deletions(-)