Re: [Qemu-devel] [PATCH 09/11] migration: cleanup stats update into function

[Peter Xu]

On Wed, Jan 03, 2018 at 11:08:49AM +0100, Juan Quintela wrote:
> Peter Xu <address@hidden> wrote:
> > We have quite a few lines in migration_thread() that calculates some
> > statistics for the migration interations.  Isolate it into a single
> > function to improve readability.
> >
> > Signed-off-by: Peter Xu <address@hidden>
> 
> 
> 
> > +static void migration_update_statistics(MigrationState *s,
> 
> 
> migration_update_counters()?

Sure, or...

> 
> statistics for me mean that they are only used for informative
> purposes.  Here we *act* on that values.
> 
> 
> >  
> > -            qemu_file_reset_rate_limit(s->to_dst_file);
> > -            initial_time = current_time;
> > -            initial_bytes = qemu_ftell(s->to_dst_file);
> > -        }
> > +        /* Conditionally update statistics */
> 
> No need for the comment.  If we think it is needed just rename the
> function to:
>    conditionally_update_statistics()?
> 
> I still preffer the:
>    migration_update_counters.

... migration_update_counters_conditionally()?

> 
> 
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 3ab5506233..248f7d9a5c 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -90,6 +90,19 @@ struct MigrationState
> >      QEMUBH *cleanup_bh;
> >      QEMUFile *to_dst_file;
> >  
> > +    /*
> > +     * Migration thread statistic variables, mostly used in
> > +     * migration_thread() iterations only.
> > +     */
> > +    uint64_t initial_bytes;
> 
>        /* bytes already send at the beggining of current interation */
>        uint64_t iteration_initial_bytes;
> 
> > +    int64_t initial_time;
>        /* time at the start of current iteration */
>        int64_t iteration_start_time;
> 
> What do you think?

Will change both.

Thanks,

-- 
Peter Xu