> -----Original Message----- > From: Derek Su <jwsu1...@gmail.com> > Sent: Wednesday, March 25, 2020 12:17 PM > To: Zhang, Chen <chen.zh...@intel.com> > Cc: qemu-devel@nongnu.org; lizhij...@cn.fujitsu.com; > jasow...@redhat.com; dere...@qnap.com > Subject: Re: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > packet_enqueue() > > Jing-Wei Su <jwsu1...@gmail.com> 於 2020年3月25日 週三 上午10:05 > 寫道: > > > > Zhang, Chen <chen.zh...@intel.com> 於 2020年3月25日 週三 上午 > 9:37寫道: > > > > > > > > > > > > > -----Original Message----- > > > > From: Jing-Wei Su <jwsu1...@gmail.com> > > > > Sent: Tuesday, March 24, 2020 10:47 AM > > > > To: Zhang, Chen <chen.zh...@intel.com> > > > > Cc: qemu-devel@nongnu.org; lizhij...@cn.fujitsu.com; > > > > jasow...@redhat.com; dere...@qnap.com > > > > Subject: Re: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in > > > > packet_enqueue() > > > > > > > > Zhang, Chen <chen.zh...@intel.com> 於 2020年3月24日 週二 上午 > 3:24 > > > > 寫道: > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Derek Su <jwsu1...@gmail.com> > > > > > > Sent: Monday, March 23, 2020 1:48 AM > > > > > > To: qemu-devel@nongnu.org > > > > > > Cc: Zhang, Chen <chen.zh...@intel.com>; > > > > > > lizhij...@cn.fujitsu.com; jasow...@redhat.com; > > > > > > dere...@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 <dere...@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 > > > > >