Thanks for your review, I wasn't aware of that. I'd very much like to
review this patch.
Still i have few questions, could you ping me when you are available?
Thanks,
Alex Wang,
On Mon, Sep 23, 2013 at 10:57 AM, Jarno Rajahalme <jrajaha...@nicira.com>wrote:
> Alex,
>
> The "key" member in struct flow_miss refers to memory held by the "struct
> upcall", hence the upcalls should be freed only after the flow misses are
> processed by the main thread. How about this instead:
>
> Free upcalls after they are used.
>
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
> ---
> ofproto/ofproto-dpif-upcall.c | 17 +++++++++++++----
> ofproto/ofproto-dpif-upcall.h | 5 +++++
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index d75c61b..c65b366 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -309,6 +309,7 @@ void
> flow_miss_batch_destroy(struct flow_miss_batch *fmb)
> {
> struct flow_miss *miss, *next;
> + struct upcall *upcall, *next_upcall;
>
> if (!fmb) {
> return;
> @@ -319,6 +320,11 @@ flow_miss_batch_destroy(struct flow_miss_batch *fmb)
> miss_destroy(miss);
> }
>
> + LIST_FOR_EACH_SAFE (upcall, next_upcall, list_node, &fmb->upcalls) {
> + list_remove(&upcall->list_node);
> + upcall_destroy(upcall);
> + }
> +
> hmap_destroy(&fmb->misses);
> free(fmb);
> }
> @@ -599,7 +605,7 @@ handle_miss_upcalls(struct udpif *udpif, struct list
> *upcalls)
> struct dpif_op ops[FLOW_MISS_MAX_BATCH];
> struct upcall *upcall, *next;
> struct flow_miss_batch *fmb;
> - size_t n_upcalls, n_ops, i;
> + size_t n_misses, n_ops, i;
> struct flow_miss *miss;
> unsigned int reval_seq;
> bool fail_open;
> @@ -626,11 +632,12 @@ handle_miss_upcalls(struct udpif *udpif, struct list
> *upcalls)
> fmb = xmalloc(sizeof *fmb);
> atomic_read(&udpif->reval_seq, &fmb->reval_seq);
> hmap_init(&fmb->misses);
> - n_upcalls = 0;
> + list_init(&fmb->upcalls);
> + n_misses = 0;
> LIST_FOR_EACH_SAFE (upcall, next, list_node, upcalls) {
> struct dpif_upcall *dupcall = &upcall->dpif_upcall;
> struct ofpbuf *packet = dupcall->packet;
> - struct flow_miss *miss = &fmb->miss_buf[n_upcalls];
> + struct flow_miss *miss = &fmb->miss_buf[n_misses];
> struct flow_miss *existing_miss;
> struct ofproto_dpif *ofproto;
> odp_port_t odp_in_port;
> @@ -661,7 +668,7 @@ handle_miss_upcalls(struct udpif *udpif, struct list
> *upcalls)
> miss->stats.used = time_msec();
> miss->stats.tcp_flags = 0;
>
> - n_upcalls++;
> + n_misses++;
> } else {
> miss = existing_miss;
> }
> @@ -814,6 +821,8 @@ handle_miss_upcalls(struct udpif *udpif, struct list
> *upcalls)
> }
> }
>
> + list_move(&fmb->upcalls, upcalls);
> +
> atomic_read(&udpif->reval_seq, &reval_seq);
> if (reval_seq != fmb->reval_seq) {
> COVERAGE_INC(fmb_queue_revalidated);
> diff --git a/ofproto/ofproto-dpif-upcall.h b/ofproto/ofproto-dpif-upcall.h
> index 9bd19ad..cd97e79 100644
> --- a/ofproto/ofproto-dpif-upcall.h
> +++ b/ofproto/ofproto-dpif-upcall.h
> @@ -104,6 +104,11 @@ struct flow_miss_batch {
> struct hmap misses;
>
> unsigned int reval_seq;
> +
> + /* Flow misses refer to the memory held by "struct upcall"s,
> + * so we need to keep track of the upcalls to be able to
> + * free them when done. */
> + struct list upcalls; /* Contains "struct upcall"s. */
> };
>
> struct flow_miss_batch *flow_miss_batch_next(struct udpif *);
> --
>
> On Sep 23, 2013, at 10:06 AM, Alex Wang <al...@nicira.com> wrote:
>
> > This commit fixes a memory leak in ofproto-dpif-upcall module.
> > The memory leak is caused by not freeing the 'struct upcall's.
> >
> > Signed-off-by: Alex Wang <al...@nicira.com>
> > ---
> > ofproto/ofproto-dpif-upcall.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/ofproto/ofproto-dpif-upcall.c
> b/ofproto/ofproto-dpif-upcall.c
> > index d75c61b..ec253e1 100644
> > --- a/ofproto/ofproto-dpif-upcall.c
> > +++ b/ofproto/ofproto-dpif-upcall.c
> > @@ -814,6 +814,12 @@ handle_miss_upcalls(struct udpif *udpif, struct
> list *upcalls)
> > }
> > }
> >
> > + /* Frees all remaining upcalls. */
> > + LIST_FOR_EACH_SAFE (upcall, next, list_node, upcalls) {
> > + list_remove(&upcall->list_node);
> > + upcall_destroy(upcall);
> > + }
> > +
> > atomic_read(&udpif->reval_seq, &reval_seq);
> > if (reval_seq != fmb->reval_seq) {
> > COVERAGE_INC(fmb_queue_revalidated);
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
