[Qemu-devel] [PATCH 0/4] migration: autoconverge counter fixes

[Qemu-devel] [PATCH 0/4] migration: autoconverge counter fixes

[Felipe Franciosi]

As discussed in the ML [1], there are some issues around the way ram.c accounts
for transferred data and dirty pages for the auto convergence calculations. The
patches in these series fix that by:

- Keeping bytes_xfer_prev zeroed until it's used
- Ensure bytes_xfer_* counters are accounted for the same period as
  dirty_pages_* counters
- Start throttling in a consistent manner

[1] http://lists.nongnu.org/archive/html/qemu-devel/2017-05/msg05524.html

Felipe Franciosi (4):
  migration: keep bytes_xfer_prev init'd to zero
  migration: set dirty_pages_rate before autoconverge logic
  migration: set bytes_xfer_* outside of autoconverge logic
  migration: use dirty_rate_high_cnt more aggressively

 migration/ram.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

-- 
1.9.5

[Qemu-devel] [PATCH 1/4] migration: keep bytes_xfer_prev init'd to zero

[Felipe Franciosi]

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(-)

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

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

[Peter Xu]

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
> 

-- 
Peter Xu

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

[Juan Quintela]

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>

[Qemu-devel] [PATCH 2/4] migration: set dirty_pages_rate before autoconverge logic

[Felipe Franciosi]

Currently, a "period" in the RAM migration logic is at least a second
long and accounts for what happened since the last period (or the
beginning of the migration). The dirty_pages_rate counter is calculated
at the end this logic.

If the auto convergence capability is enabled from the start of the
migration, it won't be able to use this counter the first time around.
This calculates dirty_pages_rate as soon as a period is deemed over,
which allows for it to be used immediately.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 migration/ram.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 36bf720..495ecbe 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -694,6 +694,10 @@ static void migration_bitmap_sync(RAMState *rs)
 
     /* more than 1 second = 1000 millisecons */
     if (end_time > rs->time_last_bitmap_sync + 1000) {
+        /* calculate period counters */
+        rs->dirty_pages_rate = rs->num_dirty_pages_period * 1000
+            / (end_time - rs->time_last_bitmap_sync);
+
         if (migrate_auto_converge()) {
             /* The following detection logic can be refined later. For now:
                Check to see if the dirtied bytes is 50% more than the approx.
@@ -702,15 +706,14 @@ static void migration_bitmap_sync(RAMState *rs)
                throttling */
             bytes_xfer_now = ram_bytes_transferred();
 
-            if (rs->dirty_pages_rate &&
-               (rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
+            if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
                    (bytes_xfer_now - rs->bytes_xfer_prev) / 2) &&
-               (rs->dirty_rate_high_cnt++ >= 2)) {
+                (rs->dirty_rate_high_cnt++ >= 2)) {
                     trace_migration_throttle();
                     rs->dirty_rate_high_cnt = 0;
                     mig_throttle_guest_down();
-             }
-             rs->bytes_xfer_prev = bytes_xfer_now;
+            }
+            rs->bytes_xfer_prev = bytes_xfer_now;
         }
 
         if (migrate_use_xbzrle()) {
@@ -723,8 +726,8 @@ static void migration_bitmap_sync(RAMState *rs)
             rs->iterations_prev = rs->iterations;
             rs->xbzrle_cache_miss_prev = rs->xbzrle_cache_miss;
         }
-        rs->dirty_pages_rate = rs->num_dirty_pages_period * 1000
-            / (end_time - rs->time_last_bitmap_sync);
+
+        /* reset period counters */
         rs->time_last_bitmap_sync = end_time;
         rs->num_dirty_pages_period = 0;
     }
-- 
1.9.5

Re: [Qemu-devel] [PATCH 2/4] migration: set dirty_pages_rate before autoconverge logic

[Peter Xu]

On Wed, May 24, 2017 at 05:10:01PM +0100, Felipe Franciosi wrote:
> Currently, a "period" in the RAM migration logic is at least a second
> long and accounts for what happened since the last period (or the
> beginning of the migration). The dirty_pages_rate counter is calculated
> at the end this logic.
> 
> If the auto convergence capability is enabled from the start of the
> migration, it won't be able to use this counter the first time around.
> This calculates dirty_pages_rate as soon as a period is deemed over,
> which allows for it to be used immediately.
> 
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>

