[ovs-dev] [PATCH] ofproto-dpif: Keep subfacets longer to avoid assert-fail in facet_account().

[Ben Pfaff]

Pushed to master, branch-1.4.

On Fri, Jan 06, 2012 at 04:52:36PM -0800, Justin Pettit wrote:
> Looks good.  Thanks!
> 
> --Justin
> 
> 
> On Jan 6, 2012, at 3:04 PM, Ben Pfaff wrote:
> 
> > 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 <blp at nicira.com>
> > Reported-by: Mike Kruze <mkruze at nicira.com>
> > Bug #9074.
> > ---
> > AUTHORS                |    1 +
> > ofproto/ofproto-dpif.c |   23 +++++++++++++++++++----
> > 2 files changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/AUTHORS b/AUTHORS
> > index 821f780..631dfec 100644
> > --- a/AUTHORS
> > +++ b/AUTHORS
> > @@ -100,6 +100,7 @@ Michael A. Collins      mike.a.collins at ark-net.org
> > Michael Hu              mhu at nicira.com
> > Michael Mao             mmao at nicira.com
> > Mike Bursell            mike.bursell at citrix.com
> > +Mike Kruze              mkruze at nicira.com
> > Murphy McCauley         murphy.mccauley at gmail.com
> > Mikael Doverhag         mdoverhag at nicira.com
> > Niklas Andersson        nandersson at nicira.com
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index baa191e..fe99d70 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);
> >     }
> > }
> > 
> > -- 
> > 1.7.2.5
> > 
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>