| Message ID | 20200328124646.7778-3-dereksu@qnap.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | COLO: handling of the full primary or secondary queue | expand |
> Subject: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary or > secondary queue > > The pervious handling of the full primary or queue is only dropping the > packet. If there are lots of clients to the guest VM, the "drop" will lead to the > lost of the networking connection until next checkpoint. > > To address the issue, this patch drops the packet firstly. > Then, do checkpoint and flush packets. > > Signed-off-by: Derek Su <dereksu@qnap.com> Looks good for me. Reviewed-by: Zhang Chen <chen.zhang@intel.com> Thanks Zhang Chen > --- > net/colo-compare.c | 39 ++++++++++++++++++++++++++++----------- > 1 file changed, 28 insertions(+), 11 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > cdd87b2aa8..fe8779cf2d 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -125,6 +125,12 @@ static const char *colo_mode[] = { > [SECONDARY_IN] = "secondary", > }; > > +enum { > + QUEUE_INSERT_ERR = -1, > + QUEUE_INSERT_OK = 0, > + QUEUE_INSERT_FULL = 1, > +}; > + > static int compare_chr_send(CompareState *s, > const uint8_t *buf, > uint32_t size, @@ -211,8 +217,10 @@ static int > colo_insert_packet(GQueue *queue, Packet *pkt, uint32_t *max_ack) } > > /* > - * Return 0 on success, if return -1 means the pkt > - * is unsupported(arp and ipv6) and will be sent later > + * Return QUEUE_INSERT_OK on success. > + * If return QUEUE_INSERT_FULL means list is full, and > + * QUEUE_INSERT_ERR means the pkt is unsupported(arp and ipv6) and > + * will be sent later > */ > static int packet_enqueue(CompareState *s, int mode, Connection **con) > { @@ -234,7 +242,7 @@ static int packet_enqueue(CompareState *s, int > mode, Connection **con) > if (parse_packet_early(pkt)) { > packet_destroy(pkt, NULL); > pkt = NULL; > - return -1; > + return QUEUE_INSERT_ERR; > } > fill_connection_key(pkt, &key); > > @@ -258,11 +266,12 @@ static int packet_enqueue(CompareState *s, int > mode, Connection **con) > "drop packet", colo_mode[mode]); > packet_destroy(pkt, NULL); > pkt = NULL; > + return QUEUE_INSERT_FULL; > } > > *con = conn; > > - return 0; > + return QUEUE_INSERT_OK; > } > > static inline bool after(uint32_t seq1, uint32_t seq2) @@ -995,17 +1004,21 > @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs) { > CompareState *s = container_of(pri_rs, CompareState, pri_rs); > Connection *conn = NULL; > + int ret; > > - if (packet_enqueue(s, PRIMARY_IN, &conn)) { > + ret = packet_enqueue(s, PRIMARY_IN, &conn); > + if (ret == QUEUE_INSERT_OK) { > + /* compare packet in the specified connection */ > + colo_compare_connection(conn, s); > + } else if (ret == QUEUE_INSERT_FULL) { > + colo_compare_inconsistency_notify(s); > + } else { > trace_colo_compare_main("primary: unsupported packet in"); > compare_chr_send(s, > pri_rs->buf, > pri_rs->packet_len, > pri_rs->vnet_hdr_len, > false); > - } else { > - /* compare packet in the specified connection */ > - colo_compare_connection(conn, s); > } > } > > @@ -1013,12 +1026,16 @@ static void > compare_sec_rs_finalize(SocketReadState *sec_rs) { > CompareState *s = container_of(sec_rs, CompareState, sec_rs); > Connection *conn = NULL; > + int ret; > > - if (packet_enqueue(s, SECONDARY_IN, &conn)) { > - trace_colo_compare_main("secondary: unsupported packet in"); > - } else { > + ret = packet_enqueue(s, SECONDARY_IN, &conn); > + if (ret == QUEUE_INSERT_OK) { > /* compare packet in the specified connection */ > colo_compare_connection(conn, s); > + } else if (ret == QUEUE_INSERT_FULL) { > + colo_compare_inconsistency_notify(s); > + } else { > + trace_colo_compare_main("secondary: unsupported packet in"); > } > } > > -- > 2.17.1
On Sat, 28 Mar 2020 20:46:46 +0800 Derek Su <dereksu@qnap.com> wrote: > The pervious handling of the full primary or queue is only dropping > the packet. If there are lots of clients to the guest VM, > the "drop" will lead to the lost of the networking connection > until next checkpoint. > > To address the issue, this patch drops the packet firstly. > Then, do checkpoint and flush packets. > > Signed-off-by: Derek Su <dereksu@qnap.com> Looks good and works well in my tests. Reviewed-by: Lukas Straub <lukasstraub2@web.de> Tested-by: Lukas Straub <lukasstraub2@web.de> Regards, Lukas Straub > --- > net/colo-compare.c | 39 ++++++++++++++++++++++++++++----------- > 1 file changed, 28 insertions(+), 11 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c > index cdd87b2aa8..fe8779cf2d 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -125,6 +125,12 @@ static const char *colo_mode[] = { > [SECONDARY_IN] = "secondary", > }; > > +enum { > + QUEUE_INSERT_ERR = -1, > + QUEUE_INSERT_OK = 0, > + QUEUE_INSERT_FULL = 1, > +}; > + > static int compare_chr_send(CompareState *s, > const uint8_t *buf, > uint32_t size, > @@ -211,8 +217,10 @@ static int colo_insert_packet(GQueue *queue, Packet *pkt, uint32_t *max_ack) > } > > /* > - * Return 0 on success, if return -1 means the pkt > - * is unsupported(arp and ipv6) and will be sent later > + * Return QUEUE_INSERT_OK on success. > + * If return QUEUE_INSERT_FULL means list is full, and > + * QUEUE_INSERT_ERR means the pkt is unsupported(arp and ipv6) and > + * will be sent later > */ > static int packet_enqueue(CompareState *s, int mode, Connection **con) > { > @@ -234,7 +242,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con) > if (parse_packet_early(pkt)) { > packet_destroy(pkt, NULL); > pkt = NULL; > - return -1; > + return QUEUE_INSERT_ERR; > } > fill_connection_key(pkt, &key); > > @@ -258,11 +266,12 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con) > "drop packet", colo_mode[mode]); > packet_destroy(pkt, NULL); > pkt = NULL; > + return QUEUE_INSERT_FULL; > } > > *con = conn; > > - return 0; > + return QUEUE_INSERT_OK; > } > > static inline bool after(uint32_t seq1, uint32_t seq2) > @@ -995,17 +1004,21 @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs) > { > CompareState *s = container_of(pri_rs, CompareState, pri_rs); > Connection *conn = NULL; > + int ret; > > - if (packet_enqueue(s, PRIMARY_IN, &conn)) { > + ret = packet_enqueue(s, PRIMARY_IN, &conn); > + if (ret == QUEUE_INSERT_OK) { > + /* compare packet in the specified connection */ > + colo_compare_connection(conn, s); > + } else if (ret == QUEUE_INSERT_FULL) { > + colo_compare_inconsistency_notify(s); > + } else { > trace_colo_compare_main("primary: unsupported packet in"); > compare_chr_send(s, > pri_rs->buf, > pri_rs->packet_len, > pri_rs->vnet_hdr_len, > false); > - } else { > - /* compare packet in the specified connection */ > - colo_compare_connection(conn, s); > } > } > > @@ -1013,12 +1026,16 @@ static void compare_sec_rs_finalize(SocketReadState *sec_rs) > { > CompareState *s = container_of(sec_rs, CompareState, sec_rs); > Connection *conn = NULL; > + int ret; > > - if (packet_enqueue(s, SECONDARY_IN, &conn)) { > - trace_colo_compare_main("secondary: unsupported packet in"); > - } else { > + ret = packet_enqueue(s, SECONDARY_IN, &conn); > + if (ret == QUEUE_INSERT_OK) { > /* compare packet in the specified connection */ > colo_compare_connection(conn, s); > + } else if (ret == QUEUE_INSERT_FULL) { > + colo_compare_inconsistency_notify(s); > + } else { > + trace_colo_compare_main("secondary: unsupported packet in"); > } > } >
On Sat, 28 Mar 2020 20:46:46 +0800 Derek Su <dereksu@qnap.com> wrote: > The pervious handling of the full primary or queue is only dropping > the packet. If there are lots of clients to the guest VM, > the "drop" will lead to the lost of the networking connection > until next checkpoint. > > To address the issue, this patch drops the packet firstly. > Then, do checkpoint and flush packets. > > Signed-off-by: Derek Su <dereksu@qnap.com> Hello, I had a look at this again and did some benchmarking. First just qemu 5.0-rc1 with my bugfixes ( https://lists.nongnu.org/archive/html/qemu-devel/2020-04/msg01432.html ) Then qemu 5.0-rc1 with my bugfixes and this patch series. This commit hurts performance too much: Client-to-server bandwidth falls from ~45.9 Mbit/s to 22.9 Mbit/s. Server-to-client bandwidth falls from ~6.3 Mbit/s to just ~674 Kbit/s. Average latency rises from ~197ms to ~397ms. Meanwhile the packet loss without this commit is negligible, only 1-2 ping packets got lost during each test run. Instead I think we should just turn the error message into a trace so it doesn't flood the logs. Regards, Lukas Straub 5.0-rc1 with bugfixes: *** iperf -c 192.168.178.65 *** [ 3] local 192.168.178.68 port 33418 connected with 192.168.178.65 port 5001 64 bytes from 192.168.178.65: icmp_seq=53 ttl=64 time=2.57 ms [ ID] Interval Transfer Bandwidth 64 bytes from 192.168.178.65: icmp_seq=54 ttl=64 time=142 ms [ 3] 0.0-10.2 sec 60.1 MBytes 49.4 Mbits/sec 64 bytes from 192.168.178.65: icmp_seq=55 ttl=64 time=2.64 ms [ 3] Sent 892 datagrams rtt min/avg/max/mdev = 1.920/235.514/1810.653/411.394 ms, pipe 4 [ 3] local 192.168.178.68 port 33420 connected with 192.168.178.65 port 5001 64 bytes from 192.168.178.65: icmp_seq=55 ttl=64 time=24.3 ms [ ID] Interval Transfer Bandwidth 64 bytes from 192.168.178.65: icmp_seq=56 ttl=64 time=505 ms [ 3] 0.0-10.3 sec 51.0 MBytes 41.7 Mbits/sec 64 bytes from 192.168.178.65: icmp_seq=57 ttl=64 time=11.7 ms [ 3] Sent 892 datagrams rtt min/avg/max/mdev = 1.962/218.713/1365.510/344.164 ms, pipe 3 [ 3] local 192.168.178.68 port 33422 connected with 192.168.178.65 port 5001 64 bytes from 192.168.178.65: icmp_seq=54 ttl=64 time=49.2 ms [ ID] Interval Transfer Bandwidth 64 bytes from 192.168.178.65: icmp_seq=55 ttl=64 time=587 ms [ 3] 0.0-10.8 sec 60.1 MBytes 46.6 Mbits/sec 64 bytes from 192.168.178.65: icmp_seq=56 ttl=64 time=167 ms [ 3] Sent 892 datagrams rtt min/avg/max/mdev = 1.573/276.375/1384.085/362.027 ms, pipe 3 *** iperf -c 192.168.178.65 -d *** [ 4] local 192.168.178.68 port 33424 connected with 192.168.178.65 port 5001 64 bytes from 192.168.178.65: icmp_seq=60 ttl=64 time=2.27 ms [ 5] local 192.168.178.68 port 5001 connected with 192.168.178.65 port 48440 ^C [ ID] Interval Transfer Bandwidth --- 192.168.178.65 ping statistics --- [ 4] 0.0-10.1 sec 60.8 MBytes 50.4 Mbits/sec 60 packets transmitted, 59 received, 1.66667% packet loss, time 29595ms [ 5] 0.0-11.7 sec 11.2 MBytes 8.05 Mbits/sec rtt min/avg/max/mdev = 1.785/254.258/1352.916/395.070 ms, pipe 3 [ 5] local 192.168.178.68 port 33426 connected with 192.168.178.65 port 5001 64 bytes from 192.168.178.65: icmp_seq=59 ttl=64 time=2.98 ms [ 4] local 192.168.178.68 port 5001 connected with 192.168.178.65 port 48442 ^C [ ID] Interval Transfer Bandwidth --- 192.168.178.65 ping statistics --- [ 5] 0.0-10.3 sec 58.6 MBytes 47.6 Mbits/sec 59 packets transmitted, 59 received, 0% packet loss, time 29088ms [ 4] 0.0-11.9 sec 13.8 MBytes 9.77 Mbits/sec rtt min/avg/max/mdev = 2.026/207.507/1518.013/331.972 ms, pipe 4 [ 5] local 192.168.178.68 port 33432 connected with 192.168.178.65 port 5001 64 bytes from 192.168.178.65: icmp_seq=93 ttl=64 time=12.6 ms [ 4] local 192.168.178.68 port 5001 connected with 192.168.178.65 port 48446 ^C [ ID] Interval Transfer Bandwidth --- 192.168.178.65 ping statistics --- [ 5] 0.0-10.2 sec 59.4 MBytes 49.0 Mbits/sec 93 packets transmitted, 92 received, 1.07527% packet loss, time 46137ms [ 4] 0.0-31.0 sec 4.41 MBytes 1.19 Mbits/sec rtt min/avg/max/mdev = 2.107/131.212/1405.670/287.854 ms, pipe 3 5.0-rc1 with bugfixes and checkpoint on queue full: *** iperf -c 192.168.178.65 *** [ 3] local 192.168.178.68 port 33402 connected with 192.168.178.65 port 5001 64 bytes from 192.168.178.65: icmp_seq=52 ttl=64 time=1329 ms [ ID] Interval Transfer Bandwidth 64 bytes from 192.168.178.65: icmp_seq=53 ttl=64 time=828 ms [ 3] 0.0-10.1 sec 35.9 MBytes 29.9 Mbits/sec 64 bytes from 192.168.178.65: icmp_seq=54 ttl=64 time=619 ms [ 3] Sent 892 datagrams rtt min/avg/max/mdev = 2.183/788.828/3329.045/958.683 ms, pipe 7 [ 3] local 192.168.178.68 port 33404 connected with 192.168.178.65 port 5001 64 bytes from 192.168.178.65: icmp_seq=57 ttl=64 time=2.58 ms [ ID] Interval Transfer Bandwidth 64 bytes from 192.168.178.65: icmp_seq=58 ttl=64 time=322 ms [ 3] 0.0-11.5 sec 36.5 MBytes 26.6 Mbits/sec 64 bytes from 192.168.178.65: icmp_seq=59 ttl=64 time=10.2 ms [ 3] Sent 892 datagrams rtt min/avg/max/mdev = 1.962/610.122/3303.544/907.708 ms, pipe 7 [ 3] local 192.168.178.68 port 33406 connected with 192.168.178.65 port 5001 64 bytes from 192.168.178.65: icmp_seq=55 ttl=64 time=2.60 ms [ ID] Interval Transfer Bandwidth 64 bytes from 192.168.178.65: icmp_seq=56 ttl=64 time=112 ms [ 3] 0.0-11.1 sec 16.1 MBytes 12.2 Mbits/sec 64 bytes from 192.168.178.65: icmp_seq=57 ttl=64 time=118 ms [ 3] Sent 892 datagrams rtt min/avg/max/mdev = 2.088/593.139/2734.770/857.021 ms, pipe 6 *** iperf -c 192.168.178.65 -d *** [ 5] local 192.168.178.68 port 33408 connected with 192.168.178.65 port 5001 64 bytes from 192.168.178.65: icmp_seq=75 ttl=64 time=2.64 ms [ 4] local 192.168.178.68 port 5001 connected with 192.168.178.65 port 47492 ^C [ ID] Interval Transfer Bandwidth --- 192.168.178.65 ping statistics --- [ 5] 0.0-10.3 sec 51.2 MBytes 41.9 Mbits/sec 75 packets transmitted, 75 received, 0% packet loss, time 37122ms [ 4] 0.0-25.0 sec 1.00 MBytes 336 Kbits/sec rtt min/avg/max/mdev = 1.830/563.652/1996.992/651.117 ms, pipe 4 [ 4] local 192.168.178.68 port 33410 connected with 192.168.178.65 port 5001 64 bytes from 192.168.178.65: icmp_seq=127 ttl=64 time=2.41 ms [ 5] local 192.168.178.68 port 5001 connected with 192.168.178.65 port 47494 ^C [ ID] Interval Transfer Bandwidth --- 192.168.178.65 ping statistics --- [ 4] 0.0-10.1 sec 39.0 MBytes 32.4 Mbits/sec 127 packets transmitted, 126 received, 0.787402% packet loss, time 63205ms [ 5] 0.0-50.8 sec 5.01 MBytes 826 Kbits/sec rtt min/avg/max/mdev = 1.791/239.200/2105.867/513.022 ms, pipe 5 [ 4] local 192.168.178.68 port 33414 connected with 192.168.178.65 port 5001 64 bytes from 192.168.178.65: icmp_seq=90 ttl=64 time=69.4 ms [ 5] local 192.168.178.68 port 5001 connected with 192.168.178.65 port 47496 ^C [ ID] Interval Transfer Bandwidth --- 192.168.178.65 ping statistics --- [ 4] 0.0-10.5 sec 46.1 MBytes 37.0 Mbits/sec 90 packets transmitted, 90 received, 0% packet loss, time 44643ms [ 5] 0.0-31.8 sec 3.26 MBytes 861 Kbits/sec rtt min/avg/max/mdev = 1.909/391.216/1878.484/645.897 ms, pipe 4
> -----Original Message----- > From: Lukas Straub <lukasstraub2@web.de> > Sent: Thursday, April 9, 2020 3:19 AM > To: Derek Su <dereksu@qnap.com> > Cc: qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; chyang@qnap.com; > jasowang@redhat.com; ctcheng@qnap.com; Zhang, Chen > <chen.zhang@intel.com>; jwsu1986@gmail.com > Subject: Re: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary > or secondary queue > > On Sat, 28 Mar 2020 20:46:46 +0800 > Derek Su <dereksu@qnap.com> wrote: > > > The pervious handling of the full primary or queue is only dropping > > the packet. If there are lots of clients to the guest VM, the "drop" > > will lead to the lost of the networking connection until next > > checkpoint. > > > > To address the issue, this patch drops the packet firstly. > > Then, do checkpoint and flush packets. > > > > Signed-off-by: Derek Su <dereksu@qnap.com> > > Hello, > I had a look at this again and did some benchmarking. > First just qemu 5.0-rc1 with my bugfixes > ( https://lists.nongnu.org/archive/html/qemu-devel/2020- > 04/msg01432.html ) Then qemu 5.0-rc1 with my bugfixes and this patch > series. > > This commit hurts performance too much: > Client-to-server bandwidth falls from ~45.9 Mbit/s to 22.9 Mbit/s. > Server-to-client bandwidth falls from ~6.3 Mbit/s to just ~674 Kbit/s. > Average latency rises from ~197ms to ~397ms. > > Meanwhile the packet loss without this commit is negligible, only 1-2 ping > packets got lost during each test run. > > Instead I think we should just turn the error message into a trace so it > doesn't flood the logs. We re-test this patch, Lukas is right. Sorry for the original idea, looks like it did not show better performance in the test. Agree with Lukas's comments. Derek, can you please change it? Thanks Zhang Chen > > Regards, > Lukas Straub
Hello, Zhang and Lukas
Sure, after my re-test, the performance is hurt. Will update it later.
By the way, could I also move the "error_report("colo compare
primary/secondary queue size too big, drop packet");" to trace?
The use of error_report is a little strange and make a flood in log.
May I also make "MAX_QUEUE_SIZE" be user-configurable in this series?
Thanks,
Derek Su
Zhang, Chen <chen.zhang@intel.com> 於 2020年4月9日 週四 下午2:59寫道：
>
>
>
> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Thursday, April 9, 2020 3:19 AM
> > To: Derek Su <dereksu@qnap.com>
> > Cc: qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; chyang@qnap.com;
> > jasowang@redhat.com; ctcheng@qnap.com; Zhang, Chen
> > <chen.zhang@intel.com>; jwsu1986@gmail.com
> > Subject: Re: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary
> > or secondary queue
> >
> > On Sat, 28 Mar 2020 20:46:46 +0800
> > Derek Su <dereksu@qnap.com> wrote:
> >
> > > The pervious handling of the full primary or queue is only dropping
> > > the packet. If there are lots of clients to the guest VM, the "drop"
> > > will lead to the lost of the networking connection until next
> > > checkpoint.
> > >
> > > To address the issue, this patch drops the packet firstly.
> > > Then, do checkpoint and flush packets.
> > >
> > > Signed-off-by: Derek Su <dereksu@qnap.com>
> >
> > Hello,
> > I had a look at this again and did some benchmarking.
> > First just qemu 5.0-rc1 with my bugfixes
> > ( https://lists.nongnu.org/archive/html/qemu-devel/2020-
> > 04/msg01432.html ) Then qemu 5.0-rc1 with my bugfixes and this patch
> > series.
> >
> > This commit hurts performance too much:
> > Client-to-server bandwidth falls from ~45.9 Mbit/s to 22.9 Mbit/s.
> > Server-to-client bandwidth falls from ~6.3 Mbit/s to just ~674 Kbit/s.
> > Average latency rises from ~197ms to ~397ms.
> >
> > Meanwhile the packet loss without this commit is negligible, only 1-2 ping
> > packets got lost during each test run.
> >
> > Instead I think we should just turn the error message into a trace so it
> > doesn't flood the logs.
>
> We re-test this patch, Lukas is right.
> Sorry for the original idea, looks like it did not show better performance in the test.
> Agree with Lukas's comments. Derek, can you please change it?
>
> Thanks
> Zhang Chen
>
>
> >
> > Regards,
> > Lukas Straub
> -----Original Message----- > From: Derek Su <jwsu1986@gmail.com> > Sent: Thursday, April 9, 2020 3:11 PM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: Lukas Straub <lukasstraub2@web.de>; Derek Su <dereksu@qnap.com>; > qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; chyang@qnap.com; > jasowang@redhat.com; ctcheng@qnap.com > Subject: Re: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary > or secondary queue > > Hello, Zhang and Lukas > > Sure, after my re-test, the performance is hurt. Will update it later. > > By the way, could I also move the "error_report("colo compare > primary/secondary queue size too big, drop packet");" to trace? Yes. > The use of error_report is a little strange and make a flood in log. > > May I also make "MAX_QUEUE_SIZE" be user-configurable in this series? I have this patch internal and will send it later. Thanks Zhang Chen > > Thanks, > Derek Su > > > > > Zhang, Chen <chen.zhang@intel.com> 於 2020年4月9日 週四 下午2:59 > 寫道： > > > > > > > > > -----Original Message----- > > > From: Lukas Straub <lukasstraub2@web.de> > > > Sent: Thursday, April 9, 2020 3:19 AM > > > To: Derek Su <dereksu@qnap.com> > > > Cc: qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; > > > chyang@qnap.com; jasowang@redhat.com; ctcheng@qnap.com; Zhang, > Chen > > > <chen.zhang@intel.com>; jwsu1986@gmail.com > > > Subject: Re: [PATCH v4 2/2] net/colo-compare.c: handling of the full > > > primary or secondary queue > > > > > > On Sat, 28 Mar 2020 20:46:46 +0800 > > > Derek Su <dereksu@qnap.com> wrote: > > > > > > > The pervious handling of the full primary or queue is only > > > > dropping the packet. If there are lots of clients to the guest VM, the > "drop" > > > > will lead to the lost of the networking connection until next > > > > checkpoint. > > > > > > > > To address the issue, this patch drops the packet firstly. > > > > Then, do checkpoint and flush packets. > > > > > > > > Signed-off-by: Derek Su <dereksu@qnap.com> > > > > > > Hello, > > > I had a look at this again and did some benchmarking. > > > First just qemu 5.0-rc1 with my bugfixes ( > > > https://lists.nongnu.org/archive/html/qemu-devel/2020- > > > 04/msg01432.html ) Then qemu 5.0-rc1 with my bugfixes and this patch > > > series. > > > > > > This commit hurts performance too much: > > > Client-to-server bandwidth falls from ~45.9 Mbit/s to 22.9 Mbit/s. > > > Server-to-client bandwidth falls from ~6.3 Mbit/s to just ~674 Kbit/s. > > > Average latency rises from ~197ms to ~397ms. > > > > > > Meanwhile the packet loss without this commit is negligible, only > > > 1-2 ping packets got lost during each test run. > > > > > > Instead I think we should just turn the error message into a trace > > > so it doesn't flood the logs. > > > > We re-test this patch, Lukas is right. > > Sorry for the original idea, looks like it did not show better performance in > the test. > > Agree with Lukas's comments. Derek, can you please change it? > > > > Thanks > > Zhang Chen > > > > > > > > > > Regards, > > > Lukas Straub
diff --git a/net/colo-compare.c b/net/colo-compare.c index cdd87b2aa8..fe8779cf2d 100644 --- a/net/colo-compare.c +++ b/net/colo-compare.c @@ -125,6 +125,12 @@ static const char *colo_mode[] = { [SECONDARY_IN] = "secondary", }; +enum { + QUEUE_INSERT_ERR = -1, + QUEUE_INSERT_OK = 0, + QUEUE_INSERT_FULL = 1, +}; + static int compare_chr_send(CompareState *s, const uint8_t *buf, uint32_t size, @@ -211,8 +217,10 @@ static int colo_insert_packet(GQueue *queue, Packet *pkt, uint32_t *max_ack) } /* - * Return 0 on success, if return -1 means the pkt - * is unsupported(arp and ipv6) and will be sent later + * Return QUEUE_INSERT_OK on success. + * If return QUEUE_INSERT_FULL means list is full, and + * QUEUE_INSERT_ERR means the pkt is unsupported(arp and ipv6) and + * will be sent later */ static int packet_enqueue(CompareState *s, int mode, Connection **con) { @@ -234,7 +242,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con) if (parse_packet_early(pkt)) { packet_destroy(pkt, NULL); pkt = NULL; - return -1; + return QUEUE_INSERT_ERR; } fill_connection_key(pkt, &key); @@ -258,11 +266,12 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con) "drop packet", colo_mode[mode]); packet_destroy(pkt, NULL); pkt = NULL; + return QUEUE_INSERT_FULL; } *con = conn; - return 0; + return QUEUE_INSERT_OK; } static inline bool after(uint32_t seq1, uint32_t seq2) @@ -995,17 +1004,21 @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs) { CompareState *s = container_of(pri_rs, CompareState, pri_rs); Connection *conn = NULL; + int ret; - if (packet_enqueue(s, PRIMARY_IN, &conn)) { + ret = packet_enqueue(s, PRIMARY_IN, &conn); + if (ret == QUEUE_INSERT_OK) { + /* compare packet in the specified connection */ + colo_compare_connection(conn, s); + } else if (ret == QUEUE_INSERT_FULL) { + colo_compare_inconsistency_notify(s); + } else { trace_colo_compare_main("primary: unsupported packet in"); compare_chr_send(s, pri_rs->buf, pri_rs->packet_len, pri_rs->vnet_hdr_len, false); - } else { - /* compare packet in the specified connection */ - colo_compare_connection(conn, s); } } @@ -1013,12 +1026,16 @@ static void compare_sec_rs_finalize(SocketReadState *sec_rs) { CompareState *s = container_of(sec_rs, CompareState, sec_rs); Connection *conn = NULL; + int ret; - if (packet_enqueue(s, SECONDARY_IN, &conn)) { - trace_colo_compare_main("secondary: unsupported packet in"); - } else { + ret = packet_enqueue(s, SECONDARY_IN, &conn); + if (ret == QUEUE_INSERT_OK) { /* compare packet in the specified connection */ colo_compare_connection(conn, s); + } else if (ret == QUEUE_INSERT_FULL) { + colo_compare_inconsistency_notify(s); + } else { + trace_colo_compare_main("secondary: unsupported packet in"); } }
The pervious handling of the full primary or queue is only dropping the packet. If there are lots of clients to the guest VM, the "drop" will lead to the lost of the networking connection until next checkpoint. To address the issue, this patch drops the packet firstly. Then, do checkpoint and flush packets. Signed-off-by: Derek Su <dereksu@qnap.com> --- net/colo-compare.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)