Re: [Qemu-devel] [PATCH V2 1/4] net/colo-compare.c: Add checkpoint min period to optimize performance

[Dr. David Alan Gilbert]

* Zhang Chen (address@hidden) wrote:
> If colo-compare find out the first different packet that means
> the following packet almost is different. we needn't do a lot
> of checkpoint in this time, so we set the no-need-checkpoint
> peroid, default just set 3 second.
> 
> Signed-off-by: Zhang Chen <address@hidden>
> ---
>  net/colo-compare.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 6d500e1..0f8e198 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -40,6 +40,9 @@
>  /* TODO: Should be configurable */
>  #define REGULAR_PACKET_CHECK_MS 3000
>  
> +/* TODO: Should be configurable */

Yes it should!

> +#define CHECKPOINT_MIN_TIME 3000
> +
>  /*
>    + CompareState ++
>    |               |
> @@ -455,6 +458,7 @@ static void colo_compare_connection(void *opaque, void 
> *user_data)
>      Packet *pkt = NULL;
>      GList *result = NULL;
>      int ret;
> +    static int64_t checkpoint_time_ms;
>  
>      while (!g_queue_is_empty(&conn->primary_list) &&
>             !g_queue_is_empty(&conn->secondary_list)) {
> @@ -494,7 +498,14 @@ static void colo_compare_connection(void *opaque, void 
> *user_data)
>               */
>              trace_colo_compare_main("packet different");
>              g_queue_push_tail(&conn->primary_list, pkt);
> -            /* TODO: colo_notify_checkpoint();*/
> +
> +            if (pkt->creation_ms - checkpoint_time_ms > CHECKPOINT_MIN_TIME) 
> {
> +                /*
> +                 * TODO: Notify colo frame to do checkpoint.
> +                 * colo_compare_inconsistent_notify();
> +                 */
> +                checkpoint_time_ms = pkt->creation_ms;
> +            }

You need to be careful how this interacts with the actual start of the
checkpoint.   Lets say you have two miscompared packets close to each
other:


    miscompare!
         checkpoint
    miscompare!
         ignore it because it was close to the 1st one

   That means we never trigger the 2nd checkpoint and it'll carry on
until the maximum checkpoint length.

   But also, I think you need to consider what happens to future packets
being compared; you can't release any packets now until the checkpoint
as soon as you know there's a miscompare.

Dave

>              break;


>          }
>      }
> -- 
> 2.7.4
> 
> 
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK