Re: [PATCH v2 3/5] migration: Add per vmstate downtime tracepoints

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> We have a bunch of savevm_section* tracepoints, they're good to analyze
> migration stream, but not always suitable if someone would like to analyze
> the migration downtime.  Two major problems:
>
>   - savevm_section* tracepoints are dumping all sections, we only care
>     about the sections that contribute to the downtime
>
>   - They don't have an identifier to show the type of sections, so no way
>     to filter downtime information either easily.
>
> We can add type into the tracepoints, but instead of doing so, this patch
> kept them untouched, instead of adding a bunch of downtime specific
> tracepoints, so one can enable "vmstate_downtime*" tracepoints and get a
> full picture of how the downtime is distributed across iterative and
> non-iterative vmstate save/load.
>
> Note that here both save() and load() need to be traced, because both of
> them may contribute to the downtime.  The contribution is not a simple "add
> them together", though: consider when the src is doing a save() of device1
> while the dest can be load()ing for device2, so they can happen
> concurrently.
>
> Tracking both sides make sense because device load() and save() can be
> imbalanced, one device can save() super fast, but load() super slow, vice
> versa.  We can't figure that out without tracing both.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

queued.

>  static
>  int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool 
> in_postcopy)
>  {
> +    int64_t start_ts_each, end_ts_each;
>      SaveStateEntry *se;
>      int ret;
>  
> @@ -1475,6 +1476,8 @@ int 
> qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy)
>                  continue;
>              }
>          }
> +
> +        start_ts_each = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
I still think that:
           int64_t start_ts_each = qemu_clock_get_us(QEMU_CLOCK_REALTIME);

>          trace_savevm_section_start(se->idstr, se->section_id);
>  
>          save_section_header(f, se, QEMU_VM_SECTION_END);
> @@ -1486,6 +1489,9 @@ int 
> qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy)
>              qemu_file_set_error(f, ret);
>              return -1;
>          }
> +        end_ts_each = qemu_clock_get_us(QEMU_CLOCK_REALTIME);

and

           int64_t end_ts_each = qemu_clock_get_us(QEMU_CLOCK_REALTIME);

is clearer.

Having to pass the type thing is not "pleasant", but I can't think of a
better way to do it.

Later, Juan.