| Message ID | 20200410020056.12104-2-dereksu@qnap.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | COLO: handling of the full primary or secondary queue | expand |
> -----Original Message----- > From: Derek Su <dereksu@qnap.com> > Sent: Friday, April 10, 2020 10:01 AM > To: qemu-devel@nongnu.org > Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; > jasowang@redhat.com; ctcheng@qnap.com; chyang@qnap.com; > jwsu1986@gmail.com; Derek Su <dereksu@qnap.com> > Subject: [PATCH v5 1/1] colo-compare: 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. > > Replace the error_report of full queue with a trace event. > Looks good for me. Thanks your contribution. Reviewed-by: Zhang Chen <chen.zhang@intel.com> Thanks Zhang Chen > Signed-off-by: Derek Su <dereksu@qnap.com> > --- > net/colo-compare.c | 23 +++++++++++++++-------- > net/trace-events | 1 + > 2 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > 10c0239f9d..035e11d4d3 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -122,6 +122,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, @@ -217,6 +221,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, @@ -245,16 +250,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) { > + trace_colo_compare_drop_packet(colo_mode[mode], > + "queue size too big, drop packet"); > + packet_destroy(pkt, NULL); > + pkt = NULL; > + } > + > *con = conn; > > return 0; > diff --git a/net/trace-events b/net/trace-events index > 02c13fd0ba..fa49c71533 100644 > --- a/net/trace-events > +++ b/net/trace-events > @@ -12,6 +12,7 @@ colo_proxy_main(const char *chr) ": %s" > > # colo-compare.c > colo_compare_main(const char *chr) ": %s" > +colo_compare_drop_packet(const char *queue, const char *chr) ": %s: %s" > colo_compare_udp_miscompare(const char *sta, int size) ": %s = %d" > colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d" > colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, > const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, > spkt size = %d, ip_src = %s, ip_dst = %s" > -- > 2.17.1
On Fri, 10 Apr 2020 10:00:56 +0800 Derek Su <dereksu@qnap.com> wrote: > 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. > > Replace the error_report of full queue with a trace event. > > Signed-off-by: Derek Su <dereksu@qnap.com> Looks good now and works well in my tests. Reviewed-by: Lukas Straub <lukasstraub2@web.de> Tested-by: Lukas Straub <lukasstraub2@web.de> Regards, Lukas Straub > --- > net/colo-compare.c | 23 +++++++++++++++-------- > net/trace-events | 1 + > 2 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c > index 10c0239f9d..035e11d4d3 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -122,6 +122,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, > @@ -217,6 +221,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, > @@ -245,16 +250,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) { > + trace_colo_compare_drop_packet(colo_mode[mode], > + "queue size too big, drop packet"); > + packet_destroy(pkt, NULL); > + pkt = NULL; > + } > + > *con = conn; > > return 0; > diff --git a/net/trace-events b/net/trace-events > index 02c13fd0ba..fa49c71533 100644 > --- a/net/trace-events > +++ b/net/trace-events > @@ -12,6 +12,7 @@ colo_proxy_main(const char *chr) ": %s" > > # colo-compare.c > colo_compare_main(const char *chr) ": %s" > +colo_compare_drop_packet(const char *queue, const char *chr) ": %s: %s" > colo_compare_udp_miscompare(const char *sta, int size) ": %s = %d" > colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d" > colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst = %s"
> -----Original Message----- > From: Lukas Straub <lukasstraub2@web.de> > Sent: Sunday, April 26, 2020 7:25 PM > To: Derek Su <dereksu@qnap.com> > Cc: qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; chyang@qnap.com; > jasowang@redhat.com; ctcheng@qnap.com; Zhang, Chen > <chen.zhang@intel.com>; jwsu1986@gmail.com > Subject: Re: [PATCH v5 1/1] colo-compare: Fix memory leak in > packet_enqueue() > > On Fri, 10 Apr 2020 10:00:56 +0800 > Derek Su <dereksu@qnap.com> wrote: > > > 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. > > > > Replace the error_report of full queue with a trace event. > > > > Signed-off-by: Derek Su <dereksu@qnap.com> > > Looks good now and works well in my tests. > Reviewed-by: Lukas Straub <lukasstraub2@web.de> > Tested-by: Lukas Straub <lukasstraub2@web.de> > I will queue this patch to COLO branch, then submit to Jason. Thanks Zhang Chen > Regards, > Lukas Straub > > > --- > > net/colo-compare.c | 23 +++++++++++++++-------- > > net/trace-events | 1 + > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > 10c0239f9d..035e11d4d3 100644 > > --- a/net/colo-compare.c > > +++ b/net/colo-compare.c > > @@ -122,6 +122,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, @@ -217,6 +221,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, @@ -245,16 +250,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) { > > + trace_colo_compare_drop_packet(colo_mode[mode], > > + "queue size too big, drop packet"); > > + packet_destroy(pkt, NULL); > > + pkt = NULL; > > + } > > + > > *con = conn; > > > > return 0; > > diff --git a/net/trace-events b/net/trace-events index > > 02c13fd0ba..fa49c71533 100644 > > --- a/net/trace-events > > +++ b/net/trace-events > > @@ -12,6 +12,7 @@ colo_proxy_main(const char *chr) ": %s" > > > > # colo-compare.c > > colo_compare_main(const char *chr) ": %s" > > +colo_compare_drop_packet(const char *queue, const char *chr) > ": %s: %s" > > colo_compare_udp_miscompare(const char *sta, int size) ": %s = %d" > > colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d" > > colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, > const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, > spkt size = %d, ip_src = %s, ip_dst = %s"
diff --git a/net/colo-compare.c b/net/colo-compare.c index 10c0239f9d..035e11d4d3 100644 --- a/net/colo-compare.c +++ b/net/colo-compare.c @@ -122,6 +122,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, @@ -217,6 +221,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, @@ -245,16 +250,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) { + trace_colo_compare_drop_packet(colo_mode[mode], + "queue size too big, drop packet"); + packet_destroy(pkt, NULL); + pkt = NULL; + } + *con = conn; return 0; diff --git a/net/trace-events b/net/trace-events index 02c13fd0ba..fa49c71533 100644 --- a/net/trace-events +++ b/net/trace-events @@ -12,6 +12,7 @@ colo_proxy_main(const char *chr) ": %s" # colo-compare.c colo_compare_main(const char *chr) ": %s" +colo_compare_drop_packet(const char *queue, const char *chr) ": %s: %s" colo_compare_udp_miscompare(const char *sta, int size) ": %s = %d" colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d" colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst = %s"
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. Replace the error_report of full queue with a trace event. Signed-off-by: Derek Su <dereksu@qnap.com> --- net/colo-compare.c | 23 +++++++++++++++-------- net/trace-events | 1 + 2 files changed, 16 insertions(+), 8 deletions(-)