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.
> >
> > 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
> > >