You fixed the indents as well, but imho it's okay.

Reviewed-by: Peter Xu <peterx@redhat.com>

> ---
>  migration/ram.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 36bf720..495ecbe 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -694,6 +694,10 @@ static void migration_bitmap_sync(RAMState *rs)
>  
>      /* more than 1 second = 1000 millisecons */
>      if (end_time > rs->time_last_bitmap_sync + 1000) {
> +        /* calculate period counters */
> +        rs->dirty_pages_rate = rs->num_dirty_pages_period * 1000
> +            / (end_time - rs->time_last_bitmap_sync);
> +
>          if (migrate_auto_converge()) {
>              /* The following detection logic can be refined later. For now:
>                 Check to see if the dirtied bytes is 50% more than the approx.
> @@ -702,15 +706,14 @@ static void migration_bitmap_sync(RAMState *rs)
>                 throttling */
>              bytes_xfer_now = ram_bytes_transferred();
>  
> -            if (rs->dirty_pages_rate &&
> -               (rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
> +            if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
>                     (bytes_xfer_now - rs->bytes_xfer_prev) / 2) &&
> -               (rs->dirty_rate_high_cnt++ >= 2)) {
> +                (rs->dirty_rate_high_cnt++ >= 2)) {
>                      trace_migration_throttle();
>                      rs->dirty_rate_high_cnt = 0;
>                      mig_throttle_guest_down();
> -             }
> -             rs->bytes_xfer_prev = bytes_xfer_now;
> +            }
> +            rs->bytes_xfer_prev = bytes_xfer_now;
>          }
>  
>          if (migrate_use_xbzrle()) {
> @@ -723,8 +726,8 @@ static void migration_bitmap_sync(RAMState *rs)
>              rs->iterations_prev = rs->iterations;
>              rs->xbzrle_cache_miss_prev = rs->xbzrle_cache_miss;
>          }
> -        rs->dirty_pages_rate = rs->num_dirty_pages_period * 1000
> -            / (end_time - rs->time_last_bitmap_sync);
> +
> +        /* reset period counters */
>          rs->time_last_bitmap_sync = end_time;
>          rs->num_dirty_pages_period = 0;
>      }
> -- 
> 1.9.5
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/4] migration: set dirty_pages_rate before autoconverge logic

[Felipe Franciosi]

> On 25 May 2017, at 01:40, Peter Xu <peterx@redhat.com> wrote:
> 
> On Wed, May 24, 2017 at 05:10:01PM +0100, Felipe Franciosi wrote:
>> Currently, a "period" in the RAM migration logic is at least a second
>> long and accounts for what happened since the last period (or the
>> beginning of the migration). The dirty_pages_rate counter is calculated
>> at the end this logic.
>> 
>> If the auto convergence capability is enabled from the start of the
>> migration, it won't be able to use this counter the first time around.
>> This calculates dirty_pages_rate as soon as a period is deemed over,
>> which allows for it to be used immediately.
>> 
>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> 
> You fixed the indents as well, but imho it's okay.

Yeah a couple of lines were off-by-one space. Fixed it given I was touching the code anyway, hope it's ok with everyone else. Would you normally patch that separately or just mention it in the commit message?

F.

> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
>> ---
>> migration/ram.c | 17 ++++++++++-------
>> 1 file changed, 10 insertions(+), 7 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 36bf720..495ecbe 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -694,6 +694,10 @@ static void migration_bitmap_sync(RAMState *rs)
>> 
>>     /* more than 1 second = 1000 millisecons */
>>     if (end_time > rs->time_last_bitmap_sync + 1000) {
>> +        /* calculate period counters */
>> +        rs->dirty_pages_rate = rs->num_dirty_pages_period * 1000
>> +            / (end_time - rs->time_last_bitmap_sync);
>> +
>>         if (migrate_auto_converge()) {
>>             /* The following detection logic can be refined later. For now:
>>                Check to see if the dirtied bytes is 50% more than the approx.
>> @@ -702,15 +706,14 @@ static void migration_bitmap_sync(RAMState *rs)
>>                throttling */
>>             bytes_xfer_now = ram_bytes_transferred();
>> 
>> -            if (rs->dirty_pages_rate &&
>> -               (rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
>> +            if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
>>                    (bytes_xfer_now - rs->bytes_xfer_prev) / 2) &&
>> -               (rs->dirty_rate_high_cnt++ >= 2)) {
>> +                (rs->dirty_rate_high_cnt++ >= 2)) {
>>                     trace_migration_throttle();
>>                     rs->dirty_rate_high_cnt = 0;
>>                     mig_throttle_guest_down();
>> -             }
>> -             rs->bytes_xfer_prev = bytes_xfer_now;
>> +            }
>> +            rs->bytes_xfer_prev = bytes_xfer_now;
>>         }
>> 
>>         if (migrate_use_xbzrle()) {
>> @@ -723,8 +726,8 @@ static void migration_bitmap_sync(RAMState *rs)
>>             rs->iterations_prev = rs->iterations;
>>             rs->xbzrle_cache_miss_prev = rs->xbzrle_cache_miss;
>>         }
>> -        rs->dirty_pages_rate = rs->num_dirty_pages_period * 1000
>> -            / (end_time - rs->time_last_bitmap_sync);
>> +
>> +        /* reset period counters */
>>         rs->time_last_bitmap_sync = end_time;
>>         rs->num_dirty_pages_period = 0;
>>     }
>> -- 
>> 1.9.5
>> 
> 
> -- 
> Peter Xu

