Re: [Qemu-devel] [PATCH 1/1] migration: fix expected_downtime

[Dr. David Alan Gilbert]

* Igor Redko (address@hidden) wrote:
> On 28.09.2015 22:22, Dr. David Alan Gilbert wrote:
> >* Denis V. Lunev (address@hidden) wrote:
> >>From: Igor Redko <address@hidden>
> >>
> >>To get this estimation we must divide pending_size by bandwidth
> >>according to description of expected-downtime ("qmp-commands.hx:3246"):
> >>   "expected-downtime": only present while migration is active
> >>               total amount in ms for downtime that was calculated on
> >>               the last bitmap round (json-int)
> >>
> >>Previous version was just wrong because dirty_bytes_rate and bandwidth
> >>are measured in Bytes/ms, so dividing first by second we get some
> >>dimensionless quantity.
> >>As it said in description above this value is showed during active
> >>migration phase and recalculated only after transferring all memory
> >>and if this process took more than 1 sec. So maybe just nobody noticed
> >>that bug.
> >
> >While I agree the existing code looks wrong, I don't see how this is
> >any more correct.
> 
> This patch is aimed to fix units of expected_downtime. It is reasonable that
> expected_downtime should be measured in milliseconds. While the existing
> implementation lacks of any units.

I agree your units are correct where the old one isn't; and I agree
it needs fixing.
However I'm worrying about whether the value in your fix is correct.

> >  I think 'pending_size' is an estimate of the number of bytes left
> >to transfer, the intention being that most of those are transferred
> >prior to pausing the machine, if those are transferred before pausing
> >then they aren't part of the downtime.
> >
> Yes, 'pending_size' is an estimate of the number of bytes left to transfer,
> indeed.
> But the condition:
> >     if (s->dirty_bytes_rate && transferred_bytes > 10000) {
> slightly modifies the meaning of pending_size correspondingly.
> dirty_bytes_rate is set in migration_sync() that is called when pending_size
> < max_downtime * bandwidth. This estimation is higher than max_downtime by
> design

I don't think that check really modifies the meaning of pending_size;
it's just a sanity check that means we don't start trying to predict downtime
when we've not transmitted much yet.

> >It feels that:
> >    * If the guest wasn't dirtying pages, then you wouldn't have to
> >      pause the guest; if it was just dirtying them a little then you
> >      wouldn't have much to transfer after the pages you'd already
> >      sent; so if the guest dirty pages fast then the estimate should be
> >      larger; so 'dirty_bytes_rate' being on top of the fraction feels right.
> >
> >    * If the bandwidth is higher then the estimate should be smaller; so
> >      'bandwidth' being on the bottom of the fraction feels right.
> >
> >Dave
> >
> The 'expected_downtime' in the existing code takes two types of values:
>   * positive - dirty_bytes_rate is higher than bandwidth. In this
>     case migration doesn't complete.
>   * zero - bandwidth is higher than dirty_bytes_rate. In this case
>     migration is possible, but we don’t have the downtime value.

OK, I don't think that should give zero; my argument being that given
each pass throuhg RAM takes some time, you're always going to some proportion
of RAM that's dirty.

> This patch has some imperfections. But if we would look back into history,
> it seems that this patch just restores the broken logic.
> The existing code is introduced by commit
> https://github.com/qemu/qemu/commit/90f8ae724a575861f093fbdbfd49a925bcfec327
> which claims, that it just restores the mistakenly deleted estimation
> (commit
> https://github.com/qemu/qemu/commit/e4ed1541ac9413eac494a03532e34beaf8a7d1c5)
> Meanwhile, the estimation has changed during this restore operation. The
> estimation before the removal (before
> e4ed1541ac9413eac494a03532e34beaf8a7d1c5) was just like the one in my patch.

Yes; remember I believe that the current code is wrong - I'm just not
sure your suggested fix is right.

> So maybe we should think about improvement of this estimation.
> I'm suggest using something like:
>     expected_downtime = migrate_max_downtime * dirty_bytes_rate / bandwidth
> 
> In my opinion this is more correct than the existing approach since the last
> step of the migration process (before pause) is transferring of max_size
> bytes (max_size =  bandwidth * migrate_max_downtime() / 1000000). So the
> bytes that were dirtied at this step will be transferred during downtime.
> The transferred bytes count is dirty_bytes_rate * max_size/bandwidth or
> migrate_max_downtime * dirty_bytes_rate and division by the bandwidth
> results in a formula:
>     expected_downtime = migrate_max_downtime * dirty_bytes_rate / bandwidth

Are you sure - I thought migrate_max_downtime is governing the time *after* 
pause;
and it doesn't make sense to me for the expectation of the system to be
related to the expectation of the user.

Dave

> Igor
> 
> >>Signed-off-by: Igor Redko <address@hidden>
> >>Reviewed-by: Anna Melekhova <address@hidden>
> >>Signed-off-by: Denis V. Lunev <address@hidden>
> >>CC: Juan Quintela <address@hidden>
> >>CC: Amit Shah <address@hidden>
> >>---
> >>  migration/migration.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/migration/migration.c b/migration/migration.c
> >>index 662e77e..d55d545 100644
> >>--- a/migration/migration.c
> >>+++ b/migration/migration.c
> >>@@ -994,7 +994,7 @@ static void *migration_thread(void *opaque)
> >>              /* if we haven't sent anything, we don't want to recalculate
> >>                 10000 is a small enough number for our purposes */
> >>              if (s->dirty_bytes_rate && transferred_bytes > 10000) {
> >>-                s->expected_downtime = s->dirty_bytes_rate / bandwidth;
> >>+                s->expected_downtime = pending_size / bandwidth;
> >>              }
> >>
> >>              qemu_file_reset_rate_limit(s->file);
> >>--
> >>2.1.4
> >>
> >>
> >--
> >Dr. David Alan Gilbert / address@hidden / Manchester, UK
> >
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK