* [PATCH v2 0/1] COLO: Fix memory leak in packet_enqueue() @ 2020-03-22 17:47 Derek Su 2020-03-22 17:47 ` [PATCH v2 1/1] net/colo-compare.c: " Derek Su 0 siblings, 1 reply; 8+ messages in thread From: Derek Su @ 2020-03-22 17:47 UTC (permalink / raw) To: qemu-devel; +Cc: dereksu, chen.zhang, jasowang, lizhijian The patch is to fix the memory leak in packet_enqueue(). The allocated "pkt" needs to be freed if the colo compare primary or secondary queue is too big to insert. Reproduce steps: (1) Setup PVM and SVM both with NIC e1000 by the steps descripted in the wiki qemu/COLO (2) Run "iperf3 -s" in PVM (3) Run "iperfs -c <PVM-IP> -t 7200" The memory usage of qemu-system-x86_64 increases as the PVM's QMP shows "qemu-system-x86_64: colo compare secondary queue size too big, drop packet". Please review, thanks. V2: - Fix incorrect patch format Derek Su (1): net/colo-compare.c: Fix memory leak in packet_enqueue() net/colo-compare.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in packet_enqueue() 2020-03-22 17:47 [PATCH v2 0/1] COLO: Fix memory leak in packet_enqueue() Derek Su @ 2020-03-22 17:47 ` Derek Su 2020-03-23 19:24 ` Zhang, Chen 0 siblings, 1 reply; 8+ messages in thread From: Derek Su @ 2020-03-22 17:47 UTC (permalink / raw) To: qemu-devel; +Cc: dereksu, chen.zhang, jasowang, lizhijian The patch is to fix the "pkt" memory leak in packet_enqueue(). The allocated "pkt" needs to be freed if the colo compare primary or secondary queue is too big. Signed-off-by: Derek Su <dereksu@qnap.com> --- net/colo-compare.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/net/colo-compare.c b/net/colo-compare.c index 7ee17f2cf8..cdd87b2aa8 100644 --- a/net/colo-compare.c +++ b/net/colo-compare.c @@ -120,6 +120,10 @@ enum { SECONDARY_IN, }; +static const char *colo_mode[] = { + [PRIMARY_IN] = "primary", + [SECONDARY_IN] = "secondary", +}; static int compare_chr_send(CompareState *s, const uint8_t *buf, @@ -215,6 +219,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con) ConnectionKey key; Packet *pkt = NULL; Connection *conn; + int ret; if (mode == PRIMARY_IN) { pkt = packet_new(s->pri_rs.buf, @@ -243,16 +248,18 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con) } if (mode == PRIMARY_IN) { - if (!colo_insert_packet(&conn->primary_list, pkt, &conn->pack)) { - error_report("colo compare primary queue size too big," - "drop packet"); - } + ret = colo_insert_packet(&conn->primary_list, pkt, &conn->pack); } else { - if (!colo_insert_packet(&conn->secondary_list, pkt, &conn->sack)) { - error_report("colo compare secondary queue size too big," - "drop packet"); - } + ret = colo_insert_packet(&conn->secondary_list, pkt, &conn->sack); } + + if (!ret) { + error_report("colo compare %s queue size too big," + "drop packet", colo_mode[mode]); + packet_destroy(pkt, NULL); + pkt = NULL; + } + *con = conn; return 0; -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in packet_enqueue() 2020-03-22 17:47 ` [PATCH v2 1/1] net/colo-compare.c: " Derek Su @ 2020-03-23 19:24 ` Zhang, Chen 2020-03-24 2:46 ` Jing-Wei Su 0 siblings, 1 reply; 8+ messages in thread From: Zhang, Chen @ 2020-03-23 19:24 UTC (permalink / raw) To: Derek Su, qemu-devel; +Cc: dereksu, jasowang, lizhijian > -----Original Message----- > From: Derek Su <jwsu1986@gmail.com> > Sent: Monday, March 23, 2020 1:48 AM > To: qemu-devel@nongnu.org > Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; > jasowang@redhat.com; dereksu@qnap.com > Subject: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > packet_enqueue() > > The patch is to fix the "pkt" memory leak in packet_enqueue(). > The allocated "pkt" needs to be freed if the colo compare primary or > secondary queue is too big. Hi Derek, Thank you for the patch. I re-think this issue in a big view, looks just free the pkg is not enough in this situation. The root cause is network is too busy to compare, So, better choice is notify COLO frame to do a checkpoint and clean up all the network queue. This work maybe decrease COLO network performance but seams better than drop lots of pkg. Thanks Zhang Chen > > Signed-off-by: Derek Su <dereksu@qnap.com> > --- > net/colo-compare.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > 7ee17f2cf8..cdd87b2aa8 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -120,6 +120,10 @@ enum { > SECONDARY_IN, > }; > > +static const char *colo_mode[] = { > + [PRIMARY_IN] = "primary", > + [SECONDARY_IN] = "secondary", > +}; > > static int compare_chr_send(CompareState *s, > const uint8_t *buf, @@ -215,6 +219,7 @@ static int > packet_enqueue(CompareState *s, int mode, Connection **con) > ConnectionKey key; > Packet *pkt = NULL; > Connection *conn; > + int ret; > > if (mode == PRIMARY_IN) { > pkt = packet_new(s->pri_rs.buf, @@ -243,16 +248,18 @@ static int > packet_enqueue(CompareState *s, int mode, Connection **con) > } > > if (mode == PRIMARY_IN) { > - if (!colo_insert_packet(&conn->primary_list, pkt, &conn->pack)) { > - error_report("colo compare primary queue size too big," > - "drop packet"); > - } > + ret = colo_insert_packet(&conn->primary_list, pkt, > + &conn->pack); > } else { > - if (!colo_insert_packet(&conn->secondary_list, pkt, &conn->sack)) { > - error_report("colo compare secondary queue size too big," > - "drop packet"); > - } > + ret = colo_insert_packet(&conn->secondary_list, pkt, > + &conn->sack); > } > + > + if (!ret) { > + error_report("colo compare %s queue size too big," > + "drop packet", colo_mode[mode]); > + packet_destroy(pkt, NULL); > + pkt = NULL; > + } > + > *con = conn; > > return 0; > -- > 2.17.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in packet_enqueue() 2020-03-23 19:24 ` Zhang, Chen @ 2020-03-24 2:46 ` Jing-Wei Su 2020-03-25 1:37 ` Zhang, Chen 0 siblings, 1 reply; 8+ messages in thread From: Jing-Wei Su @ 2020-03-24 2:46 UTC (permalink / raw) To: Zhang, Chen; +Cc: dereksu, jasowang, qemu-devel, lizhijian Zhang, Chen <chen.zhang@intel.com> 於 2020年3月24日 週二 上午3:24寫道: > > > > > -----Original Message----- > > From: Derek Su <jwsu1986@gmail.com> > > Sent: Monday, March 23, 2020 1:48 AM > > To: qemu-devel@nongnu.org > > Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; > > jasowang@redhat.com; dereksu@qnap.com > > Subject: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > > packet_enqueue() > > > > The patch is to fix the "pkt" memory leak in packet_enqueue(). > > The allocated "pkt" needs to be freed if the colo compare primary or > > secondary queue is too big. > > Hi Derek, > > Thank you for the patch. > I re-think this issue in a big view, looks just free the pkg is not enough in this situation. > The root cause is network is too busy to compare, So, better choice is notify COLO frame > to do a checkpoint and clean up all the network queue. This work maybe decrease > COLO network performance but seams better than drop lots of pkg. > > Thanks > Zhang Chen > Hello, Zhang Got it. What is the concern of the massive "drop packets"? Does the behavior make the COLO do checkpoint periodically (~20 seconds) instead of doing immediate checkpoint when encountering different response packets? It seems that frequent checkpoints caused by the full queue (busy network) instead of different response packets may harm the high speed network (10 Gbps or higher) performance dramatically. Thanks Derek > > > > Signed-off-by: Derek Su <dereksu@qnap.com> > > --- > > net/colo-compare.c | 23 +++++++++++++++-------- > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > 7ee17f2cf8..cdd87b2aa8 100644 > > --- a/net/colo-compare.c > > +++ b/net/colo-compare.c > > @@ -120,6 +120,10 @@ enum { > > SECONDARY_IN, > > }; > > > > +static const char *colo_mode[] = { > > + [PRIMARY_IN] = "primary", > > + [SECONDARY_IN] = "secondary", > > +}; > > > > static int compare_chr_send(CompareState *s, > > const uint8_t *buf, @@ -215,6 +219,7 @@ static int > > packet_enqueue(CompareState *s, int mode, Connection **con) > > ConnectionKey key; > > Packet *pkt = NULL; > > Connection *conn; > > + int ret; > > > > if (mode == PRIMARY_IN) { > > pkt = packet_new(s->pri_rs.buf, @@ -243,16 +248,18 @@ static int > > packet_enqueue(CompareState *s, int mode, Connection **con) > > } > > > > if (mode == PRIMARY_IN) { > > - if (!colo_insert_packet(&conn->primary_list, pkt, &conn->pack)) { > > - error_report("colo compare primary queue size too big," > > - "drop packet"); > > - } > > + ret = colo_insert_packet(&conn->primary_list, pkt, > > + &conn->pack); > > } else { > > - if (!colo_insert_packet(&conn->secondary_list, pkt, &conn->sack)) { > > - error_report("colo compare secondary queue size too big," > > - "drop packet"); > > - } > > + ret = colo_insert_packet(&conn->secondary_list, pkt, > > + &conn->sack); > > } > > + > > + if (!ret) { > > + error_report("colo compare %s queue size too big," > > + "drop packet", colo_mode[mode]); > > + packet_destroy(pkt, NULL); > > + pkt = NULL; > > + } > > + > > *con = conn; > > > > return 0; > > -- > > 2.17.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in packet_enqueue() 2020-03-24 2:46 ` Jing-Wei Su @ 2020-03-25 1:37 ` Zhang, Chen 2020-03-25 2:05 ` Jing-Wei Su 0 siblings, 1 reply; 8+ messages in thread From: Zhang, Chen @ 2020-03-25 1:37 UTC (permalink / raw) To: Jing-Wei Su; +Cc: dereksu, jasowang, qemu-devel, lizhijian > -----Original Message----- > From: Jing-Wei Su <jwsu1986@gmail.com> > Sent: Tuesday, March 24, 2020 10:47 AM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; > jasowang@redhat.com; dereksu@qnap.com > Subject: Re: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > packet_enqueue() > > Zhang, Chen <chen.zhang@intel.com> 於 2020年3月24日 週二 上午3:24 > 寫道: > > > > > > > > > -----Original Message----- > > > From: Derek Su <jwsu1986@gmail.com> > > > Sent: Monday, March 23, 2020 1:48 AM > > > To: qemu-devel@nongnu.org > > > Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; > > > jasowang@redhat.com; dereksu@qnap.com > > > Subject: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > > > packet_enqueue() > > > > > > The patch is to fix the "pkt" memory leak in packet_enqueue(). > > > The allocated "pkt" needs to be freed if the colo compare primary or > > > secondary queue is too big. > > > > Hi Derek, > > > > Thank you for the patch. > > I re-think this issue in a big view, looks just free the pkg is not enough in > this situation. > > The root cause is network is too busy to compare, So, better choice is > > notify COLO frame to do a checkpoint and clean up all the network > > queue. This work maybe decrease COLO network performance but seams > better than drop lots of pkg. > > > > Thanks > > Zhang Chen > > > > Hello, Zhang > > Got it. > What is the concern of the massive "drop packets"? > Does the behavior make the COLO do checkpoint periodically (~20 seconds) > instead of doing immediate checkpoint when encountering different > response packets? The concern of the "drop packets" is guest will lose network connection with most of real clients until next periodic force checkpoint. COLO designed for dynamic control checkpoint, so I think do a checkpoint here will help guest supply service faster. > > It seems that frequent checkpoints caused by the full queue (busy > network) instead of different > response packets may harm the high speed network (10 Gbps or higher) > performance dramatically. Yes, maybe I can send a patch to make user adjust queue size depend on it's own environment. But with larger queue size, colo-compare will spend much time to do compare packet when network Is real busy status. Thanks Zhang Chen > > Thanks > Derek > > > > > > > Signed-off-by: Derek Su <dereksu@qnap.com> > > > --- > > > net/colo-compare.c | 23 +++++++++++++++-------- > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > > 7ee17f2cf8..cdd87b2aa8 100644 > > > --- a/net/colo-compare.c > > > +++ b/net/colo-compare.c > > > @@ -120,6 +120,10 @@ enum { > > > SECONDARY_IN, > > > }; > > > > > > +static const char *colo_mode[] = { > > > + [PRIMARY_IN] = "primary", > > > + [SECONDARY_IN] = "secondary", > > > +}; > > > > > > static int compare_chr_send(CompareState *s, > > > const uint8_t *buf, @@ -215,6 +219,7 @@ > > > static int packet_enqueue(CompareState *s, int mode, Connection > **con) > > > ConnectionKey key; > > > Packet *pkt = NULL; > > > Connection *conn; > > > + int ret; > > > > > > if (mode == PRIMARY_IN) { > > > pkt = packet_new(s->pri_rs.buf, @@ -243,16 +248,18 @@ > > > static int packet_enqueue(CompareState *s, int mode, Connection > **con) > > > } > > > > > > if (mode == PRIMARY_IN) { > > > - if (!colo_insert_packet(&conn->primary_list, pkt, &conn->pack)) { > > > - error_report("colo compare primary queue size too big," > > > - "drop packet"); > > > - } > > > + ret = colo_insert_packet(&conn->primary_list, pkt, > > > + &conn->pack); > > > } else { > > > - if (!colo_insert_packet(&conn->secondary_list, pkt, &conn->sack)) { > > > - error_report("colo compare secondary queue size too big," > > > - "drop packet"); > > > - } > > > + ret = colo_insert_packet(&conn->secondary_list, pkt, > > > + &conn->sack); > > > } > > > + > > > + if (!ret) { > > > + error_report("colo compare %s queue size too big," > > > + "drop packet", colo_mode[mode]); > > > + packet_destroy(pkt, NULL); > > > + pkt = NULL; > > > + } > > > + > > > *con = conn; > > > > > > return 0; > > > -- > > > 2.17.1 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in packet_enqueue() 2020-03-25 1:37 ` Zhang, Chen @ 2020-03-25 2:05 ` Jing-Wei Su 2020-03-25 4:16 ` Derek Su 0 siblings, 1 reply; 8+ messages in thread From: Jing-Wei Su @ 2020-03-25 2:05 UTC (permalink / raw) To: Zhang, Chen; +Cc: dereksu, jasowang, qemu-devel, lizhijian Zhang, Chen <chen.zhang@intel.com> 於 2020年3月25日 週三 上午9:37寫道: > > > > > -----Original Message----- > > From: Jing-Wei Su <jwsu1986@gmail.com> > > Sent: Tuesday, March 24, 2020 10:47 AM > > To: Zhang, Chen <chen.zhang@intel.com> > > Cc: qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; > > jasowang@redhat.com; dereksu@qnap.com > > Subject: Re: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > > packet_enqueue() > > > > Zhang, Chen <chen.zhang@intel.com> 於 2020年3月24日 週二 上午3:24 > > 寫道: > > > > > > > > > > > > > -----Original Message----- > > > > From: Derek Su <jwsu1986@gmail.com> > > > > Sent: Monday, March 23, 2020 1:48 AM > > > > To: qemu-devel@nongnu.org > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; > > > > jasowang@redhat.com; dereksu@qnap.com > > > > Subject: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > > > > packet_enqueue() > > > > > > > > The patch is to fix the "pkt" memory leak in packet_enqueue(). > > > > The allocated "pkt" needs to be freed if the colo compare primary or > > > > secondary queue is too big. > > > > > > Hi Derek, > > > > > > Thank you for the patch. > > > I re-think this issue in a big view, looks just free the pkg is not enough in > > this situation. > > > The root cause is network is too busy to compare, So, better choice is > > > notify COLO frame to do a checkpoint and clean up all the network > > > queue. This work maybe decrease COLO network performance but seams > > better than drop lots of pkg. > > > > > > Thanks > > > Zhang Chen > > > > > > > Hello, Zhang > > > > Got it. > > What is the concern of the massive "drop packets"? > > Does the behavior make the COLO do checkpoint periodically (~20 seconds) > > instead of doing immediate checkpoint when encountering different > > response packets? > > The concern of the "drop packets" is guest will lose network connection with > most of real clients until next periodic force checkpoint. COLO designed for dynamic > control checkpoint, so I think do a checkpoint here will help guest supply service faster. > I see. I'll update the patch with your suggestion later. > > > > It seems that frequent checkpoints caused by the full queue (busy > > network) instead of different > > response packets may harm the high speed network (10 Gbps or higher) > > performance dramatically. > > Yes, maybe I can send a patch to make user adjust queue size depend on it's own environment. > But with larger queue size, colo-compare will spend much time to do compare packet when network > Is real busy status. Thank you. The user-configurable queue size will be very helpful. Thanks. Derek Su > > Thanks > Zhang Chen > > > > > Thanks > > Derek > > > > > > > > > > Signed-off-by: Derek Su <dereksu@qnap.com> > > > > --- > > > > net/colo-compare.c | 23 +++++++++++++++-------- > > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > > > 7ee17f2cf8..cdd87b2aa8 100644 > > > > --- a/net/colo-compare.c > > > > +++ b/net/colo-compare.c > > > > @@ -120,6 +120,10 @@ enum { > > > > SECONDARY_IN, > > > > }; > > > > > > > > +static const char *colo_mode[] = { > > > > + [PRIMARY_IN] = "primary", > > > > + [SECONDARY_IN] = "secondary", > > > > +}; > > > > > > > > static int compare_chr_send(CompareState *s, > > > > const uint8_t *buf, @@ -215,6 +219,7 @@ > > > > static int packet_enqueue(CompareState *s, int mode, Connection > > **con) > > > > ConnectionKey key; > > > > Packet *pkt = NULL; > > > > Connection *conn; > > > > + int ret; > > > > > > > > if (mode == PRIMARY_IN) { > > > > pkt = packet_new(s->pri_rs.buf, @@ -243,16 +248,18 @@ > > > > static int packet_enqueue(CompareState *s, int mode, Connection > > **con) > > > > } > > > > > > > > if (mode == PRIMARY_IN) { > > > > - if (!colo_insert_packet(&conn->primary_list, pkt, &conn->pack)) { > > > > - error_report("colo compare primary queue size too big," > > > > - "drop packet"); > > > > - } > > > > + ret = colo_insert_packet(&conn->primary_list, pkt, > > > > + &conn->pack); > > > > } else { > > > > - if (!colo_insert_packet(&conn->secondary_list, pkt, &conn->sack)) { > > > > - error_report("colo compare secondary queue size too big," > > > > - "drop packet"); > > > > - } > > > > + ret = colo_insert_packet(&conn->secondary_list, pkt, > > > > + &conn->sack); > > > > } > > > > + > > > > + if (!ret) { > > > > + error_report("colo compare %s queue size too big," > > > > + "drop packet", colo_mode[mode]); > > > > + packet_destroy(pkt, NULL); > > > > + pkt = NULL; > > > > + } > > > > + > > > > *con = conn; > > > > > > > > return 0; > > > > -- > > > > 2.17.1 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in packet_enqueue() 2020-03-25 2:05 ` Jing-Wei Su @ 2020-03-25 4:16 ` Derek Su 2020-03-25 5:42 ` Zhang, Chen 0 siblings, 1 reply; 8+ messages in thread From: Derek Su @ 2020-03-25 4:16 UTC (permalink / raw) To: Zhang, Chen; +Cc: dereksu, jasowang, qemu-devel, lizhijian Jing-Wei Su <jwsu1986@gmail.com> 於 2020年3月25日 週三 上午10:05寫道: > > Zhang, Chen <chen.zhang@intel.com> 於 2020年3月25日 週三 上午9:37寫道: > > > > > > > > > -----Original Message----- > > > From: Jing-Wei Su <jwsu1986@gmail.com> > > > Sent: Tuesday, March 24, 2020 10:47 AM > > > To: Zhang, Chen <chen.zhang@intel.com> > > > Cc: qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; > > > jasowang@redhat.com; dereksu@qnap.com > > > Subject: Re: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > > > packet_enqueue() > > > > > > Zhang, Chen <chen.zhang@intel.com> 於 2020年3月24日 週二 上午3:24 > > > 寫道: > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > From: Derek Su <jwsu1986@gmail.com> > > > > > Sent: Monday, March 23, 2020 1:48 AM > > > > > To: qemu-devel@nongnu.org > > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; > > > > > jasowang@redhat.com; dereksu@qnap.com > > > > > Subject: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > > > > > packet_enqueue() > > > > > > > > > > The patch is to fix the "pkt" memory leak in packet_enqueue(). > > > > > The allocated "pkt" needs to be freed if the colo compare primary or > > > > > secondary queue is too big. > > > > > > > > Hi Derek, > > > > > > > > Thank you for the patch. > > > > I re-think this issue in a big view, looks just free the pkg is not enough in > > > this situation. > > > > The root cause is network is too busy to compare, So, better choice is > > > > notify COLO frame to do a checkpoint and clean up all the network > > > > queue. This work maybe decrease COLO network performance but seams > > > better than drop lots of pkg. > > > > > > > > Thanks > > > > Zhang Chen > > > > > > > > > > Hello, Zhang > > > > > > Got it. > > > What is the concern of the massive "drop packets"? > > > Does the behavior make the COLO do checkpoint periodically (~20 seconds) > > > instead of doing immediate checkpoint when encountering different > > > response packets? > > > > The concern of the "drop packets" is guest will lose network connection with > > most of real clients until next periodic force checkpoint. COLO designed for dynamic > > control checkpoint, so I think do a checkpoint here will help guest supply service faster. > > > > I see. > I'll update the patch with your suggestion later. > Hi, Zhang Here is the idea and pseudo code to handle the "drop packet". ``` ret = packet_enqueue (1) ret == 0 compare connection (2) ret == -1 send packet (3) ret == queue insertion fail do checkpoint send all queued primary packets remove all queued secondary packets ``` Do you have any comment for the handling? Thanks Derek > > > > > > It seems that frequent checkpoints caused by the full queue (busy > > > network) instead of different > > > response packets may harm the high speed network (10 Gbps or higher) > > > performance dramatically. > > > > Yes, maybe I can send a patch to make user adjust queue size depend on it's own environment. > > But with larger queue size, colo-compare will spend much time to do compare packet when network > > Is real busy status. > > Thank you. The user-configurable queue size will be very helpful. > > Thanks. > Derek Su > > > > > Thanks > > Zhang Chen > > > > > > > > Thanks > > > Derek > > > > > > > > > > > > > Signed-off-by: Derek Su <dereksu@qnap.com> > > > > > --- > > > > > net/colo-compare.c | 23 +++++++++++++++-------- > > > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > > > > 7ee17f2cf8..cdd87b2aa8 100644 > > > > > --- a/net/colo-compare.c > > > > > +++ b/net/colo-compare.c > > > > > @@ -120,6 +120,10 @@ enum { > > > > > SECONDARY_IN, > > > > > }; > > > > > > > > > > +static const char *colo_mode[] = { > > > > > + [PRIMARY_IN] = "primary", > > > > > + [SECONDARY_IN] = "secondary", > > > > > +}; > > > > > > > > > > static int compare_chr_send(CompareState *s, > > > > > const uint8_t *buf, @@ -215,6 +219,7 @@ > > > > > static int packet_enqueue(CompareState *s, int mode, Connection > > > **con) > > > > > ConnectionKey key; > > > > > Packet *pkt = NULL; > > > > > Connection *conn; > > > > > + int ret; > > > > > > > > > > if (mode == PRIMARY_IN) { > > > > > pkt = packet_new(s->pri_rs.buf, @@ -243,16 +248,18 @@ > > > > > static int packet_enqueue(CompareState *s, int mode, Connection > > > **con) > > > > > } > > > > > > > > > > if (mode == PRIMARY_IN) { > > > > > - if (!colo_insert_packet(&conn->primary_list, pkt, &conn->pack)) { > > > > > - error_report("colo compare primary queue size too big," > > > > > - "drop packet"); > > > > > - } > > > > > + ret = colo_insert_packet(&conn->primary_list, pkt, > > > > > + &conn->pack); > > > > > } else { > > > > > - if (!colo_insert_packet(&conn->secondary_list, pkt, &conn->sack)) { > > > > > - error_report("colo compare secondary queue size too big," > > > > > - "drop packet"); > > > > > - } > > > > > + ret = colo_insert_packet(&conn->secondary_list, pkt, > > > > > + &conn->sack); > > > > > } > > > > > + > > > > > + if (!ret) { > > > > > + error_report("colo compare %s queue size too big," > > > > > + "drop packet", colo_mode[mode]); > > > > > + packet_destroy(pkt, NULL); > > > > > + pkt = NULL; > > > > > + } > > > > > + > > > > > *con = conn; > > > > > > > > > > return 0; > > > > > -- > > > > > 2.17.1 > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in packet_enqueue() 2020-03-25 4:16 ` Derek Su @ 2020-03-25 5:42 ` Zhang, Chen 0 siblings, 0 replies; 8+ messages in thread From: Zhang, Chen @ 2020-03-25 5:42 UTC (permalink / raw) To: Derek Su; +Cc: dereksu, jasowang, qemu-devel, lizhijian > -----Original Message----- > From: Derek Su <jwsu1986@gmail.com> > Sent: Wednesday, March 25, 2020 12:17 PM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; > jasowang@redhat.com; dereksu@qnap.com > Subject: Re: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > packet_enqueue() > > Jing-Wei Su <jwsu1986@gmail.com> 於 2020年3月25日 週三 上午10:05 > 寫道: > > > > Zhang, Chen <chen.zhang@intel.com> 於 2020年3月25日 週三 上午 > 9:37寫道: > > > > > > > > > > > > > -----Original Message----- > > > > From: Jing-Wei Su <jwsu1986@gmail.com> > > > > Sent: Tuesday, March 24, 2020 10:47 AM > > > > To: Zhang, Chen <chen.zhang@intel.com> > > > > Cc: qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; > > > > jasowang@redhat.com; dereksu@qnap.com > > > > Subject: Re: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > > > > packet_enqueue() > > > > > > > > Zhang, Chen <chen.zhang@intel.com> 於 2020年3月24日 週二 上午 > 3:24 > > > > 寫道: > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Derek Su <jwsu1986@gmail.com> > > > > > > Sent: Monday, March 23, 2020 1:48 AM > > > > > > To: qemu-devel@nongnu.org > > > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; > > > > > > lizhijian@cn.fujitsu.com; jasowang@redhat.com; > > > > > > dereksu@qnap.com > > > > > > Subject: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > > > > > > packet_enqueue() > > > > > > > > > > > > The patch is to fix the "pkt" memory leak in packet_enqueue(). > > > > > > The allocated "pkt" needs to be freed if the colo compare > > > > > > primary or secondary queue is too big. > > > > > > > > > > Hi Derek, > > > > > > > > > > Thank you for the patch. > > > > > I re-think this issue in a big view, looks just free the pkg is > > > > > not enough in > > > > this situation. > > > > > The root cause is network is too busy to compare, So, better > > > > > choice is notify COLO frame to do a checkpoint and clean up all > > > > > the network queue. This work maybe decrease COLO network > > > > > performance but seams > > > > better than drop lots of pkg. > > > > > > > > > > Thanks > > > > > Zhang Chen > > > > > > > > > > > > > Hello, Zhang > > > > > > > > Got it. > > > > What is the concern of the massive "drop packets"? > > > > Does the behavior make the COLO do checkpoint periodically (~20 > > > > seconds) instead of doing immediate checkpoint when encountering > > > > different response packets? > > > > > > The concern of the "drop packets" is guest will lose network > > > connection with most of real clients until next periodic force > > > checkpoint. COLO designed for dynamic control checkpoint, so I think do > a checkpoint here will help guest supply service faster. > > > > > > > I see. > > I'll update the patch with your suggestion later. > > > > Hi, Zhang > Here is the idea and pseudo code to handle the "drop packet". > > ``` > ret = packet_enqueue > (1) ret == 0 > compare connection > (2) ret == -1 > send packet > (3) ret == queue insertion fail > do checkpoint > send all queued primary packets > remove all queued secondary packets ``` > > Do you have any comment for the handling? Looks good for me. Thanks Zhang Chen > > Thanks > Derek > > > > > > > > > It seems that frequent checkpoints caused by the full queue (busy > > > > network) instead of different > > > > response packets may harm the high speed network (10 Gbps or > > > > higher) performance dramatically. > > > > > > Yes, maybe I can send a patch to make user adjust queue size depend on > it's own environment. > > > But with larger queue size, colo-compare will spend much time to do > > > compare packet when network Is real busy status. > > > > Thank you. The user-configurable queue size will be very helpful. > > > > Thanks. > > Derek Su > > > > > > > > Thanks > > > Zhang Chen > > > > > > > > > > > Thanks > > > > Derek > > > > > > > > > > > > > > > > Signed-off-by: Derek Su <dereksu@qnap.com> > > > > > > --- > > > > > > net/colo-compare.c | 23 +++++++++++++++-------- > > > > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > > > > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > > > > > 7ee17f2cf8..cdd87b2aa8 100644 > > > > > > --- a/net/colo-compare.c > > > > > > +++ b/net/colo-compare.c > > > > > > @@ -120,6 +120,10 @@ enum { > > > > > > SECONDARY_IN, > > > > > > }; > > > > > > > > > > > > +static const char *colo_mode[] = { > > > > > > + [PRIMARY_IN] = "primary", > > > > > > + [SECONDARY_IN] = "secondary", }; > > > > > > > > > > > > static int compare_chr_send(CompareState *s, > > > > > > const uint8_t *buf, @@ -215,6 > > > > > > +219,7 @@ static int packet_enqueue(CompareState *s, int mode, > > > > > > Connection > > > > **con) > > > > > > ConnectionKey key; > > > > > > Packet *pkt = NULL; > > > > > > Connection *conn; > > > > > > + int ret; > > > > > > > > > > > > if (mode == PRIMARY_IN) { > > > > > > pkt = packet_new(s->pri_rs.buf, @@ -243,16 +248,18 @@ > > > > > > static int packet_enqueue(CompareState *s, int mode, > > > > > > Connection > > > > **con) > > > > > > } > > > > > > > > > > > > if (mode == PRIMARY_IN) { > > > > > > - if (!colo_insert_packet(&conn->primary_list, pkt, &conn- > >pack)) { > > > > > > - error_report("colo compare primary queue size too big," > > > > > > - "drop packet"); > > > > > > - } > > > > > > + ret = colo_insert_packet(&conn->primary_list, pkt, > > > > > > + &conn->pack); > > > > > > } else { > > > > > > - if (!colo_insert_packet(&conn->secondary_list, pkt, &conn- > >sack)) { > > > > > > - error_report("colo compare secondary queue size too big," > > > > > > - "drop packet"); > > > > > > - } > > > > > > + ret = colo_insert_packet(&conn->secondary_list, pkt, > > > > > > + &conn->sack); > > > > > > } > > > > > > + > > > > > > + if (!ret) { > > > > > > + error_report("colo compare %s queue size too big," > > > > > > + "drop packet", colo_mode[mode]); > > > > > > + packet_destroy(pkt, NULL); > > > > > > + pkt = NULL; > > > > > > + } > > > > > > + > > > > > > *con = conn; > > > > > > > > > > > > return 0; > > > > > > -- > > > > > > 2.17.1 > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-03-25 5:43 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-22 17:47 [PATCH v2 0/1] COLO: Fix memory leak in packet_enqueue() Derek Su 2020-03-22 17:47 ` [PATCH v2 1/1] net/colo-compare.c: " Derek Su 2020-03-23 19:24 ` Zhang, Chen 2020-03-24 2:46 ` Jing-Wei Su 2020-03-25 1:37 ` Zhang, Chen 2020-03-25 2:05 ` Jing-Wei Su 2020-03-25 4:16 ` Derek Su 2020-03-25 5:42 ` Zhang, Chen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.