Re: [Qemu-devel] [PATCH] migration: keep bytes_xfer_prev init'd to zero

[Peter Xu]

On Fri, May 19, 2017 at 10:59:02PM +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 <address@hidden>
> ~
> ---
>  migration/ram.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index f59fdd4..793af39 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -670,10 +670,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
> 
> 

I feel like this patch wants to correctly initialize bytes_xfer_prev,
however I still see problem. E.g., when user specify auto-convergence
during migration, and in the first iteration we'll always have a very
small bytes_xfer_prev (with this patch, it'll be zero) with a very big
bytes_xfer_now (which is the ram_bytes_transferred() value). If so, do
you think squash below change together with current one would be more
accurate?

diff --git a/migration/ram.c b/migration/ram.c
index f59fdd4..e01a218 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -703,7 +703,8 @@ static void migration_bitmap_sync(RAMState *rs)
                throttling */
             bytes_xfer_now = ram_bytes_transferred();
 
-            if (rs->dirty_pages_rate &&
+            /* Skip first iteration when bytes_xfer_prev not inited */
+            if (rs->bytes_xfer_prev && rs->dirty_pages_rate &&
                (rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
                    (bytes_xfer_now - rs->bytes_xfer_prev) / 2) &&
                (rs->dirty_rate_high_cnt++ >= 2)) {

Even I am not sure whether we can move bytes_xfer_prev out of RAMState
since it's only used by migration_bitmap_sync().

Thanks,

-- 
Peter Xu