[PATCH v1 0/2] colo-compare bugfixes

[PATCH v1 0/2] colo-compare bugfixes

[Derek Su]

Hello,

The fixes are for the bugs found in colo-compare during our testing
and applications.

Please help to review, thanks a lot.

Regards,
Derek Su

Derek Su (2):
  colo-compare: Fix incorrect data type conversion
  colo-compare: Record packet creation time by QEMU_CLOCK_REALTIME

 net/colo-compare.c | 7 ++++---
 net/colo.c         | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v1 1/2] colo-compare: Fix incorrect data type conversion

[Derek Su]

Fix data type conversion of compare_timeout. The incorrect
conversion results in a random compare_timeout value and
unexpected stalls in packet comparison.

Signed-off-by: Derek Su <dereksu@qnap.com>
---
 net/colo-compare.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 2c20de1537..c4de86ef34 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -619,11 +619,12 @@ static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
                                        ppkt->size - offset);
 }
 
-static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time)
+static int colo_old_packet_check_one(Packet *pkt, void *user_data)
 {
     int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
+    uint32_t check_time = *(uint32_t *)user_data;
 
-    if ((now - pkt->creation_ms) > (*check_time)) {
+    if ((now - pkt->creation_ms) > check_time) {
         trace_colo_old_packet_check_found(pkt->creation_ms);
         return 0;
     } else {
-- 
2.25.1

RE: [PATCH v1 1/2] colo-compare: Fix incorrect data type conversion

[Zhang, Chen]

> -----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 1/2] colo-compare: Fix incorrect data type conversion
> 
> Fix data type conversion of compare_timeout. The incorrect conversion
> results in a random compare_timeout value and unexpected stalls in packet
> comparison.
> 

This bug already found on our internal test too. Just waiting to send.
Good catch! But this patch not fixed the root cause.
Change the compare_timeout from uint32_t to uint64_t is better.
I will send a patch for this and tag reported by you.

Thanks
Zhang Chen


> Signed-off-by: Derek Su <dereksu@qnap.com>
> ---
>  net/colo-compare.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 2c20de1537..c4de86ef34 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -619,11 +619,12 @@ static int colo_packet_compare_other(Packet *spkt,
> Packet *ppkt)
>                                         ppkt->size - offset);  }
> 
> -static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time)
> +static int colo_old_packet_check_one(Packet *pkt, void *user_data)
>  {
>      int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> +    uint32_t check_time = *(uint32_t *)user_data;
> 
> -    if ((now - pkt->creation_ms) > (*check_time)) {
> +    if ((now - pkt->creation_ms) > check_time) {
>          trace_colo_old_packet_check_found(pkt->creation_ms);
>          return 0;
>      } else {
> --
> 2.25.1

Re: [PATCH v1 1/2] colo-compare: Fix incorrect data type conversion

[Derek Su]

[-- Attachment #1: Type: text/plain, Size: 2335 bytes --]

Hi, Chen

Got it, thank you :)

Regards,
Derek

Zhang, Chen <chen.zhang@intel.com>於 2020年9月14日 週一，上午4:02寫道：

>
>
>
>
> > -----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 1/2] colo-compare: Fix incorrect data type conversion
>
> >
>
> > Fix data type conversion of compare_timeout. The incorrect conversion
>
> > results in a random compare_timeout value and unexpected stalls in packet
>
> > comparison.
>
> >
>
>
>
> This bug already found on our internal test too. Just waiting to send.
>
> Good catch! But this patch not fixed the root cause.
>
> Change the compare_timeout from uint32_t to uint64_t is better.
>
> I will send a patch for this and tag reported by you.
>
>
>
> Thanks
>
> Zhang Chen
>
>
>
>
>
> > Signed-off-by: Derek Su <dereksu@qnap.com>
>
> > ---
>
> >  net/colo-compare.c | 5 +++--
>
> >  1 file changed, 3 insertions(+), 2 deletions(-)
>
> >
>
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
>
> > 2c20de1537..c4de86ef34 100644
>
> > --- a/net/colo-compare.c
>
> > +++ b/net/colo-compare.c
>
> > @@ -619,11 +619,12 @@ static int colo_packet_compare_other(Packet *spkt,
>
> > Packet *ppkt)
>
> >                                         ppkt->size - offset);  }
>
> >
>
> > -static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time)
>
> > +static int colo_old_packet_check_one(Packet *pkt, void *user_data)
>
> >  {
>
> >      int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>
> > +    uint32_t check_time = *(uint32_t *)user_data;
>
> >
>
> > -    if ((now - pkt->creation_ms) > (*check_time)) {
>
> > +    if ((now - pkt->creation_ms) > check_time) {
>
> >          trace_colo_old_packet_check_found(pkt->creation_ms);
>
> >          return 0;
>
> >      } else {
>
> > --
>
> > 2.25.1
>
>
>
> --

Best regards,

Derek Su
QNAP Systems, Inc.
Email: dereksu@qnap.com
Tel: (+886)-2-2393-5152 ext. 15017
Address: 13F., No.56, Sec. 1, Xinsheng S. Rd., Zhongzheng Dist., Taipei
City, Taiwan

[-- Attachment #2: Type: text/html, Size: 4724 bytes --]

[PATCH v1 2/2] colo-compare: Record packet creation time by QEMU_CLOCK_REALTIME

[Derek Su]

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(-)

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

RE: [PATCH v1 2/2] colo-compare: Record packet creation time by QEMU_CLOCK_REALTIME

[Zhang, Chen]

> -----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

RE: [PATCH v1 2/2] colo-compare: Record packet creation time by QEMU_CLOCK_REALTIME

[Zhang, Chen]

> -----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

Re: [PATCH v1 2/2] colo-compare: Record packet creation time by QEMU_CLOCK_REALTIME

[Derek Su]

[-- Attachment #1: Type: text/plain, Size: 3655 bytes --]

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
>
>
>
>

[-- Attachment #2: Type: text/html, Size: 5805 bytes --]

RE: [PATCH v1 2/2] colo-compare: Record packet creation time by QEMU_CLOCK_REALTIME

[Zhang, Chen]

[-- Attachment #1: Type: text/plain, Size: 4912 bytes --]



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



[-- Attachment #2: Type: text/html, Size: 13543 bytes --]

Re: [PATCH v1 2/2] colo-compare: Record packet creation time by QEMU_CLOCK_REALTIME

[Derek Su]

[-- Attachment #1: Type: text/plain, Size: 5787 bytes --]

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
>
>
>

[-- Attachment #2: Type: text/html, Size: 13933 bytes --]

RE: [PATCH v1 2/2] colo-compare: Record packet creation time by QEMU_CLOCK_REALTIME

[Zhang, Chen]

[-- Attachment #1: Type: text/plain, Size: 6450 bytes --]

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


[-- Attachment #2: Type: text/html, Size: 20644 bytes --]

Re: [PATCH v1 2/2] colo-compare: Record packet creation time by QEMU_CLOCK_REALTIME

[Derek Su]

[-- Attachment #1: Type: text/plain, Size: 7058 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 18487 bytes --]