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

[Derek Su]

Jing-Wei Su <address@hidden> 於 2020年3月25日 週三 上午10:05寫道：
>
> 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.
>

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