From 551a2f6ca9a90a577bc25cdd913f6df6bd8d9b23 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 6 Jan 2012 15:03:07 -0800 Subject: [PATCH] ofproto-dpif: Keep subfacets longer to avoid assert-fail in facet_account(). If a subfacet expired when its facet still had statistics that had not yet been pushed into the rule, and the facet either used the "normal" action or the bridge contained a bond port, then facet_account() would be called after the last subfacet was removed from its facet's list of subfacets, triggering an assertion failure in list_front(). This fixes the problem by always running facet_flush_stats() (which calls facet_account()) before deleting the last subfacet from a facet. This problem took a while to surface because subfacets usually expire only long after their statistics have been pushed into the rule. Signed-off-by: Ben Pfaff Reported-by: Mike Kruze Bug #9074. --- AUTHORS | 1 + ofproto/ofproto-dpif.c | 23 +++++++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/AUTHORS b/AUTHORS index 821f780d..631dfecf 100644 --- a/AUTHORS +++ b/AUTHORS @@ -100,6 +100,7 @@ Michael A. Collins mike.a.collins@ark-net.org Michael Hu mhu@nicira.com Michael Mao mmao@nicira.com Mike Bursell mike.bursell@citrix.com +Mike Kruze mkruze@nicira.com Murphy McCauley murphy.mccauley@gmail.com Mikael Doverhag mdoverhag@nicira.com Niklas Andersson nandersson@nicira.com diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index baa191e7..fe99d70d 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3231,12 +3231,25 @@ facet_remove(struct ofproto_dpif *ofproto, struct facet *facet) { struct subfacet *subfacet, *next_subfacet; + assert(!list_is_empty(&facet->subfacets)); + + /* First uninstall all of the subfacets to get final statistics. */ + LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) { + subfacet_uninstall(ofproto, subfacet); + } + + /* Flush the final stats to the rule. + * + * This might require us to have at least one subfacet around so that we + * can use its actions for accounting in facet_account(), which is why we + * have uninstalled but not yet destroyed the subfacets. */ + facet_flush_stats(ofproto, facet); + + /* Now we're really all done so destroy everything. */ LIST_FOR_EACH_SAFE (subfacet, next_subfacet, list_node, &facet->subfacets) { subfacet_destroy__(ofproto, subfacet); } - - facet_flush_stats(ofproto, facet); hmap_remove(&ofproto->facets, &facet->hmap_node); list_remove(&facet->list_node); facet_free(facet); @@ -3711,9 +3724,11 @@ subfacet_destroy(struct ofproto_dpif *ofproto, struct subfacet *subfacet) { struct facet *facet = subfacet->facet; - subfacet_destroy__(ofproto, subfacet); - if (list_is_empty(&facet->subfacets)) { + if (list_is_singleton(&facet->subfacets)) { + /* facet_remove() needs at least one subfacet (it will remove it). */ facet_remove(ofproto, facet); + } else { + subfacet_destroy__(ofproto, subfacet); } } -- 2.30.2