Re: [Qemu-devel] [PATCH 2/4] migration: set dirty_pages_rate before autoconverge logic

[Peter Xu]

On Thu, May 25, 2017 at 10:52:32AM +0000, Felipe Franciosi wrote:
> 
> > On 25 May 2017, at 01:40, Peter Xu <peterx@redhat.com> wrote:
> > 
> > On Wed, May 24, 2017 at 05:10:01PM +0100, Felipe Franciosi wrote:
> >> Currently, a "period" in the RAM migration logic is at least a second
> >> long and accounts for what happened since the last period (or the
> >> beginning of the migration). The dirty_pages_rate counter is calculated
> >> at the end this logic.
> >> 
> >> If the auto convergence capability is enabled from the start of the
> >> migration, it won't be able to use this counter the first time around.
> >> This calculates dirty_pages_rate as soon as a period is deemed over,
> >> which allows for it to be used immediately.
> >> 
> >> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> > 
> > You fixed the indents as well, but imho it's okay.
> 
> Yeah a couple of lines were off-by-one space. Fixed it given I was touching the code anyway, hope it's ok with everyone else. Would you normally patch that separately or just mention it in the commit message?

For me normally I don't intentionally touch code up only for
indentation to make sure commit log won't be affected for those lines.
However I'm also okay if we fix some of them, either separately or
squashed into patch like this.

IMHO at last it really depends on the maintainers' flavor on this. :-)

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/4] migration: set dirty_pages_rate before autoconverge logic

[Juan Quintela]

Felipe Franciosi <felipe@nutanix.com> wrote:
> Currently, a "period" in the RAM migration logic is at least a second
> long and accounts for what happened since the last period (or the
> beginning of the migration). The dirty_pages_rate counter is calculated
> at the end this logic.
>
> If the auto convergence capability is enabled from the start of the
> migration, it won't be able to use this counter the first time around.
> This calculates dirty_pages_rate as soon as a period is deemed over,
> which allows for it to be used immediately.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] [PATCH 3/4] migration: set bytes_xfer_* outside of autoconverge logic

[Felipe Franciosi]

The bytes_xfer_now/prev counters are only used by the auto convergence
logic. However, they are used alongside the dirty_pages_rate counter,
which is calculated (and required) outside of this logic. The problem
with this approach is that if the auto convergence capability is changed
while a migration is ongoing, the relationship of the counters will be
broken.

This moves the management of bytes_xfer_now/prev counters outside of the
auto convergence logic to address this issue.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 migration/ram.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 495ecbe..1a3d9e6 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -697,6 +697,7 @@ static void migration_bitmap_sync(RAMState *rs)
         /* calculate period counters */
         rs->dirty_pages_rate = rs->num_dirty_pages_period * 1000
             / (end_time - rs->time_last_bitmap_sync);
+        bytes_xfer_now = ram_bytes_transferred();
 
         if (migrate_auto_converge()) {
             /* The following detection logic can be refined later. For now:
@@ -704,7 +705,6 @@ static void migration_bitmap_sync(RAMState *rs)
                amount of bytes that just got transferred since the last time we
                were in this routine. If that happens twice, start or increase
                throttling */
-            bytes_xfer_now = ram_bytes_transferred();
 
             if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
                    (bytes_xfer_now - rs->bytes_xfer_prev) / 2) &&
