RE: [PATCH 3/3] migration/colo: Merge multi checkpoint request into one.

[Zhanghailiang]

> -----Original Message-----
> From: Zhang Chen [mailto:chen.zhang@intel.com]
> Sent: Friday, May 15, 2020 12:28 PM
> To: Dr . David Alan Gilbert <dgilbert@redhat.com>; Juan Quintela
> <quintela@redhat.com>; Zhanghailiang <zhang.zhanghailiang@huawei.com>;
> qemu-dev <qemu-devel@nongnu.org>
> Cc: Zhang Chen <zhangckid@gmail.com>; Jason Wang
> <jasowang@redhat.com>; Zhang Chen <chen.zhang@intel.com>
> Subject: [PATCH 3/3] migration/colo: Merge multi checkpoint request into
> one.
> 
> From: Zhang Chen <chen.zhang@intel.com>
> 
> When COLO guest occur issues, COLO-compare will catch lots of different
> network packet and trigger notification multi times, force periodic may
> happen at the same time. So this can be efficient merge checkpoint request
> within COLO_CHECKPOINT_INTERVAL.
> 
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  migration/colo.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c index
> d5bced22cb..e6a7d8c6e2 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -47,6 +47,9 @@ static COLOMode last_colo_mode;
> 
>  #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
> 
> +/* Default COLO_CHECKPOINT_INTERVAL is 1000 ms */ #define
> +COLO_CHECKPOINT_INTERVAL 1000
> +
>  bool migration_in_colo_state(void)
>  {
>      MigrationState *s = migrate_get_current(); @@ -651,13 +654,20 @@
> out:
>  void colo_checkpoint_notify(void *opaque)  {
>      MigrationState *s = opaque;
> -    int64_t next_notify_time;
> +    int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> 
> -    qemu_sem_post(&s->colo_checkpoint_sem);
> -    s->colo_checkpoint_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> -    next_notify_time = s->colo_checkpoint_time +
> -                    s->parameters.x_checkpoint_delay;
> -    timer_mod(s->colo_delay_timer, next_notify_time);
> +    /*
> +     * When COLO guest occur issues, COLO-compare will catch lots of
> +     * different network packet and trigger notification multi times,
> +     * force periodic may happen at the same time. So this can be
> +     * efficient merge checkpoint request within
> COLO_CHECKPOINT_INTERVAL.
> +     */
> +    if (now > s->colo_checkpoint_time + COLO_CHECKPOINT_INTERVAL) {
> +        qemu_sem_post(&s->colo_checkpoint_sem);

It is not right here, this notification should not be controlled by the 
interval time,
I got what happened here, when multiple checkpoint requires come, this
Colo_delay_time will be added every time and it will be a big value which is 
not what we want.

Besides, please update this patch based on [PATCH 0/6] colo: migration related 
bugfixes series which
Has modified the same place.



> +        timer_mod(s->colo_delay_timer, now +
> +                  s->parameters.x_checkpoint_delay);
> +        s->colo_checkpoint_time = now;
> +    }
>  }
> 
>  void migrate_start_colo_process(MigrationState *s)
> --
> 2.17.1