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

[Zhang Chen <zhangchen.fnst@cn.fujitsu.com>]

On 07/14/2017 08:10 PM, Dr. David Alan Gilbert wrote:
> * Zhang Chen (zhangchen.fnst@cn.fujitsu.com) 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 <zhangchen.fnst@cn.fujitsu.com>
>> ---
>>   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.

We need some time to do the checkpoint, and in this period we can ignore
the miscompare to get better performance. Like that:

currently:

     miscompare!
          notify checkpoint
     miscompare!
          notify checkpoint
     miscompare!
          notify checkpoint
     miscompare!
          notify checkpoint
     vm_stop and do checkpoint

     vm_start and finish checkpoint

     vm_stop and do checkpoint

     vm_start and finish checkpoint

     vm_stop and do checkpoint

     vm_start and finish checkpoint

     vm_stop and do checkpoint

     vm_start and finish checkpoint


running normally.


after:

     miscompare!
          notify checkpoint
     miscompare!
          ignore
     miscompare!
          ignore
     miscompare!
          ignore
     vm_stop and do checkpoint

     vm_start and finish checkpoint

running normally.



Thanks
Zhang Chen
  


>
> Dave
>
>>               break;
>
>>           }
>>       }
>> -- 
>> 2.7.4
>>
>>
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
> .
>

-- 
Thanks
Zhang Chen