| Message ID | 20200911190508.1316487-3-dereksu@qnap.com |
|---|---|
| State | New |
| Headers | show |
| Series | colo-compare bugfixes | expand |
> -----Original Message----- > From: Derek Su <dereksu@qnap.com> > Sent: Saturday, September 12, 2020 3:05 AM > To: qemu-devel@nongnu.org > Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; > jasowang@redhat.com; Derek Su <dereksu@qnap.com> > Subject: [PATCH v1 2/2] colo-compare: Record packet creation time by > QEMU_CLOCK_REALTIME > > Record packet creation time by QEMU_CLOCK_REALTIME instead of > QEMU_CLOCK_HOST. The time difference between `now` and packet > `creation_ms` has the possibility of an unexpected negative value and results > in wrong comparison after changing the host clock. > Hi Derek, I think this issue caused by other code in this file use the qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); I will change all code to QEMU_CLOCK_HOST to fix it with the patch 1/2. Thanks Zhang Chen > Signed-off-by: Derek Su <dereksu@qnap.com> > --- > net/colo-compare.c | 2 +- > net/colo.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > c4de86ef34..29d7f986e3 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -621,7 +621,7 @@ static int colo_packet_compare_other(Packet *spkt, > Packet *ppkt) > > static int colo_old_packet_check_one(Packet *pkt, void *user_data) { > - int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST); > + int64_t now = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > uint32_t check_time = *(uint32_t *)user_data; > > if ((now - pkt->creation_ms) > check_time) { diff --git a/net/colo.c > b/net/colo.c index a6c66d829a..0441910169 100644 > --- a/net/colo.c > +++ b/net/colo.c > @@ -164,7 +164,7 @@ Packet *packet_new(const void *data, int size, int > vnet_hdr_len) > > pkt->data = g_memdup(data, size); > pkt->size = size; > - pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_HOST); > + pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > pkt->vnet_hdr_len = vnet_hdr_len; > pkt->tcp_seq = 0; > pkt->tcp_ack = 0; > -- > 2.25.1
> -----Original Message----- > From: Zhang, Chen > Sent: Monday, September 14, 2020 4:02 AM > To: 'Derek Su' <dereksu@qnap.com>; qemu-devel@nongnu.org > Cc: lizhijian@cn.fujitsu.com; jasowang@redhat.com > Subject: RE: [PATCH v1 2/2] colo-compare: Record packet creation time by > QEMU_CLOCK_REALTIME > > > > > -----Original Message----- > > From: Derek Su <dereksu@qnap.com> > > Sent: Saturday, September 12, 2020 3:05 AM > > To: qemu-devel@nongnu.org > > Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; > > jasowang@redhat.com; Derek Su <dereksu@qnap.com> > > Subject: [PATCH v1 2/2] colo-compare: Record packet creation time by > > QEMU_CLOCK_REALTIME > > > > Record packet creation time by QEMU_CLOCK_REALTIME instead of > > QEMU_CLOCK_HOST. The time difference between `now` and packet > > `creation_ms` has the possibility of an unexpected negative value and > > results in wrong comparison after changing the host clock. > > > > Hi Derek, > > I think this issue caused by other code in this file use the > qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); > I will change all code to QEMU_CLOCK_HOST to fix it with the patch 1/2. If you feel OK, or you can send the new version. :-) > > Thanks > Zhang Chen > > > Signed-off-by: Derek Su <dereksu@qnap.com> > > --- > > net/colo-compare.c | 2 +- > > net/colo.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > c4de86ef34..29d7f986e3 100644 > > --- a/net/colo-compare.c > > +++ b/net/colo-compare.c > > @@ -621,7 +621,7 @@ static int colo_packet_compare_other(Packet *spkt, > > Packet *ppkt) > > > > static int colo_old_packet_check_one(Packet *pkt, void *user_data) { > > - int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST); > > + int64_t now = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > > uint32_t check_time = *(uint32_t *)user_data; > > > > if ((now - pkt->creation_ms) > check_time) { diff --git > > a/net/colo.c b/net/colo.c index a6c66d829a..0441910169 100644 > > --- a/net/colo.c > > +++ b/net/colo.c > > @@ -164,7 +164,7 @@ Packet *packet_new(const void *data, int size, int > > vnet_hdr_len) > > > > pkt->data = g_memdup(data, size); > > pkt->size = size; > > - pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_HOST); > > + pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > > pkt->vnet_hdr_len = vnet_hdr_len; > > pkt->tcp_seq = 0; > > pkt->tcp_ack = 0; > > -- > > 2.25.1
Zhang, Chen <chen.zhang@intel.com>於 2020年9月14日 週一，上午4:06寫道： > > > > > > -----Original Message----- > > > From: Zhang, Chen > > > Sent: Monday, September 14, 2020 4:02 AM > > > To: 'Derek Su' <dereksu@qnap.com>; qemu-devel@nongnu.org > > > Cc: lizhijian@cn.fujitsu.com; jasowang@redhat.com > > > Subject: RE: [PATCH v1 2/2] colo-compare: Record packet creation time by > > > QEMU_CLOCK_REALTIME > > > > > > > > > > > > > -----Original Message----- > > > > From: Derek Su <dereksu@qnap.com> > > > > Sent: Saturday, September 12, 2020 3:05 AM > > > > To: qemu-devel@nongnu.org > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; > > > > jasowang@redhat.com; Derek Su <dereksu@qnap.com> > > > > Subject: [PATCH v1 2/2] colo-compare: Record packet creation time by > > > > QEMU_CLOCK_REALTIME > > > > > > > > Record packet creation time by QEMU_CLOCK_REALTIME instead of > > > > QEMU_CLOCK_HOST. The time difference between `now` and packet > > > > `creation_ms` has the possibility of an unexpected negative value and > > > > results in wrong comparison after changing the host clock. > > > > > > > > > > Hi Derek, > > > > > > I think this issue caused by other code in this file use the > > > qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); > > > I will change all code to QEMU_CLOCK_HOST to fix it with the patch 1/2. > > > > If you feel OK, or you can send the new version. :-) > > Hello, Chen Is the monotonically increasing clock better than host clock in the COLO compare framework? The host clock is sometimes changed by the users manually or NTP synchronization automatically, and the cases may lead to the relative time be disordered. For example, the `creation_time` of one packet is advanced to the `now` in our tests. I incline to replace QEMU_CLOCK_REALTIME and QEMU_CLOCK_VIRTUAL with the monotonically increasing clock QEMU_CLOCK_REALTIME in COLO compare framework. How about your thoughts? If OK, I will send the new version again, thanks. :) Thank you. Regards, Derek > > > > > > Thanks > > > Zhang Chen > > > > > > > Signed-off-by: Derek Su <dereksu@qnap.com> > > > > --- > > > > net/colo-compare.c | 2 +- > > > > net/colo.c | 2 +- > > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > > > c4de86ef34..29d7f986e3 100644 > > > > --- a/net/colo-compare.c > > > > +++ b/net/colo-compare.c > > > > @@ -621,7 +621,7 @@ static int colo_packet_compare_other(Packet *spkt, > > > > Packet *ppkt) > > > > > > > > static int colo_old_packet_check_one(Packet *pkt, void *user_data) { > > > > - int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST); > > > > + int64_t now = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > > > > uint32_t check_time = *(uint32_t *)user_data; > > > > > > > > if ((now - pkt->creation_ms) > check_time) { diff --git > > > > a/net/colo.c b/net/colo.c index a6c66d829a..0441910169 100644 > > > > --- a/net/colo.c > > > > +++ b/net/colo.c > > > > @@ -164,7 +164,7 @@ Packet *packet_new(const void *data, int size, int > > > > vnet_hdr_len) > > > > > > > > pkt->data = g_memdup(data, size); > > > > pkt->size = size; > > > > - pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_HOST); > > > > + pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > > > > pkt->vnet_hdr_len = vnet_hdr_len; > > > > pkt->tcp_seq = 0; > > > > pkt->tcp_ack = 0; > > > > -- > > > > 2.25.1 > > > >
From: Derek Su <dereksu@qnap.com> Sent: Monday, September 14, 2020 9:00 AM To: Zhang, Chen <chen.zhang@intel.com> Cc: jasowang@redhat.com; lizhijian@cn.fujitsu.com; qemu-devel@nongnu.org Subject: Re: [PATCH v1 2/2] colo-compare: Record packet creation time by QEMU_CLOCK_REALTIME Zhang, Chen <chen.zhang@intel.com<mailto:chen.zhang@intel.com>>於 2020年9月14日 週一，上午4:06寫道： > -----Original Message----- > From: Zhang, Chen > Sent: Monday, September 14, 2020 4:02 AM > To: 'Derek Su' <dereksu@qnap.com<mailto:dereksu@qnap.com>>; qemu-devel@nongnu.org<mailto:qemu-devel@nongnu.org> > Cc: lizhijian@cn.fujitsu.com<mailto:lizhijian@cn.fujitsu.com>; jasowang@redhat.com<mailto:jasowang@redhat.com> > Subject: RE: [PATCH v1 2/2] colo-compare: Record packet creation time by > QEMU_CLOCK_REALTIME > > > > > -----Original Message----- > > From: Derek Su <dereksu@qnap.com<mailto:dereksu@qnap.com>> > > Sent: Saturday, September 12, 2020 3:05 AM > > To: qemu-devel@nongnu.org<mailto:qemu-devel@nongnu.org> > > Cc: Zhang, Chen <chen.zhang@intel.com<mailto:chen.zhang@intel.com>>; lizhijian@cn.fujitsu.com<mailto:lizhijian@cn.fujitsu.com>; > > jasowang@redhat.com<mailto:jasowang@redhat.com>; Derek Su <dereksu@qnap.com<mailto:dereksu@qnap.com>> > > Subject: [PATCH v1 2/2] colo-compare: Record packet creation time by > > QEMU_CLOCK_REALTIME > > > > Record packet creation time by QEMU_CLOCK_REALTIME instead of > > QEMU_CLOCK_HOST. The time difference between `now` and packet > > `creation_ms` has the possibility of an unexpected negative value and > > results in wrong comparison after changing the host clock. > > > > Hi Derek, > > I think this issue caused by other code in this file use the > qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); > I will change all code to QEMU_CLOCK_HOST to fix it with the patch 1/2. If you feel OK, or you can send the new version. :-) Hello, Chen Is the monotonically increasing clock better than host clock in the COLO compare framework? The host clock is sometimes changed by the users manually or NTP synchronization automatically, and the cases may lead to the relative time be disordered. For example, the `creation_time` of one packet is advanced to the `now` in our tests. I incline to replace QEMU_CLOCK_REALTIME and QEMU_CLOCK_VIRTUAL with the monotonically increasing clock QEMU_CLOCK_REALTIME in COLO compare framework. How about your thoughts? Hi Derek, /** * QEMUClockType: * * The following clock types are available: * * @QEMU_CLOCK_REALTIME: Real time clock * * The real time clock should be used only for stuff which does not * change the virtual machine state, as it runs even if the virtual * machine is stopped. * * @QEMU_CLOCK_VIRTUAL: virtual clock * * The virtual clock only runs during the emulation. It stops * when the virtual machine is stopped. * * @QEMU_CLOCK_HOST: host clock * * The host clock should be used for device models that emulate accurate * real time sources. It will continue to run when the virtual machine * is suspended, and it will reflect system time changes the host may * undergo (e.g. due to NTP). When COLO running, it will change the virtual machine state. So I think use the QEMU_CLOCK_HOST is better. Thanks Zhang Chen If OK, I will send the new version again, thanks. :) Thank you. Regards, Derek > > Thanks > Zhang Chen > > > Signed-off-by: Derek Su <dereksu@qnap.com<mailto:dereksu@qnap.com>> > > --- > > net/colo-compare.c | 2 +- > > net/colo.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > c4de86ef34..29d7f986e3 100644 > > --- a/net/colo-compare.c > > +++ b/net/colo-compare.c > > @@ -621,7 +621,7 @@ static int colo_packet_compare_other(Packet *spkt, > > Packet *ppkt) > > > > static int colo_old_packet_check_one(Packet *pkt, void *user_data) { > > - int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST); > > + int64_t now = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > > uint32_t check_time = *(uint32_t *)user_data; > > > > if ((now - pkt->creation_ms) > check_time) { diff --git > > a/net/colo.c b/net/colo.c index a6c66d829a..0441910169 100644 > > --- a/net/colo.c > > +++ b/net/colo.c > > @@ -164,7 +164,7 @@ Packet *packet_new(const void *data, int size, int > > vnet_hdr_len) > > > > pkt->data = g_memdup(data, size); > > pkt->size = size; > > - pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_HOST); > > + pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > > pkt->vnet_hdr_len = vnet_hdr_len; > > pkt->tcp_seq = 0; > > pkt->tcp_ack = 0; > > -- > > 2.25.1
Hello, Chen My humble opinion is that the `creation_ms` and `now` fixed in my patch should be OK in colo-compare and performs well in my test if used QEMU_CLOCK_REALTIME. Though the vm state is changed in COLO scenario, the record and comparison of `creation_ms` and `now` are not affected by these vm changes. In net/colo.c and net/colo-compare.c functions using timer_mod(), using QEMU_CLOCK_HOST is dangerous if users change the host clock. The timer might not be fired on time as expected. The original time_mod using QEMU_CLOCK_VIRTUAL seems OK currently. Thanks. Regards, Derek Zhang, Chen <chen.zhang@intel.com> 於 2020年9月14日 週一 下午3:42寫道： > > > > > *From:* Derek Su <dereksu@qnap.com> > *Sent:* Monday, September 14, 2020 9:00 AM > *To:* Zhang, Chen <chen.zhang@intel.com> > *Cc:* jasowang@redhat.com; lizhijian@cn.fujitsu.com; qemu-devel@nongnu.org > *Subject:* Re: [PATCH v1 2/2] colo-compare: Record packet creation time > by QEMU_CLOCK_REALTIME > > > > > > > > Zhang, Chen <chen.zhang@intel.com>於 2020年9月14日 週一，上午4:06寫道： > > > > > > > -----Original Message----- > > > From: Zhang, Chen > > > Sent: Monday, September 14, 2020 4:02 AM > > > To: 'Derek Su' <dereksu@qnap.com>; qemu-devel@nongnu.org > > > Cc: lizhijian@cn.fujitsu.com; jasowang@redhat.com > > > Subject: RE: [PATCH v1 2/2] colo-compare: Record packet creation time by > > > QEMU_CLOCK_REALTIME > > > > > > > > > > > > > -----Original Message----- > > > > From: Derek Su <dereksu@qnap.com> > > > > Sent: Saturday, September 12, 2020 3:05 AM > > > > To: qemu-devel@nongnu.org > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; > > > > jasowang@redhat.com; Derek Su <dereksu@qnap.com> > > > > Subject: [PATCH v1 2/2] colo-compare: Record packet creation time by > > > > QEMU_CLOCK_REALTIME > > > > > > > > Record packet creation time by QEMU_CLOCK_REALTIME instead of > > > > QEMU_CLOCK_HOST. The time difference between `now` and packet > > > > `creation_ms` has the possibility of an unexpected negative value and > > > > results in wrong comparison after changing the host clock. > > > > > > > > > > Hi Derek, > > > > > > I think this issue caused by other code in this file use the > > > qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); > > > I will change all code to QEMU_CLOCK_HOST to fix it with the patch 1/2. > > > > If you feel OK, or you can send the new version. :-) > > > > Hello, Chen > > > > Is the monotonically increasing clock better than host clock in the COLO > compare framework? > > The host clock is sometimes changed by the users manually or NTP > synchronization automatically, and the cases may lead to the relative time > be disordered. > > For example, the `creation_time` of one packet is advanced to the `now` in > our tests. > > > > I incline to replace QEMU_CLOCK_REALTIME and QEMU_CLOCK_VIRTUAL with > the monotonically increasing clock QEMU_CLOCK_REALTIME in COLO compare > framework. > > How about your thoughts? > > > > Hi Derek, > > > > /** > > * QEMUClockType: > > * > > * The following clock types are available: > > * > > * @QEMU_CLOCK_REALTIME: Real time clock > > * > > * The real time clock should be used only for stuff which does not > > * change the virtual machine state, as it runs even if the virtual > > * machine is stopped. > > * > > * @QEMU_CLOCK_VIRTUAL: virtual clock > > * > > * The virtual clock only runs during the emulation. It stops > > * when the virtual machine is stopped. > > * > > * @QEMU_CLOCK_HOST: host clock > > * > > * The host clock should be used for device models that emulate accurate > > * real time sources. It will continue to run when the virtual machine > > * is suspended, and it will reflect system time changes the host may > > * undergo (e.g. due to NTP). > > > > > > When COLO running, it will change the virtual machine state. > > So I think use the QEMU_CLOCK_HOST is better. > > > > Thanks > > Zhang Chen > > > > If OK, I will send the new version again, thanks. :) > > Thank you. > > > > Regards, > > Derek > > > > > > > > > > > Thanks > > > Zhang Chen > > > > > > > Signed-off-by: Derek Su <dereksu@qnap.com> > > > > --- > > > > net/colo-compare.c | 2 +- > > > > net/colo.c | 2 +- > > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > > > c4de86ef34..29d7f986e3 100644 > > > > --- a/net/colo-compare.c > > > > +++ b/net/colo-compare.c > > > > @@ -621,7 +621,7 @@ static int colo_packet_compare_other(Packet *spkt, > > > > Packet *ppkt) > > > > > > > > static int colo_old_packet_check_one(Packet *pkt, void *user_data) { > > > > - int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST); > > > > + int64_t now = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > > > > uint32_t check_time = *(uint32_t *)user_data; > > > > > > > > if ((now - pkt->creation_ms) > check_time) { diff --git > > > > a/net/colo.c b/net/colo.c index a6c66d829a..0441910169 100644 > > > > --- a/net/colo.c > > > > +++ b/net/colo.c > > > > @@ -164,7 +164,7 @@ Packet *packet_new(const void *data, int size, int > > > > vnet_hdr_len) > > > > > > > > pkt->data = g_memdup(data, size); > > > > pkt->size = size; > > > > - pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_HOST); > > > > + pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > > > > pkt->vnet_hdr_len = vnet_hdr_len; > > > > pkt->tcp_seq = 0; > > > > pkt->tcp_ack = 0; > > > > -- > > > > 2.25.1 > > >
Hi Derek, Looks qemu vl.c, migration/migration.c…etc use the “QEMU_CLOCK_HOST” too. It will also affected by host NTP. Do you means we should change all the QEMU_CLOCK_HOST to QEMU_CLOCK_REALTIME? And use the QEMU_CLOCK_VIRTUAL is not correct, because we need to know time changes during VM stop. Thanks Zhang Chen From: Derek Su <dereksu@qnap.com> Sent: Tuesday, September 15, 2020 12:24 AM To: Zhang, Chen <chen.zhang@intel.com> Cc: jasowang@redhat.com; lizhijian@cn.fujitsu.com; qemu-devel@nongnu.org; C.H. Yang <chyang@qnap.com>; CT Cheng <ctcheng@qnap.com> Subject: Re: [PATCH v1 2/2] colo-compare: Record packet creation time by QEMU_CLOCK_REALTIME Hello, Chen My humble opinion is that the `creation_ms` and `now` fixed in my patch should be OK in colo-compare and performs well in my test if used QEMU_CLOCK_REALTIME. Though the vm state is changed in COLO scenario, the record and comparison of `creation_ms` and `now` are not affected by these vm changes. In net/colo.c and net/colo-compare.c functions using timer_mod(), using QEMU_CLOCK_HOST is dangerous if users change the host clock. The timer might not be fired on time as expected. The original time_mod using QEMU_CLOCK_VIRTUAL seems OK currently. Thanks. Regards, Derek Zhang, Chen <chen.zhang@intel.com<mailto:chen.zhang@intel.com>> 於 2020年9月14日 週一 下午3:42寫道： From: Derek Su <dereksu@qnap.com<mailto:dereksu@qnap.com>> Sent: Monday, September 14, 2020 9:00 AM To: Zhang, Chen <chen.zhang@intel.com<mailto:chen.zhang@intel.com>> Cc: jasowang@redhat.com<mailto:jasowang@redhat.com>; lizhijian@cn.fujitsu.com<mailto:lizhijian@cn.fujitsu.com>; qemu-devel@nongnu.org<mailto:qemu-devel@nongnu.org> Subject: Re: [PATCH v1 2/2] colo-compare: Record packet creation time by QEMU_CLOCK_REALTIME Zhang, Chen <chen.zhang@intel.com<mailto:chen.zhang@intel.com>>於 2020年9月14日 週一，上午4:06寫道： > -----Original Message----- > From: Zhang, Chen > Sent: Monday, September 14, 2020 4:02 AM > To: 'Derek Su' <dereksu@qnap.com<mailto:dereksu@qnap.com>>; qemu-devel@nongnu.org<mailto:qemu-devel@nongnu.org> > Cc: lizhijian@cn.fujitsu.com<mailto:lizhijian@cn.fujitsu.com>; jasowang@redhat.com<mailto:jasowang@redhat.com> > Subject: RE: [PATCH v1 2/2] colo-compare: Record packet creation time by > QEMU_CLOCK_REALTIME > > > > > -----Original Message----- > > From: Derek Su <dereksu@qnap.com<mailto:dereksu@qnap.com>> > > Sent: Saturday, September 12, 2020 3:05 AM > > To: qemu-devel@nongnu.org<mailto:qemu-devel@nongnu.org> > > Cc: Zhang, Chen <chen.zhang@intel.com<mailto:chen.zhang@intel.com>>; lizhijian@cn.fujitsu.com<mailto:lizhijian@cn.fujitsu.com>; > > jasowang@redhat.com<mailto:jasowang@redhat.com>; Derek Su <dereksu@qnap.com<mailto:dereksu@qnap.com>> > > Subject: [PATCH v1 2/2] colo-compare: Record packet creation time by > > QEMU_CLOCK_REALTIME > > > > Record packet creation time by QEMU_CLOCK_REALTIME instead of > > QEMU_CLOCK_HOST. The time difference between `now` and packet > > `creation_ms` has the possibility of an unexpected negative value and > > results in wrong comparison after changing the host clock. > > > > Hi Derek, > > I think this issue caused by other code in this file use the > qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); > I will change all code to QEMU_CLOCK_HOST to fix it with the patch 1/2. If you feel OK, or you can send the new version. :-) Hello, Chen Is the monotonically increasing clock better than host clock in the COLO compare framework? The host clock is sometimes changed by the users manually or NTP synchronization automatically, and the cases may lead to the relative time be disordered. For example, the `creation_time` of one packet is advanced to the `now` in our tests. I incline to replace QEMU_CLOCK_REALTIME and QEMU_CLOCK_VIRTUAL with the monotonically increasing clock QEMU_CLOCK_REALTIME in COLO compare framework. How about your thoughts? Hi Derek, /** * QEMUClockType: * * The following clock types are available: * * @QEMU_CLOCK_REALTIME: Real time clock * * The real time clock should be used only for stuff which does not * change the virtual machine state, as it runs even if the virtual * machine is stopped. * * @QEMU_CLOCK_VIRTUAL: virtual clock * * The virtual clock only runs during the emulation. It stops * when the virtual machine is stopped. * * @QEMU_CLOCK_HOST: host clock * * The host clock should be used for device models that emulate accurate * real time sources. It will continue to run when the virtual machine * is suspended, and it will reflect system time changes the host may * undergo (e.g. due to NTP). When COLO running, it will change the virtual machine state. So I think use the QEMU_CLOCK_HOST is better. Thanks Zhang Chen If OK, I will send the new version again, thanks. :) Thank you. Regards, Derek > > Thanks > Zhang Chen > > > Signed-off-by: Derek Su <dereksu@qnap.com<mailto:dereksu@qnap.com>> > > --- > > net/colo-compare.c | 2 +- > > net/colo.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > c4de86ef34..29d7f986e3 100644 > > --- a/net/colo-compare.c > > +++ b/net/colo-compare.c > > @@ -621,7 +621,7 @@ static int colo_packet_compare_other(Packet *spkt, > > Packet *ppkt) > > > > static int colo_old_packet_check_one(Packet *pkt, void *user_data) { > > - int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST); > > + int64_t now = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > > uint32_t check_time = *(uint32_t *)user_data; > > > > if ((now - pkt->creation_ms) > check_time) { diff --git > > a/net/colo.c b/net/colo.c index a6c66d829a..0441910169 100644 > > --- a/net/colo.c > > +++ b/net/colo.c > > @@ -164,7 +164,7 @@ Packet *packet_new(const void *data, int size, int > > vnet_hdr_len) > > > > pkt->data = g_memdup(data, size); > > pkt->size = size; > > - pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_HOST); > > + pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > > pkt->vnet_hdr_len = vnet_hdr_len; > > pkt->tcp_seq = 0; > > pkt->tcp_ack = 0; > > -- > > 2.25.1
Hello, Chen Zhang, Chen <chen.zhang@intel.com> 於 2020年9月15日 週二 上午8:09寫道： > Hi Derek, > > > > Looks qemu vl.c, migration/migration.c…etc use the “QEMU_CLOCK_HOST” too. > It will also affected by host NTP. Do you means we should change all the > QEMU_CLOCK_HOST to QEMU_CLOCK_REALTIME? > No, I think it depends on the purpose. For example, the setup_time/end_time in migration/migraion.c are for statistical information. There is maybe a low probability of the issue I mentioned. It would be classified as a known issue in practical applications. QEMU_CLOCK_HOST seems enough as you said. > And use the QEMU_CLOCK_VIRTUAL is not correct, because we need to know > time changes during VM stop. > Thanks. Regards, Derek > Thanks > > Zhang Chen > > > > *From:* Derek Su <dereksu@qnap.com> > *Sent:* Tuesday, September 15, 2020 12:24 AM > *To:* Zhang, Chen <chen.zhang@intel.com> > *Cc:* jasowang@redhat.com; lizhijian@cn.fujitsu.com; qemu-devel@nongnu.org; > C.H. Yang <chyang@qnap.com>; CT Cheng <ctcheng@qnap.com> > *Subject:* Re: [PATCH v1 2/2] colo-compare: Record packet creation time > by QEMU_CLOCK_REALTIME > > > > > Hello, Chen > > > > My humble opinion is that the `creation_ms` and `now` fixed in my patch > should be OK in colo-compare and performs well in my test if used > QEMU_CLOCK_REALTIME. Though the vm state is changed in COLO scenario, the > record and comparison of `creation_ms` and `now` are not affected by these > vm changes. > > > > In net/colo.c and net/colo-compare.c functions using timer_mod(), > using QEMU_CLOCK_HOST is dangerous if users change the host clock. The > timer might not be fired on time as expected. The original time_mod using > QEMU_CLOCK_VIRTUAL seems OK currently. > > Thanks. > > > > Regards, > > Derek > > > > > > Zhang, Chen <chen.zhang@intel.com> 於 2020年9月14日 週一 下午3:42寫道： > > > > > > *From:* Derek Su <dereksu@qnap.com> > *Sent:* Monday, September 14, 2020 9:00 AM > *To:* Zhang, Chen <chen.zhang@intel.com> > *Cc:* jasowang@redhat.com; lizhijian@cn.fujitsu.com; qemu-devel@nongnu.org > *Subject:* Re: [PATCH v1 2/2] colo-compare: Record packet creation time > by QEMU_CLOCK_REALTIME > > > > > > > > Zhang, Chen <chen.zhang@intel.com>於 2020年9月14日 週一，上午4:06寫道： > > > > > > > -----Original Message----- > > > From: Zhang, Chen > > > Sent: Monday, September 14, 2020 4:02 AM > > > To: 'Derek Su' <dereksu@qnap.com>; qemu-devel@nongnu.org > > > Cc: lizhijian@cn.fujitsu.com; jasowang@redhat.com > > > Subject: RE: [PATCH v1 2/2] colo-compare: Record packet creation time by > > > QEMU_CLOCK_REALTIME > > > > > > > > > > > > > -----Original Message----- > > > > From: Derek Su <dereksu@qnap.com> > > > > Sent: Saturday, September 12, 2020 3:05 AM > > > > To: qemu-devel@nongnu.org > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; > > > > jasowang@redhat.com; Derek Su <dereksu@qnap.com> > > > > Subject: [PATCH v1 2/2] colo-compare: Record packet creation time by > > > > QEMU_CLOCK_REALTIME > > > > > > > > Record packet creation time by QEMU_CLOCK_REALTIME instead of > > > > QEMU_CLOCK_HOST. The time difference between `now` and packet > > > > `creation_ms` has the possibility of an unexpected negative value and > > > > results in wrong comparison after changing the host clock. > > > > > > > > > > Hi Derek, > > > > > > I think this issue caused by other code in this file use the > > > qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); > > > I will change all code to QEMU_CLOCK_HOST to fix it with the patch 1/2. > > > > If you feel OK, or you can send the new version. :-) > > > > Hello, Chen > > > > Is the monotonically increasing clock better than host clock in the COLO > compare framework? > > The host clock is sometimes changed by the users manually or NTP > synchronization automatically, and the cases may lead to the relative time > be disordered. > > For example, the `creation_time` of one packet is advanced to the `now` in > our tests. > > > > I incline to replace QEMU_CLOCK_REALTIME and QEMU_CLOCK_VIRTUAL with > the monotonically increasing clock QEMU_CLOCK_REALTIME in COLO compare > framework. > > How about your thoughts? > > > > Hi Derek, > > > > /** > > * QEMUClockType: > > * > > * The following clock types are available: > > * > > * @QEMU_CLOCK_REALTIME: Real time clock > > * > > * The real time clock should be used only for stuff which does not > > * change the virtual machine state, as it runs even if the virtual > > * machine is stopped. > > * > > * @QEMU_CLOCK_VIRTUAL: virtual clock > > * > > * The virtual clock only runs during the emulation. It stops > > * when the virtual machine is stopped. > > * > > * @QEMU_CLOCK_HOST: host clock > > * > > * The host clock should be used for device models that emulate accurate > > * real time sources. It will continue to run when the virtual machine > > * is suspended, and it will reflect system time changes the host may > > * undergo (e.g. due to NTP). > > > > > > When COLO running, it will change the virtual machine state. > > So I think use the QEMU_CLOCK_HOST is better. > > > > Thanks > > Zhang Chen > > > > If OK, I will send the new version again, thanks. :) > > Thank you. > > > > Regards, > > Derek > > > > > > > > > > > Thanks > > > Zhang Chen > > > > > > > Signed-off-by: Derek Su <dereksu@qnap.com> > > > > --- > > > > net/colo-compare.c | 2 +- > > > > net/colo.c | 2 +- > > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > > > c4de86ef34..29d7f986e3 100644 > > > > --- a/net/colo-compare.c > > > > +++ b/net/colo-compare.c > > > > @@ -621,7 +621,7 @@ static int colo_packet_compare_other(Packet *spkt, > > > > Packet *ppkt) > > > > > > > > static int colo_old_packet_check_one(Packet *pkt, void *user_data) { > > > > - int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST); > > > > + int64_t now = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > > > > uint32_t check_time = *(uint32_t *)user_data; > > > > > > > > if ((now - pkt->creation_ms) > check_time) { diff --git > > > > a/net/colo.c b/net/colo.c index a6c66d829a..0441910169 100644 > > > > --- a/net/colo.c > > > > +++ b/net/colo.c > > > > @@ -164,7 +164,7 @@ Packet *packet_new(const void *data, int size, int > > > > vnet_hdr_len) > > > > > > > > pkt->data = g_memdup(data, size); > > > > pkt->size = size; > > > > - pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_HOST); > > > > + pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > > > > pkt->vnet_hdr_len = vnet_hdr_len; > > > > pkt->tcp_seq = 0; > > > > pkt->tcp_ack = 0; > > > > -- > > > > 2.25.1 > >
diff --git a/net/colo-compare.c b/net/colo-compare.c index c4de86ef34..29d7f986e3 100644 --- a/net/colo-compare.c +++ b/net/colo-compare.c @@ -621,7 +621,7 @@ static int colo_packet_compare_other(Packet *spkt, Packet *ppkt) static int colo_old_packet_check_one(Packet *pkt, void *user_data) { - int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST); + int64_t now = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); uint32_t check_time = *(uint32_t *)user_data; if ((now - pkt->creation_ms) > check_time) { diff --git a/net/colo.c b/net/colo.c index a6c66d829a..0441910169 100644 --- a/net/colo.c +++ b/net/colo.c @@ -164,7 +164,7 @@ Packet *packet_new(const void *data, int size, int vnet_hdr_len) pkt->data = g_memdup(data, size); pkt->size = size; - pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_HOST); + pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); pkt->vnet_hdr_len = vnet_hdr_len; pkt->tcp_seq = 0; pkt->tcp_ack = 0;
Record packet creation time by QEMU_CLOCK_REALTIME instead of QEMU_CLOCK_HOST. The time difference between `now` and packet `creation_ms` has the possibility of an unexpected negative value and results in wrong comparison after changing the host clock. Signed-off-by: Derek Su <dereksu@qnap.com> --- net/colo-compare.c | 2 +- net/colo.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)