@@ -713,7 +713,6 @@ static void migration_bitmap_sync(RAMState *rs)
                     rs->dirty_rate_high_cnt = 0;
                     mig_throttle_guest_down();
             }
-            rs->bytes_xfer_prev = bytes_xfer_now;
         }
 
         if (migrate_use_xbzrle()) {
@@ -730,6 +729,7 @@ static void migration_bitmap_sync(RAMState *rs)
         /* reset period counters */
         rs->time_last_bitmap_sync = end_time;
         rs->num_dirty_pages_period = 0;
+        rs->bytes_xfer_prev = bytes_xfer_now;
     }
     if (migrate_use_events()) {
         qapi_event_send_migration_pass(rs->bitmap_sync_count, NULL);
-- 
1.9.5

Re: [Qemu-devel] [PATCH 3/4] migration: set bytes_xfer_* outside of autoconverge logic

[Peter Xu]

On Wed, May 24, 2017 at 05:10:02PM +0100, Felipe Franciosi wrote:
> The bytes_xfer_now/prev counters are only used by the auto convergence
> logic. However, they are used alongside the dirty_pages_rate counter,
> which is calculated (and required) outside of this logic. The problem
> with this approach is that if the auto convergence capability is changed
> while a migration is ongoing, the relationship of the counters will be
> broken.
> 
> This moves the management of bytes_xfer_now/prev counters outside of the
> auto convergence logic to address this issue.
> 
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

> ---
>  migration/ram.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 495ecbe..1a3d9e6 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -697,6 +697,7 @@ static void migration_bitmap_sync(RAMState *rs)
>          /* calculate period counters */
>          rs->dirty_pages_rate = rs->num_dirty_pages_period * 1000
>              / (end_time - rs->time_last_bitmap_sync);
> +        bytes_xfer_now = ram_bytes_transferred();
>  
>          if (migrate_auto_converge()) {
>              /* The following detection logic can be refined later. For now:
> @@ -704,7 +705,6 @@ static void migration_bitmap_sync(RAMState *rs)
>                 amount of bytes that just got transferred since the last time we
>                 were in this routine. If that happens twice, start or increase
>                 throttling */
> -            bytes_xfer_now = ram_bytes_transferred();
>  
>              if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
>                     (bytes_xfer_now - rs->bytes_xfer_prev) / 2) &&
> @@ -713,7 +713,6 @@ static void migration_bitmap_sync(RAMState *rs)
>                      rs->dirty_rate_high_cnt = 0;
>                      mig_throttle_guest_down();
>              }
> -            rs->bytes_xfer_prev = bytes_xfer_now;
>          }
>  
>          if (migrate_use_xbzrle()) {
> @@ -730,6 +729,7 @@ static void migration_bitmap_sync(RAMState *rs)
>          /* reset period counters */
>          rs->time_last_bitmap_sync = end_time;
>          rs->num_dirty_pages_period = 0;
> +        rs->bytes_xfer_prev = bytes_xfer_now;
>      }
>      if (migrate_use_events()) {
>          qapi_event_send_migration_pass(rs->bitmap_sync_count, NULL);
> -- 
> 1.9.5
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 3/4] migration: set bytes_xfer_* outside of autoconverge logic

[Juan Quintela]

Felipe Franciosi <felipe@nutanix.com> wrote:
> The bytes_xfer_now/prev counters are only used by the auto convergence
> logic. However, they are used alongside the dirty_pages_rate counter,
> which is calculated (and required) outside of this logic. The problem
> with this approach is that if the auto convergence capability is changed
> while a migration is ongoing, the relationship of the counters will be
> broken.
>
> This moves the management of bytes_xfer_now/prev counters outside of the
> auto convergence logic to address this issue.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] [PATCH 4/4] migration: use dirty_rate_high_cnt more aggressively

[Felipe Franciosi]

The commit message from 070afca25 suggests that dirty_rate_high_cnt
should be used more aggressively to start throttling after two
iterations instead of four. The code, however, only changes the auto
convergence behaviour to throttle after three iterations. This makes the
behaviour more aggressive by kicking off throttling after two iterations
as originally intended.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 migration/ram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 1a3d9e6..26e03a5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -708,7 +708,7 @@ static void migration_bitmap_sync(RAMState *rs)
 
             if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
                    (bytes_xfer_now - rs->bytes_xfer_prev) / 2) &&
