| Message ID | 1499925175-21218-2-git-send-email-zhangchen.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017年07月13日 13:52, Zhang Chen 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 */ > +#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; Let's avoid static variable here since we support more than one compare instance. Thanks > > 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; > + } > break; > } > }
* 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. Dave > break; > } > } > -- > 2.7.4 > > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 07/14/2017 11:22 AM, Jason Wang wrote: > > > On 2017年07月13日 13:52, Zhang Chen 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 */ >> +#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; > > Let's avoid static variable here since we support more than one > compare instance. OK, I will add the "checkpoint_time_ms" to CompareState. Thanks Zhang Chen > > Thanks > >> 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; >> + } >> break; >> } >> } > > > > . >
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 > > > . >
* Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote: > > > 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. Yes, but you must make sure that you don't ignore any miscompares after the start of the next checkpoint - I don't see how you avoid that. Also we must be careful about packets released after the 1st miscompare. Dave > > > Thanks > Zhang Chen > > > > > > Dave > > > > > break; > > > > > } > > > } > > > -- > > > 2.7.4 > > > > > > > > > > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > > > > . > > > > -- > Thanks > Zhang Chen > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 07/17/2017 08:24 PM, Dr. David Alan Gilbert wrote: > * Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote: >> >> 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. > Yes, but you must make sure that you don't > ignore any miscompares after the start of the next checkpoint - I don't > see how you avoid that. Good catch, I will fix it in next version. > > Also we must be careful about packets released after the 1st miscompare. Yes, after the 1st miscompare, all ignored packet will be enqueued. Then, we will flush all packet in the queue during do checkpoint. Thanks Zhang Chen > > Dave > >> >> Thanks >> Zhang Chen >> >> >>> Dave >>> >>>> break; >>>> } >>>> } >>>> -- >>>> 2.7.4 >>>> >>>> >>>> >>>> >>> -- >>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >>> >>> >>> . >>> >> -- >> Thanks >> Zhang Chen >> >> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > . >
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 */ +#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; + } break; } }
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(-)