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

[Jing-Wei Su]

Zhang, Chen <address@hidden> 於 2020年3月25日 週三 上午9:37寫道：
>
>
>
> > -----Original Message-----
> > From: Jing-Wei Su <address@hidden>
> > Sent: Tuesday, March 24, 2020 10:47 AM
> > To: Zhang, Chen <address@hidden>
> > Cc: address@hidden; address@hidden;
> > address@hidden; address@hidden
> > Subject: Re: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in
> > packet_enqueue()
> >
> > Zhang, Chen <address@hidden> 於 2020年3月24日 週二 上午3:24
> > 寫道：
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Derek Su <address@hidden>
> > > > Sent: Monday, March 23, 2020 1:48 AM
> > > > To: address@hidden
> > > > Cc: Zhang, Chen <address@hidden>; address@hidden;
> > > > address@hidden; address@hidden
> > > > 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 <address@hidden>
> > > > ---
> > > >  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
> > >