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

["Zhang, Chen" <chen.zhang@intel.com>]

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