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

[Balamuruhan S]

On Thu, Jan 24, 2019 at 03:18:22PM +0800, Peter Xu wrote:
> On Wed, Jan 23, 2019 at 05:35:03PM +0100, Juan Quintela wrote:
> > address@hidden wrote:
> > > From: Balamuruhan S <address@hidden>
> > >
> > > 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 <address@hidden>
> > > Suggested-by: Dr. David Alan Gilbert <address@hidden>
> > > Signed-off-by: Balamuruhan S <address@hidden>
> > > ---
> > >  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).
> 
> Actually I have had a question on how expected_downtime is defined and
> how it will be used by users.
> 
> My understanding is that the expected_downtime is defined as: how long
> time the guest will be down if we stop the VM right now and migrate
> all the rest of pages.
> 
> This definition makes sense in that it helps the customer to
> dynamically decide whether it's a good point to go into the last phase
> of migration.  Currently we should be able to achieve that by setting
> a very high target downtime.
> 
> And if that definition is the thing we want, the current calculation
> seems exactly the number we want, since if we stop the VM right now
> then there won't be any more data to be dirtied as well.

Thank you Peter, I thought about your definition and it makes sense
with your definition that existing calculation is appropriate and
correct.

-- Bala
> 
> Regards,
> 
> -- 
> Peter Xu
>