Re: [Qemu-devel] [PATCH 2/5] migration: rename max_size to threshold_size

[Peter Xu]

On Fri, Mar 31, 2017 at 07:59:19PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (address@hidden) wrote:
> > In migration codes (especially in migration_thread()), max_size is used
> > in many place for the threshold value that we will start to do the final
> > flush and jump to the next stage to dump the whole rest things to
> > destination. However its name is confusing to first readers. Let's
> > rename it to "threshold_size" when proper and add a comment for it. No
> > functional change is made.
> > 
> > CC: Juan Quintela <address@hidden>
> > CC: "Dr. David Alan Gilbert" <address@hidden>
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  include/migration/vmstate.h |  3 ++-
> >  migration/migration.c       | 17 +++++++++--------
> >  migration/savevm.c          |  4 ++--
> >  3 files changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index f2dbf84..dad3984 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -56,7 +56,8 @@ typedef struct SaveVMHandlers {
> >  
> >      /* This runs outside the iothread lock!  */
> >      int (*save_live_setup)(QEMUFile *f, void *opaque);
> > -    void (*save_live_pending)(QEMUFile *f, void *opaque, uint64_t max_size,
> > +    void (*save_live_pending)(QEMUFile *f, void *opaque,
> > +                              uint64_t threshold_size,
> >                                uint64_t *non_postcopiable_pending,
> >                                uint64_t *postcopiable_pending);
> >      LoadStateHandler *load_state;
> > diff --git a/migration/migration.c b/migration/migration.c
> > index f9f4d98..b065fe4 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1907,7 +1907,8 @@ static void *migration_thread(void *opaque)
> >      int64_t initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> >      int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> >      int64_t initial_bytes = 0;
> > -    int64_t max_size = 0;
> > +    /* We'll do the final flush when reachs threshold_size */
> 
> I think that's 'reaches' - however perhaps we should make a more
> explicit comment:
>    'The final stage happens when the remaining data is smaller than
>     this threshold; it's calculated from the requested downtime
>     and measured bandwidth'

Yes it looks better. Will "steal" that. :)

> 
> other than that:
> Reviewed-by: Dr. David Alan Gilbert <address@hidden>

Thanks!

-- peterx