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