Re: [Qemu-devel] [PATCH 1/1] migration: calculate expected_downtime considering redirtied ram

[Juan Quintela <quintela@redhat.com>]

bala24@linux.vnet.ibm.com wrote:
> From: Balamuruhan S <bala24@linux.vnet.ibm.com>
>
> currently we calculate expected_downtime by time taken to transfer
> remaining ram, but during the time we had transferred remaining ram
> few pages of ram might be redirtied and we need to retransfer it,
> so it is better to consider them for calculating expected_downtime
> for getting more accurate values.
>
> Total ram to be transferred = remaining ram + (redirtied ram at the
>                                                time when the remaining
>                                                ram gets transferred)
>
> redirtied ram = dirty_pages_rate * time taken to transfer remaining ram
>
> redirtied ram = dirty_pages_rate * (remaining ram / bandwidth)
>
> expected_downtime = (remaining ram + redirtied ram) / bandwidth
>
> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
> ---
>  migration/migration.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index ffc4d9e556..dc38e9a380 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2903,7 +2903,13 @@ static void migration_update_counters(MigrationState *s,
>       * recalculate. 10000 is a small enough number for our purposes
>       */
>      if (ram_counters.dirty_pages_rate && transferred > 10000) {
> -        s->expected_downtime = ram_counters.remaining / bandwidth;
> +        /* Time required to transfer remaining ram */
> +        remaining_ram_transfer_time = ram_counters.remaining / bandwidth

missing semicolon

> +
> +        /* redirty of ram at the time remaining ram gets transferred*/
> +        newly_dirtied_ram = ram_counters.dirty_pages_rate * remaining_ram_transfer_time

the same.

Declaration of the new variables is also missing.

> +        s->expected_downtime = (ram_counters.remaining + newly_dirtied_ram) / bandwidth;
>      }
>  
>      qemu_file_reset_rate_limit(s->to_dst_file);

About the numbers, I am not against it.  It is an heuristic.  Without
numbers (and it is very load dependent) it is not clear that this one is
going to be much worse/better than previous one (this should be a bit
better, though).

Thanks, Juan.