-                (rs->dirty_rate_high_cnt++ >= 2)) {
+                (++rs->dirty_rate_high_cnt >= 2)) {
                     trace_migration_throttle();
                     rs->dirty_rate_high_cnt = 0;
                     mig_throttle_guest_down();
-- 
1.9.5

Re: [Qemu-devel] [PATCH 4/4] migration: use dirty_rate_high_cnt more aggressively

[Daniel P. Berrange]

On Wed, May 24, 2017 at 05:10:03PM +0100, Felipe Franciosi wrote:
> The commit message from 070afca25 suggests that dirty_rate_high_cnt
> should be used more aggressively to start throttling after two
> iterations instead of four. The code, however, only changes the auto
> convergence behaviour to throttle after three iterations. This makes the
> behaviour more aggressive by kicking off throttling after two iterations
> as originally intended.

This description looks suspect vs the code. You say it is changing
from "after four" to "after two", but you are merely switching a
post-increment to a pre-increment which can only reduce the boundary
condition by 1, not 2. So either you mean to write

"after two iterations instead of three" 

or

"after three iterations intead of four"

> 
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
>  migration/ram.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 1a3d9e6..26e03a5 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -708,7 +708,7 @@ static void migration_bitmap_sync(RAMState *rs)
>  
>              if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
>                     (bytes_xfer_now - rs->bytes_xfer_prev) / 2) &&
> -                (rs->dirty_rate_high_cnt++ >= 2)) {
> +                (++rs->dirty_rate_high_cnt >= 2)) {
>                      trace_migration_throttle();
>                      rs->dirty_rate_high_cnt = 0;
>                      mig_throttle_guest_down();
> -- 
> 1.9.5
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 4/4] migration: use dirty_rate_high_cnt more aggressively

[Felipe Franciosi]

> On 24 May 2017, at 17:25, Daniel P. Berrange <berrange@redhat.com> wrote:
> 
> On Wed, May 24, 2017 at 05:10:03PM +0100, Felipe Franciosi wrote:
>> The commit message from 070afca25 suggests that dirty_rate_high_cnt
>> should be used more aggressively to start throttling after two
>> iterations instead of four. The code, however, only changes the auto
>> convergence behaviour to throttle after three iterations. This makes the
>> behaviour more aggressive by kicking off throttling after two iterations
>> as originally intended.
> 
> This description looks suspect vs the code. You say it is changing
> from "after four" to "after two", but you are merely switching a
> post-increment to a pre-increment which can only reduce the boundary
> condition by 1, not 2. So either you mean to write
> 
> "after two iterations instead of three" 
> 
> or
> 
> "after three iterations intead of four"

Hi Daniel,

Thanks for the quick feedback. Let me clarify what I meant. If there's still confusion I'll update the commit message to reflect it better.

Pre-070afca25: throttling kicked in after four iterations.
Post-070afca25: throttling kicked in after three iterations (but Jason wrote he meant to start throttling after two).
This patch: throttles kicks in after two iterations (so Jason's code, minus one).

Perhaps where I said "The code, however," I could say "Commit 070afca25, however,". Let me know.

Thanks,
Felipe

> 
>> 
>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>> ---
>> migration/ram.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 1a3d9e6..26e03a5 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -708,7 +708,7 @@ static void migration_bitmap_sync(RAMState *rs)
>> 
>>             if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
>>                    (bytes_xfer_now - rs->bytes_xfer_prev) / 2) &&
>> -                (rs->dirty_rate_high_cnt++ >= 2)) {
>> +                (++rs->dirty_rate_high_cnt >= 2)) {
>>                     trace_migration_throttle();
>>                     rs->dirty_rate_high_cnt = 0;
>>                     mig_throttle_guest_down();
>> -- 
>> 1.9.5
>> 
>> 
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 4/4] migration: use dirty_rate_high_cnt more aggressively

[Peter Xu]

On Wed, May 24, 2017 at 05:10:03PM +0100, Felipe Franciosi wrote:
> The commit message from 070afca25 suggests that dirty_rate_high_cnt
> should be used more aggressively to start throttling after two
> iterations instead of four. The code, however, only changes the auto
> convergence behaviour to throttle after three iterations. This makes the
> behaviour more aggressive by kicking off throttling after two iterations
> as originally intended.

