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

[Derek Su <dereksu@qnap.com>]

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