Thanks, applied to branch-1.4.
At the same time, I noticed that I had forgotten to tag 1.4.6, so I
did that too.
On Wed, May 01, 2013 at 09:55:58AM -0700, Justin Pettit wrote:
> The code's tricky, but I looked over it a couple of times, and it
> looks reasonable to me.
>
> Thanks,
>
> --Justin
>
>
> On Apr 25, 2013, at 11:23 AM, Ben Pfaff <b...@nicira.com> wrote:
>
> > In the case where execute_controller_action() returned true to
> > handle_flow_miss(), indicating that the packet had been sent to the
> > controller, nothing ever freed the packet, causing a memory leak.
> >
> > One plausible solution seemed to be to turn over ownership of the packet to
> > execute_controller_action(), by passing false instead of true as its
> > 'clone' argument. Another is to add an else case to the "if" statement
> > that calls execute_controller_action() to free the packet.
> >
> > However, neither of these solutions is actually correct, for a subtle
> > reason. The block of memory that includes the packet also includes the
> > key for the flow miss. At the end of handle_flow_miss(), past the end of
> > the loop, the key is normally needed to install the flow in the datapath.
> > Thus, by destroying the packet we potentially corrupt the key, and this
> > has actually been seen while testing memory leak fixes.
> >
> > This commit uses a different approach: instead of trying to destroy the
> > packets one at a time when it becomes possible, it destroys all the packets
> > together at the end.
> >
> > Reported-by: Zoltan Kiss <zoltan.k...@citrix.com>
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > v1->v2: Fix typo in commit fix, fix double-free of 'todo' hmap.
> >
> > ofproto/ofproto-dpif.c | 19 ++++++++++---------
> > 1 files changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 2949085..1189919 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks.
> > + * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira Networks.
> > *
> > * Licensed under the Apache License, Version 2.0 (the "License");
> > * you may not use this file except in compliance with the License.
> > @@ -2516,10 +2516,9 @@ handle_flow_miss(struct ofproto_dpif *ofproto,
> > struct flow_miss *miss,
> > miss->key_fitness, miss->key, miss->key_len,
> > miss->initial_tci);
> >
> > - LIST_FOR_EACH_SAFE (packet, next_packet, list_node, &miss->packets) {
> > + LIST_FOR_EACH (packet, list_node, &miss->packets) {
> > struct dpif_flow_stats stats;
> >
> > - list_remove(&packet->list_node);
> > ofproto->n_matches++;
> >
> > if (facet->rule->up.cr.priority == FAIL_OPEN_PRIORITY) {
> > @@ -2710,14 +2709,10 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto,
> > struct dpif_upcall *upcalls,
> > /* Process each element in the to-do list, constructing the set of
> > * operations to batch. */
> > n_ops = 0;
> > - HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
> > + HMAP_FOR_EACH (miss, hmap_node, &todo) {
> > handle_flow_miss(ofproto, miss, flow_miss_ops, &n_ops);
> > - ofpbuf_list_delete(&miss->packets);
> > - hmap_remove(&todo, &miss->hmap_node);
> > - free(miss);
> > }
> > assert(n_ops <= ARRAY_SIZE(flow_miss_ops));
> > - hmap_destroy(&todo);
> >
> > /* Execute batch. */
> > for (i = 0; i < n_ops; i++) {
> > @@ -2737,7 +2732,6 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto,
> > struct dpif_upcall *upcalls,
> > if (op->subfacet->actions != execute->actions) {
> > free((struct nlattr *) execute->actions);
> > }
> > - ofpbuf_delete((struct ofpbuf *) execute->packet);
> > break;
> >
> > case DPIF_OP_FLOW_PUT:
> > @@ -2748,6 +2742,13 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto,
> > struct dpif_upcall *upcalls,
> > break;
> > }
> > }
> > +
> > + HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
> > + ofpbuf_list_delete(&miss->packets);
> > + hmap_remove(&todo, &miss->hmap_node);
> > + free(miss);
> > + }
> > + hmap_destroy(&todo);
> > }
> >
> > static void
> > --
> > 1.7.2.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