For this one, I don't think fixing the code to match the commit
message is that important. Instead, for me this patch looks more like
something "changed iteration loops from 3 to 2". So the point is, what
would be the best practical number for it. And when we change an
existing value, we should have some reason, since it'll change
behavior of existing user (though I'm not sure whether this one will
affect much).

I believe with higher dirty_rate_high_cnt, we have more smooth
throttling, but it'll be slower in responding; While if lower or even
remove it, we'll get very fast throttling response speed but I guess
it may be more possible to report a false positive? IMHO here 3 is
okay since after all we are solving the problem of unconverged
migration, so as long as we can converge, I think it'll be fine.

Thanks,

> 
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
>  migration/ram.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 1a3d9e6..26e03a5 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -708,7 +708,7 @@ static void migration_bitmap_sync(RAMState *rs)
>  
>              if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
>                     (bytes_xfer_now - rs->bytes_xfer_prev) / 2) &&
> -                (rs->dirty_rate_high_cnt++ >= 2)) {
> +                (++rs->dirty_rate_high_cnt >= 2)) {
>                      trace_migration_throttle();
>                      rs->dirty_rate_high_cnt = 0;
>                      mig_throttle_guest_down();
> -- 
> 1.9.5
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 4/4] migration: use dirty_rate_high_cnt more aggressively

[Felipe Franciosi]

+ Matthew Rosato, original reviewer of 070afca25

> On 25 May 2017, at 02:03, Peter Xu <peterx@redhat.com> wrote:
> 
> On Wed, May 24, 2017 at 05:10:03PM +0100, Felipe Franciosi wrote:
>> The commit message from 070afca25 suggests that dirty_rate_high_cnt
>> should be used more aggressively to start throttling after two
>> iterations instead of four. The code, however, only changes the auto
>> convergence behaviour to throttle after three iterations. This makes the
>> behaviour more aggressive by kicking off throttling after two iterations
>> as originally intended.
> 
> For this one, I don't think fixing the code to match the commit
> message is that important. Instead, for me this patch looks more like
> something "changed iteration loops from 3 to 2".

I agree. If we decide a v2 is needed I can amend the commit message accordingly.

> So the point is, what
> would be the best practical number for it. And when we change an
> existing value, we should have some reason, since it'll change
> behavior of existing user (though I'm not sure whether this one will
> affect much).

We've done a few tests with this internally using various workloads (DBs, synthetic mem stress, &c.). In summary, and along the lines with how Qemu implements auto convergence today, I would say this counter should be removed.

Consider that when live migrating a large-ish VM (100GB+ RAM), the network will be under stress for (at least) several minutes. At the same time, the sole purpose of this counter is to attempt convergence without having to throttle the guest. That is, it defers throttling in the hope that either the guest calms down or that the network gets less congested.

For real workloads, both cases are unlikely to happen. Throttling will eventually be needed otherwise the migration will not converge.

> I believe with higher dirty_rate_high_cnt, we have more smooth
> throttling, but it'll be slower in responding; While if lower or even
> remove it, we'll get very fast throttling response speed but I guess
> it may be more possible to report a false positive?

With a higher dirty_rate_high_cnt, migration will simply take longer (if not converging). For real workloads, it doesn't change how much throttling is required. For spiky or varied workloads, it gives a chance for migration to converge without throttling, at the cost of massive network stress and a longer overall migration time (which is bad user experience IMO).

> IMHO here 3 is
> okay since after all we are solving the problem of unconverged
> migration, so as long as we can converge, I think it'll be fine.

