RE: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in packet_enqueue()

["Zhang, Chen" <chen.zhang@intel.com>]

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