RE: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send
> -Original Message-
> From: Lukas Straub
> Sent: Friday, May 8, 2020 3:56 PM
> To: Zhang, Chen
> Cc: qemu-devel ; Li Zhijian
> ; Jason Wang ; Marc-
> André Lureau ; Paolo Bonzini
>
> Subject: Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> compare_chr_send
>
> On Fri, 8 May 2020 06:28:45 +
> "Zhang, Chen" wrote:
>
> > > -Original Message-
> > > From: Lukas Straub
> > > Sent: Friday, May 8, 2020 2:08 PM
> > > To: Zhang, Chen
> > > Cc: qemu-devel ; Li Zhijian
> > > ; Jason Wang ; Marc-
> > > André Lureau ; Paolo Bonzini
> > >
> > > Subject: Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> > > compare_chr_send
> > >
> > > On Fri, 8 May 2020 02:19:00 +
> > > "Zhang, Chen" wrote:
> > > > > > No need to init the notify_sendco each time, because the
> > > > > > notify dev just
> > > > > an optional parameter.
> > > > > > You can use the if (s->notify_dev) here. Just Xen use the
> > > chr_notify_dev.
> > > > >
> > > > > Ok, I will change that and the code below in the next version.
> > > > >
> > > > > > Overall, make the chr_send job to coroutine is a good idea. It
> > > > > > looks good
> > > > > for me.
> > > > > > And your patch inspired me, it looks we can re-use the
> > > > > > compare_chr_send
> > > > > code on filter mirror/redirector too.
> > > > >
> > > > > I already have patch for that, but I don't think it is a good
> > > > > idea, because the guest then can send packets faster than
> > > > > colo-compare can process. This leads bufferbloat and the
> performance drops in my tests:
> > > > > Client-to-server tcp:
> > > > > without patch: ~66 Mbit/s
> > > > > with patch: ~59 Mbit/s
> > > > > Server-to-client tcp:
> > > > > without patch: ~702 Kbit/s
> > > > > with patch: ~328 Kbit/s
> > > >
> > > > Oh, a big performance drop, is that caused by memcpy/zero_copy
> parts ?
> > > >
> > > > Thanks
> > > > Zhang Chen
> > >
> > > No, there is no memcpy overhead with this patch, see below.
> >
> > I means for the filter mirror/redirector parts why coroutine will lead huge
> performance drop?
>
> It's because having a additional buffer before the network bottleneck (colo-
> compare in our case) confuses TCP's congestion-control:
> TCP will speed up the data transfer until packets start to drop (or the
> network interface is blocked). This feedback has to be quick so TCP can select
> a suitable transfer speed. But with the patch, the guest will fill the buffer
> as
> fast as it can (it does not "see" the slow bandwidth of colo-compare behind
> the buffer) until it it hits against the TCP congestion window. At this point
> TCP
> drastically reduces its transfer speed and it stays low because the full
> buffer
> delays the packets so it doesn't receive ACK's so it can't speed up the
> transfer again. Until the buffer is empty again (can take up to a second in my
> tests). Then this cycle repeats.
Make sense!
After fix above issue:
Reviewed-by: Zhang Chen
Thanks
Zhang Chen
>
> Regards,
> Lukas Straub
>
> > Thanks
> > Zhang Chen
> >
> > >
> > > Regards,
> > > Lukas Straub
> > >
> > > ---
> > > net/filter-mirror.c | 142
> > > +---
> > > 1 file changed, 106 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > > d83e815545..6bcd317502 100644
> > > --- a/net/filter-mirror.c
> > > +++ b/net/filter-mirror.c
> > > @@ -20,6 +20,8 @@
> > > #include "chardev/char-fe.h"
> > > #include "qemu/iov.h"
> > > #include "qemu/sockets.h"
> > > +#include "block/aio-wait.h"
> > > +#include "qemu/coroutine.h"
> > >
> > > #define FILTER_MIRROR(obj) \
> > > OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR) @@ -31,6
> > > +33,18 @@ #define TYPE_FILTER_REDIRECTOR "filter-redirector"
> > > #define REDIRECTOR_MAX_LEN NET_BUFSIZE
> > >
> > > +typedef struct SendCo {
> > > +Coroutine *co;
> > > +GQueue send_list;
> > > +
Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send
On Fri, 8 May 2020 06:28:45 +
"Zhang, Chen" wrote:
> > -Original Message-
> > From: Lukas Straub
> > Sent: Friday, May 8, 2020 2:08 PM
> > To: Zhang, Chen
> > Cc: qemu-devel ; Li Zhijian
> > ; Jason Wang ; Marc-
> > André Lureau ; Paolo Bonzini
> >
> > Subject: Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> > compare_chr_send
> >
> > On Fri, 8 May 2020 02:19:00 +
> > "Zhang, Chen" wrote:
> > > > > No need to init the notify_sendco each time, because the notify
> > > > > dev just
> > > > an optional parameter.
> > > > > You can use the if (s->notify_dev) here. Just Xen use the
> > chr_notify_dev.
> > > >
> > > > Ok, I will change that and the code below in the next version.
> > > >
> > > > > Overall, make the chr_send job to coroutine is a good idea. It
> > > > > looks good
> > > > for me.
> > > > > And your patch inspired me, it looks we can re-use the
> > > > > compare_chr_send
> > > > code on filter mirror/redirector too.
> > > >
> > > > I already have patch for that, but I don't think it is a good idea,
> > > > because the guest then can send packets faster than colo-compare can
> > > > process. This leads bufferbloat and the performance drops in my tests:
> > > > Client-to-server tcp:
> > > > without patch: ~66 Mbit/s
> > > > with patch: ~59 Mbit/s
> > > > Server-to-client tcp:
> > > > without patch: ~702 Kbit/s
> > > > with patch: ~328 Kbit/s
> > >
> > > Oh, a big performance drop, is that caused by memcpy/zero_copy parts ?
> > >
> > > Thanks
> > > Zhang Chen
> >
> > No, there is no memcpy overhead with this patch, see below.
>
> I means for the filter mirror/redirector parts why coroutine will lead huge
> performance drop?
It's because having a additional buffer before the network bottleneck
(colo-compare in our case) confuses TCP's congestion-control:
TCP will speed up the data transfer until packets start to drop (or the network
interface is blocked). This feedback has to be quick so TCP can select a
suitable transfer speed. But with the patch, the guest will fill the buffer as
fast as it can (it does not "see" the slow bandwidth of colo-compare behind the
buffer) until it it hits against the TCP congestion window. At this point TCP
drastically reduces its transfer speed and it stays low because the full buffer
delays the packets so it doesn't receive ACK's so it can't speed up the
transfer again. Until the buffer is empty again (can take up to a second in my
tests). Then this cycle repeats.
Regards,
Lukas Straub
> Thanks
> Zhang Chen
>
> >
> > Regards,
> > Lukas Straub
> >
> > ---
> > net/filter-mirror.c | 142 +---
> > 1 file changed, 106 insertions(+), 36 deletions(-)
> >
> > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > d83e815545..6bcd317502 100644
> > --- a/net/filter-mirror.c
> > +++ b/net/filter-mirror.c
> > @@ -20,6 +20,8 @@
> > #include "chardev/char-fe.h"
> > #include "qemu/iov.h"
> > #include "qemu/sockets.h"
> > +#include "block/aio-wait.h"
> > +#include "qemu/coroutine.h"
> >
> > #define FILTER_MIRROR(obj) \
> > OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR) @@ -31,6
> > +33,18 @@ #define TYPE_FILTER_REDIRECTOR "filter-redirector"
> > #define REDIRECTOR_MAX_LEN NET_BUFSIZE
> >
> > +typedef struct SendCo {
> > +Coroutine *co;
> > +GQueue send_list;
> > +bool done;
> > +int ret;
> > +} SendCo;
> > +
> > +typedef struct SendEntry {
> > +ssize_t size;
> > +uint8_t buf[];
> > +} SendEntry;
> > +
> > typedef struct MirrorState {
> > NetFilterState parent_obj;
> > char *indev;
> > @@ -38,59 +52,101 @@ typedef struct MirrorState {
> > CharBackend chr_in;
> > CharBackend chr_out;
> > SocketReadState rs;
> > +SendCo sendco;
> > bool vnet_hdr;
> > } MirrorState;
> >
> > -static int filter_send(MirrorState *s,
> > - const struct iovec *iov,
> > - int iovcnt)
> > +static void coroutine_fn _filter_send(void *opaque)
> > {
> > +MirrorState *s = opaque;
> > +SendCo *sendco =
RE: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send
> -Original Message-
> From: Lukas Straub
> Sent: Friday, May 8, 2020 2:08 PM
> To: Zhang, Chen
> Cc: qemu-devel ; Li Zhijian
> ; Jason Wang ; Marc-
> André Lureau ; Paolo Bonzini
>
> Subject: Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> compare_chr_send
>
> On Fri, 8 May 2020 02:19:00 +
> "Zhang, Chen" wrote:
> > > > No need to init the notify_sendco each time, because the notify
> > > > dev just
> > > an optional parameter.
> > > > You can use the if (s->notify_dev) here. Just Xen use the
> chr_notify_dev.
> > >
> > > Ok, I will change that and the code below in the next version.
> > >
> > > > Overall, make the chr_send job to coroutine is a good idea. It
> > > > looks good
> > > for me.
> > > > And your patch inspired me, it looks we can re-use the
> > > > compare_chr_send
> > > code on filter mirror/redirector too.
> > >
> > > I already have patch for that, but I don't think it is a good idea,
> > > because the guest then can send packets faster than colo-compare can
> > > process. This leads bufferbloat and the performance drops in my tests:
> > > Client-to-server tcp:
> > > without patch: ~66 Mbit/s
> > > with patch: ~59 Mbit/s
> > > Server-to-client tcp:
> > > without patch: ~702 Kbit/s
> > > with patch: ~328 Kbit/s
> >
> > Oh, a big performance drop, is that caused by memcpy/zero_copy parts ?
> >
> > Thanks
> > Zhang Chen
>
> No, there is no memcpy overhead with this patch, see below.
I means for the filter mirror/redirector parts why coroutine will lead huge
performance drop?
Thanks
Zhang Chen
>
> Regards,
> Lukas Straub
>
> ---
> net/filter-mirror.c | 142 +---
> 1 file changed, 106 insertions(+), 36 deletions(-)
>
> diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> d83e815545..6bcd317502 100644
> --- a/net/filter-mirror.c
> +++ b/net/filter-mirror.c
> @@ -20,6 +20,8 @@
> #include "chardev/char-fe.h"
> #include "qemu/iov.h"
> #include "qemu/sockets.h"
> +#include "block/aio-wait.h"
> +#include "qemu/coroutine.h"
>
> #define FILTER_MIRROR(obj) \
> OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR) @@ -31,6
> +33,18 @@ #define TYPE_FILTER_REDIRECTOR "filter-redirector"
> #define REDIRECTOR_MAX_LEN NET_BUFSIZE
>
> +typedef struct SendCo {
> +Coroutine *co;
> +GQueue send_list;
> +bool done;
> +int ret;
> +} SendCo;
> +
> +typedef struct SendEntry {
> +ssize_t size;
> +uint8_t buf[];
> +} SendEntry;
> +
> typedef struct MirrorState {
> NetFilterState parent_obj;
> char *indev;
> @@ -38,59 +52,101 @@ typedef struct MirrorState {
> CharBackend chr_in;
> CharBackend chr_out;
> SocketReadState rs;
> +SendCo sendco;
> bool vnet_hdr;
> } MirrorState;
>
> -static int filter_send(MirrorState *s,
> - const struct iovec *iov,
> - int iovcnt)
> +static void coroutine_fn _filter_send(void *opaque)
> {
> +MirrorState *s = opaque;
> +SendCo *sendco = >sendco;
> NetFilterState *nf = NETFILTER(s);
> int ret = 0;
> -ssize_t size = 0;
> -uint32_t len = 0;
> -char *buf;
> -
> -size = iov_size(iov, iovcnt);
> -if (!size) {
> -return 0;
> -}
>
> -len = htonl(size);
> -ret = qemu_chr_fe_write_all(>chr_out, (uint8_t *), sizeof(len));
> -if (ret != sizeof(len)) {
> -goto err;
> -}
> +while (!g_queue_is_empty(>send_list)) {
> +SendEntry *entry = g_queue_pop_tail(>send_list);
> +uint32_t len = htonl(entry->size);
>
> -if (s->vnet_hdr) {
> -/*
> - * If vnet_hdr = on, we send vnet header len to make other
> - * module(like colo-compare) know how to parse net
> - * packet correctly.
> - */
> -ssize_t vnet_hdr_len;
> +ret = qemu_chr_fe_write_all(>chr_out,
> +(uint8_t *),
> +sizeof(len));
> +if (ret != sizeof(len)) {
> +g_free(entry);
> +goto err;
> +}
>
> -vnet_hdr_len = nf->netdev->vnet_hdr_len;
> +if (s->vnet_hdr) {
> +/*
> + * If vnet_hdr = on, we send vnet header len to make other
> + * module(like colo-compare) know
Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send
On Fri, 8 May 2020 02:19:00 +
"Zhang, Chen" wrote:
> > > No need to init the notify_sendco each time, because the notify dev just
> > an optional parameter.
> > > You can use the if (s->notify_dev) here. Just Xen use the chr_notify_dev.
> > >
> >
> > Ok, I will change that and the code below in the next version.
> >
> > > Overall, make the chr_send job to coroutine is a good idea. It looks good
> > >
> > for me.
> > > And your patch inspired me, it looks we can re-use the compare_chr_send
> > code on filter mirror/redirector too.
> >
> > I already have patch for that, but I don't think it is a good idea, because
> > the
> > guest then can send packets faster than colo-compare can process. This leads
> > bufferbloat and the performance drops in my tests:
> > Client-to-server tcp:
> > without patch: ~66 Mbit/s
> > with patch: ~59 Mbit/s
> > Server-to-client tcp:
> > without patch: ~702 Kbit/s
> > with patch: ~328 Kbit/s
>
> Oh, a big performance drop, is that caused by memcpy/zero_copy parts ?
>
> Thanks
> Zhang Chen
No, there is no memcpy overhead with this patch, see below.
Regards,
Lukas Straub
---
net/filter-mirror.c | 142 +---
1 file changed, 106 insertions(+), 36 deletions(-)
diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index d83e815545..6bcd317502 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -20,6 +20,8 @@
#include "chardev/char-fe.h"
#include "qemu/iov.h"
#include "qemu/sockets.h"
+#include "block/aio-wait.h"
+#include "qemu/coroutine.h"
#define FILTER_MIRROR(obj) \
OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR)
@@ -31,6 +33,18 @@
#define TYPE_FILTER_REDIRECTOR "filter-redirector"
#define REDIRECTOR_MAX_LEN NET_BUFSIZE
+typedef struct SendCo {
+Coroutine *co;
+GQueue send_list;
+bool done;
+int ret;
+} SendCo;
+
+typedef struct SendEntry {
+ssize_t size;
+uint8_t buf[];
+} SendEntry;
+
typedef struct MirrorState {
NetFilterState parent_obj;
char *indev;
@@ -38,59 +52,101 @@ typedef struct MirrorState {
CharBackend chr_in;
CharBackend chr_out;
SocketReadState rs;
+SendCo sendco;
bool vnet_hdr;
} MirrorState;
-static int filter_send(MirrorState *s,
- const struct iovec *iov,
- int iovcnt)
+static void coroutine_fn _filter_send(void *opaque)
{
+MirrorState *s = opaque;
+SendCo *sendco = >sendco;
NetFilterState *nf = NETFILTER(s);
int ret = 0;
-ssize_t size = 0;
-uint32_t len = 0;
-char *buf;
-
-size = iov_size(iov, iovcnt);
-if (!size) {
-return 0;
-}
-len = htonl(size);
-ret = qemu_chr_fe_write_all(>chr_out, (uint8_t *), sizeof(len));
-if (ret != sizeof(len)) {
-goto err;
-}
+while (!g_queue_is_empty(>send_list)) {
+SendEntry *entry = g_queue_pop_tail(>send_list);
+uint32_t len = htonl(entry->size);
-if (s->vnet_hdr) {
-/*
- * If vnet_hdr = on, we send vnet header len to make other
- * module(like colo-compare) know how to parse net
- * packet correctly.
- */
-ssize_t vnet_hdr_len;
+ret = qemu_chr_fe_write_all(>chr_out,
+(uint8_t *),
+sizeof(len));
+if (ret != sizeof(len)) {
+g_free(entry);
+goto err;
+}
-vnet_hdr_len = nf->netdev->vnet_hdr_len;
+if (s->vnet_hdr) {
+/*
+ * If vnet_hdr = on, we send vnet header len to make other
+ * module(like colo-compare) know how to parse net
+ * packet correctly.
+ */
+
+len = htonl(nf->netdev->vnet_hdr_len);
+ret = qemu_chr_fe_write_all(>chr_out,
+(uint8_t *),
+sizeof(len));
+if (ret != sizeof(len)) {
+g_free(entry);
+goto err;
+}
+}
-len = htonl(vnet_hdr_len);
-ret = qemu_chr_fe_write_all(>chr_out, (uint8_t *), sizeof(len));
-if (ret != sizeof(len)) {
+ret = qemu_chr_fe_write_all(>chr_out,
+(uint8_t *)entry->buf,
+entry->size);
+if (ret != entry->size) {
+g_free(entry);
goto err;
}
-}
-buf = g_malloc(size);
-iov_to_buf(iov, iovcnt, 0, buf, size);
-ret = qemu_chr_fe_write_all(>chr_out, (uint8_t *)buf, size);
-g_free(buf);
-if (ret != size) {
-goto err;
+g_free(entry);
}
-return 0;
+sendco->ret = 0;
+goto out;
err:
-return ret < 0 ? ret : -EIO;
+while (!g_queue_is_empty(>send_list)) {
+SendEntry *entry = g_queue_pop_tail(>send_list);
+g_free(entry);
+}
+sendco->ret = ret < 0 ? ret : -EIO;
RE: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send
> -Original Message-
> From: Lukas Straub
> Sent: Thursday, May 7, 2020 11:51 PM
> To: Zhang, Chen
> Cc: qemu-devel ; Li Zhijian
> ; Jason Wang ; Marc-
> André Lureau ; Paolo Bonzini
>
> Subject: Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> compare_chr_send
>
> On Thu, 7 May 2020 11:00:26 +
> "Zhang, Chen" wrote:
>
> > > -Original Message-
> > > From: Lukas Straub
> > > Sent: Monday, May 4, 2020 6:28 PM
> > > To: qemu-devel
> > > Cc: Zhang, Chen ; Li Zhijian
> > > ; Jason Wang ; Marc-
> > > André Lureau ; Paolo Bonzini
> > >
> > > Subject: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> > > compare_chr_send
> > >
> > > The chr_out chardev is connected to a filter-redirector running in
> > > the main loop. qemu_chr_fe_write_all might block here in
> > > compare_chr_send if the (socket-)buffer is full.
> > > If another filter-redirector in the main loop want's to send data to
> > > chr_pri_in it might also block if the buffer is full. This leads to
> > > a deadlock because both event loops get blocked.
> > >
> > > Fix this by converting compare_chr_send to a coroutine and putting
> > > the packets in a send queue.
> > >
> > > Signed-off-by: Lukas Straub
> > > ---
> > > net/colo-compare.c | 187 ++---
>
> > >
> > > net/colo.c | 7 ++
> > > net/colo.h | 1 +
> > > 3 files changed, 150 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > 1de4220fe2..2a4e7f7c4e 100644
> > > --- a/net/colo-compare.c
> > > +++ b/net/colo-compare.c
> > > @@ -32,6 +32,9 @@
> > > #include "migration/migration.h"
> > > #include "util.h"
> > >
> > > +#include "block/aio-wait.h"
> > > +#include "qemu/coroutine.h"
> > > +
> > > #define TYPE_COLO_COMPARE "colo-compare"
> > > #define COLO_COMPARE(obj) \
> > > OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE) @@ -
> 77,6
> > > +80,23 @@ static int event_unhandled_count;
> > > *|packet | |packet +|packet | |packet +
> > > *++ ++++ ++
> > > */
> > > +
> > > +typedef struct SendCo {
> > > +Coroutine *co;
> > > +struct CompareState *s;
> > > +CharBackend *chr;
> > > +GQueue send_list;
> > > +bool notify_remote_frame;
> > > +bool done;
> > > +int ret;
> > > +} SendCo;
> > > +
> > > +typedef struct SendEntry {
> > > +uint32_t size;
> > > +uint32_t vnet_hdr_len;
> > > +uint8_t *buf;
> > > +} SendEntry;
> > > +
> > > typedef struct CompareState {
> > > Object parent;
> > >
> > > @@ -91,6 +111,8 @@ typedef struct CompareState {
> > > SocketReadState pri_rs;
> > > SocketReadState sec_rs;
> > > SocketReadState notify_rs;
> > > +SendCo out_sendco;
> > > +SendCo notify_sendco;
> > > bool vnet_hdr;
> > > uint32_t compare_timeout;
> > > uint32_t expired_scan_cycle;
> > > @@ -124,10 +146,11 @@ enum {
> > >
> > >
> > > static int compare_chr_send(CompareState *s,
> > > -const uint8_t *buf,
> > > +uint8_t *buf,
> > > uint32_t size,
> > > uint32_t vnet_hdr_len,
> > > -bool notify_remote_frame);
> > > +bool notify_remote_frame,
> > > +bool zero_copy);
> > >
> > > static bool packet_matches_str(const char *str,
> > > const uint8_t *buf, @@ -145,7 +168,7
> > > @@ static void notify_remote_frame(CompareState *s)
> > > char msg[] = "DO_CHECKPOINT";
> > > int ret = 0;
> > >
> > > -ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
> > > +ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true,
> > > + false);
> > > if (ret < 0) {
> > > error_report("Notify Xen COLO-frame
Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send
On Thu, 7 May 2020 11:00:26 +
"Zhang, Chen" wrote:
> > -Original Message-
> > From: Lukas Straub
> > Sent: Monday, May 4, 2020 6:28 PM
> > To: qemu-devel
> > Cc: Zhang, Chen ; Li Zhijian
> > ; Jason Wang ; Marc-
> > André Lureau ; Paolo Bonzini
> >
> > Subject: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> > compare_chr_send
> >
> > The chr_out chardev is connected to a filter-redirector running in the
> > main loop. qemu_chr_fe_write_all might block here in compare_chr_send
> > if the (socket-)buffer is full.
> > If another filter-redirector in the main loop want's to send data to
> > chr_pri_in it might also block if the buffer is full. This leads to a
> > deadlock because both event loops get blocked.
> >
> > Fix this by converting compare_chr_send to a coroutine and putting the
> > packets in a send queue.
> >
> > Signed-off-by: Lukas Straub
> > ---
> > net/colo-compare.c | 187 ++---
> >
> > net/colo.c | 7 ++
> > net/colo.h | 1 +
> > 3 files changed, 150 insertions(+), 45 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > 1de4220fe2..2a4e7f7c4e 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -32,6 +32,9 @@
> > #include "migration/migration.h"
> > #include "util.h"
> >
> > +#include "block/aio-wait.h"
> > +#include "qemu/coroutine.h"
> > +
> > #define TYPE_COLO_COMPARE "colo-compare"
> > #define COLO_COMPARE(obj) \
> > OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE) @@ -77,6
> > +80,23 @@ static int event_unhandled_count;
> > *|packet | |packet +|packet | |packet +
> > *++ ++++ ++
> > */
> > +
> > +typedef struct SendCo {
> > +Coroutine *co;
> > +struct CompareState *s;
> > +CharBackend *chr;
> > +GQueue send_list;
> > +bool notify_remote_frame;
> > +bool done;
> > +int ret;
> > +} SendCo;
> > +
> > +typedef struct SendEntry {
> > +uint32_t size;
> > +uint32_t vnet_hdr_len;
> > +uint8_t *buf;
> > +} SendEntry;
> > +
> > typedef struct CompareState {
> > Object parent;
> >
> > @@ -91,6 +111,8 @@ typedef struct CompareState {
> > SocketReadState pri_rs;
> > SocketReadState sec_rs;
> > SocketReadState notify_rs;
> > +SendCo out_sendco;
> > +SendCo notify_sendco;
> > bool vnet_hdr;
> > uint32_t compare_timeout;
> > uint32_t expired_scan_cycle;
> > @@ -124,10 +146,11 @@ enum {
> >
> >
> > static int compare_chr_send(CompareState *s,
> > -const uint8_t *buf,
> > +uint8_t *buf,
> > uint32_t size,
> > uint32_t vnet_hdr_len,
> > -bool notify_remote_frame);
> > +bool notify_remote_frame,
> > +bool zero_copy);
> >
> > static bool packet_matches_str(const char *str,
> > const uint8_t *buf, @@ -145,7 +168,7
> > @@ static void notify_remote_frame(CompareState *s)
> > char msg[] = "DO_CHECKPOINT";
> > int ret = 0;
> >
> > -ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
> > +ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true,
> > + false);
> > if (ret < 0) {
> > error_report("Notify Xen COLO-frame failed");
> > }
> > @@ -272,12 +295,13 @@ static void
> > colo_release_primary_pkt(CompareState *s, Packet *pkt)
> > pkt->data,
> > pkt->size,
> > pkt->vnet_hdr_len,
> > - false);
> > + false,
> > + true);
> > if (ret < 0) {
> > error_report("colo send primary packet failed");
> > }
> > trace_colo_compare_main("packet same and release packet");
> > -packet_destroy(pkt, NULL);
> > +packet_destroy_partial(pkt, NULL);
> > }
> >
> > /*
> > @@ -699,65 +723,115 @@ static void colo_compare_connection(void
> > *opaque, void *user_data)
> > }
> > }
> >
> > -static int compare_chr_send(CompareState *s,
> > -const uint8_t *buf,
> > -uint32_t size,
> > -uint32_t vnet_hdr_len,
> > -bool notify_remote_frame)
> > +static void coroutine_fn _compare_chr_send(void *opaque)
> > {
> > +SendCo *sendco = opaque;
> > +CompareState *s = sendco->s;
> > int ret = 0;
> > -uint32_t len = htonl(size);
> >
> > -if (!size) {
> > -return 0;
> > -}
> > +while (!g_queue_is_empty(>send_list)) {
> > +SendEntry *entry = g_queue_pop_tail(>send_list);
> > +uint32_t len = htonl(entry->size);
> >
> > -if (notify_remote_frame) {
> > -ret =
RE: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send
> -Original Message-
> From: Lukas Straub
> Sent: Monday, May 4, 2020 6:28 PM
> To: qemu-devel
> Cc: Zhang, Chen ; Li Zhijian
> ; Jason Wang ; Marc-
> André Lureau ; Paolo Bonzini
>
> Subject: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> compare_chr_send
>
> The chr_out chardev is connected to a filter-redirector running in the
> main loop. qemu_chr_fe_write_all might block here in compare_chr_send
> if the (socket-)buffer is full.
> If another filter-redirector in the main loop want's to send data to
> chr_pri_in it might also block if the buffer is full. This leads to a
> deadlock because both event loops get blocked.
>
> Fix this by converting compare_chr_send to a coroutine and putting the
> packets in a send queue.
>
> Signed-off-by: Lukas Straub
> ---
> net/colo-compare.c | 187 ++---
>
> net/colo.c | 7 ++
> net/colo.h | 1 +
> 3 files changed, 150 insertions(+), 45 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 1de4220fe2..2a4e7f7c4e 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -32,6 +32,9 @@
> #include "migration/migration.h"
> #include "util.h"
>
> +#include "block/aio-wait.h"
> +#include "qemu/coroutine.h"
> +
> #define TYPE_COLO_COMPARE "colo-compare"
> #define COLO_COMPARE(obj) \
> OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE) @@ -77,6
> +80,23 @@ static int event_unhandled_count;
> *|packet | |packet +|packet | |packet +
> *++ ++++ ++
> */
> +
> +typedef struct SendCo {
> +Coroutine *co;
> +struct CompareState *s;
> +CharBackend *chr;
> +GQueue send_list;
> +bool notify_remote_frame;
> +bool done;
> +int ret;
> +} SendCo;
> +
> +typedef struct SendEntry {
> +uint32_t size;
> +uint32_t vnet_hdr_len;
> +uint8_t *buf;
> +} SendEntry;
> +
> typedef struct CompareState {
> Object parent;
>
> @@ -91,6 +111,8 @@ typedef struct CompareState {
> SocketReadState pri_rs;
> SocketReadState sec_rs;
> SocketReadState notify_rs;
> +SendCo out_sendco;
> +SendCo notify_sendco;
> bool vnet_hdr;
> uint32_t compare_timeout;
> uint32_t expired_scan_cycle;
> @@ -124,10 +146,11 @@ enum {
>
>
> static int compare_chr_send(CompareState *s,
> -const uint8_t *buf,
> +uint8_t *buf,
> uint32_t size,
> uint32_t vnet_hdr_len,
> -bool notify_remote_frame);
> +bool notify_remote_frame,
> +bool zero_copy);
>
> static bool packet_matches_str(const char *str,
> const uint8_t *buf, @@ -145,7 +168,7
> @@ static void notify_remote_frame(CompareState *s)
> char msg[] = "DO_CHECKPOINT";
> int ret = 0;
>
> -ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
> +ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true,
> + false);
> if (ret < 0) {
> error_report("Notify Xen COLO-frame failed");
> }
> @@ -272,12 +295,13 @@ static void
> colo_release_primary_pkt(CompareState *s, Packet *pkt)
> pkt->data,
> pkt->size,
> pkt->vnet_hdr_len,
> - false);
> + false,
> + true);
> if (ret < 0) {
> error_report("colo send primary packet failed");
> }
> trace_colo_compare_main("packet same and release packet");
> -packet_destroy(pkt, NULL);
> +packet_destroy_partial(pkt, NULL);
> }
>
> /*
> @@ -699,65 +723,115 @@ static void colo_compare_connection(void
> *opaque, void *user_data)
> }
> }
>
> -static int compare_chr_send(CompareState *s,
> -const uint8_t *buf,
> -uint32_t size,
> -uint32_t vnet_hdr_len,
> -bool notify_remote_frame)
> +static void coroutine_fn _compare_chr_send(void *opaque)
> {
> +SendCo *sendco = opaque;
> +CompareState *s = sendco->s;
> int ret = 0;
> -uint32_t len = htonl(size);
>
> -if (!size) {
> -return 0;
> -}
> +while (!g_queue_is_empty(>send_list)) {
> +SendEntry *entry = g_queue_pop_tail(>send_list);
> +uint32_t len = htonl(entry->size);
>
> -if (notify_remote_frame) {
> -ret = qemu_chr_fe_write_all(>chr_notify_dev,
> -(uint8_t *),
> -sizeof(len));
> -} else {
> -ret = qemu_chr_fe_write_all(>chr_out, (uint8_t *),
> sizeof(len));
> -}
> +ret = qemu_chr_fe_write_all(sendco->chr, (uint8_t *),
> + sizeof(len));
>
> -