Based on 070afca25's commit message, Jason seemed to think that four was too much and meant to change it to two. As explained above, I'd be in favour of removing this counter altogether, or at least make it configurable (perhaps a #define would be enough an improvement for now). This patch is intended as a compromise by effectively using two.

Great feedback! Thanks again.
Felipe


> 
> Thanks,
> 
>> 
>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>> ---
>> migration/ram.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 1a3d9e6..26e03a5 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -708,7 +708,7 @@ static void migration_bitmap_sync(RAMState *rs)
>> 
>>             if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
>>                    (bytes_xfer_now - rs->bytes_xfer_prev) / 2) &&
>> -                (rs->dirty_rate_high_cnt++ >= 2)) {
>> +                (++rs->dirty_rate_high_cnt >= 2)) {
>>                     trace_migration_throttle();
>>                     rs->dirty_rate_high_cnt = 0;
>>                     mig_throttle_guest_down();
>> -- 
>> 1.9.5
>> 
> 
> -- 
> Peter Xu

Re: [Qemu-devel] [PATCH 4/4] migration: use dirty_rate_high_cnt more aggressively

[Peter Xu]

On Thu, May 25, 2017 at 11:20:02AM +0000, Felipe Franciosi wrote:
> + Matthew Rosato, original reviewer of 070afca25
> 
> > On 25 May 2017, at 02:03, Peter Xu <peterx@redhat.com> wrote:
> > 
> > On Wed, May 24, 2017 at 05:10:03PM +0100, Felipe Franciosi wrote:
> >> The commit message from 070afca25 suggests that dirty_rate_high_cnt
> >> should be used more aggressively to start throttling after two
> >> iterations instead of four. The code, however, only changes the auto
> >> convergence behaviour to throttle after three iterations. This makes the
> >> behaviour more aggressive by kicking off throttling after two iterations
> >> as originally intended.
> > 
> > For this one, I don't think fixing the code to match the commit
> > message is that important. Instead, for me this patch looks more like
> > something "changed iteration loops from 3 to 2".
> 
> I agree. If we decide a v2 is needed I can amend the commit message accordingly.
> 
> > So the point is, what
> > would be the best practical number for it. And when we change an
> > existing value, we should have some reason, since it'll change
> > behavior of existing user (though I'm not sure whether this one will
> > affect much).
> 
> We've done a few tests with this internally using various workloads (DBs, synthetic mem stress, &c.). In summary, and along the lines with how Qemu implements auto convergence today, I would say this counter should be removed.
> 
> Consider that when live migrating a large-ish VM (100GB+ RAM), the network will be under stress for (at least) several minutes. At the same time, the sole purpose of this counter is to attempt convergence without having to throttle the guest. That is, it defers throttling in the hope that either the guest calms down or that the network gets less congested.
> 
> For real workloads, both cases are unlikely to happen. Throttling will eventually be needed otherwise the migration will not converge.

I am not much experienced with using auto convergence with real
workload, but from what you explained I see it a reasonable change to
even remove it (that sounds more persuasive to me than "just try to
follow what the commit message said", with test results :), especially
considering that we have cpu-throttle-initial and
cpu-throttle-increment parameters as tunables, so we should be fine
even we want to tune the speed down a bit in the future.

> 
> > I believe with higher dirty_rate_high_cnt, we have more smooth
> > throttling, but it'll be slower in responding; While if lower or even
> > remove it, we'll get very fast throttling response speed but I guess
> > it may be more possible to report a false positive?
> 
> With a higher dirty_rate_high_cnt, migration will simply take longer (if not converging). For real workloads, it doesn't change how much throttling is required. For spiky or varied workloads, it gives a chance for migration to converge without throttling, at the cost of massive network stress and a longer overall migration time (which is bad user experience IMO).
> 
> > IMHO here 3 is
> > okay since after all we are solving the problem of unconverged
> > migration, so as long as we can converge, I think it'll be fine.
> 
> Based on 070afca25's commit message, Jason seemed to think that four was too much and meant to change it to two. As explained above, I'd be in favour of removing this counter altogether, or at least make it configurable (perhaps a #define would be enough an improvement for now). This patch is intended as a compromise by effectively using two.

If to be a tunable, maybe another MigrationParameter? But I don't know
whether it would really worth it since the other two can do more
fine-grained tunes already. So I would prefer to remove it as well if
thorough tests are carried out.

Maybe we can also wait to see how other people think about it.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 4/4] migration: use dirty_rate_high_cnt more aggressively

[Juan Quintela]

Felipe Franciosi <felipe@nutanix.com> wrote:
> The commit message from 070afca25 suggests that dirty_rate_high_cnt
> should be used more aggressively to start throttling after two
> iterations instead of four. The code, however, only changes the auto
> convergence behaviour to throttle after three iterations. This makes the
> behaviour more aggressive by kicking off throttling after two iterations
> as originally intended.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

First, Felipe is doing performance testing with the changes.
Second, if you want autoconverge slower, you can use smaller increments.
So, I think that this is ok.

It is more, if you want you can send numbers of your workloads with
current parameter and removing it altogether to convince me that it is a
good idea just to drop it.

Thanks, Juan.