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

[Jing-Wei Su]

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?

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.

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
>