[ovs-dev] [optimize 06/26] ofproto-dpif: Don't do any accounting at all when removing facets.

Ben Pfaff blp at nicira.com
Wed Apr 18 17:53:33 UTC 2012


I think that this commit needs to get broken into two, with better
commit log.  I'll work on that.

On Tue, Apr 17, 2012 at 03:46:00PM -0700, Ethan Jackson wrote:
> I'm having a bit of trouble understanding this patch.
> 
> It seems to me like it continues to do accounting when removing
> facets, what it's actually doing is avoiding learning when removing
> them.  I may be misinterpreting though.  Just to make sure I'm
> understanding: this is an optimization because it avoid the redundant
> call to xlate_actions() in facet_learn()?
> 
> Why remove "facet->accounted_bytes = facet->byte_count" from
> facet_account(), every caller seems to do it manually now.
> 
> I think I'm just a bit confused, it may be worth expanding the commit
> message a bit.
> 
> Ethan
> 
> On Mon, Apr 16, 2012 at 17:18, Ben Pfaff <blp at nicira.com> wrote:
> > From: Ben Pfaff <blp at hardrock.nicira.com>
> >
> > Signed-off-by: Ben Pfaff <blp at hardrock.nicira.com>
> > ---
> >  ofproto/ofproto-dpif.c |   90 +++++++++++++++++++++++++-----------------------
> >  1 files changed, 47 insertions(+), 43 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 594a705..cc96ccf 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -209,17 +209,13 @@ struct action_xlate_ctx {
> >      * revalidating without a packet to refer to. */
> >     const struct ofpbuf *packet;
> >
> > -    /* Should OFPP_NORMAL update the MAC learning table?  We want to update it
> > -     * if we are actually processing a packet, or if we are accounting for
> > -     * packets that the datapath has processed, but not if we are just
> > -     * revalidating. */
> > -    bool may_learn_macs;
> > -
> > -    /* Should "learn" actions update the flow table?  We want to update it if
> > -     * we are actually processing a packet, or in most cases if we are
> > -     * accounting for packets that the datapath has processed, but not if we
> > -     * are just revalidating.  */
> > -    bool may_flow_mod;
> > +    /* Should OFPP_NORMAL update the MAC learning table?  Should "learn"
> > +     * actions update the flow table?
> > +     *
> > +     * We want to update these tables if we are actually processing a packet,
> > +     * or if we are accounting for packets that the datapath has processed, but
> > +     * not if we are just revalidating. */
> > +    bool may_learn;
> >
> >     /* The rule that we are currently translating, or NULL. */
> >     struct rule_dpif *rule;
> > @@ -352,7 +348,8 @@ static void facet_flush_stats(struct facet *);
> >  static void facet_update_time(struct facet *, long long int used);
> >  static void facet_reset_counters(struct facet *);
> >  static void facet_push_stats(struct facet *);
> > -static void facet_account(struct facet *, bool may_flow_mod);
> > +static void facet_learn(struct facet *);
> > +static void facet_account(struct facet *);
> >
> >  static bool facet_is_controller_flow(struct facet *);
> >
> > @@ -2998,7 +2995,11 @@ update_stats(struct ofproto_dpif *p)
> >             facet->tcp_flags |= stats->tcp_flags;
> >
> >             subfacet_update_time(subfacet, stats->used);
> > -            facet_account(facet, true);
> > +            if (facet->accounted_bytes < facet->byte_count) {
> > +                facet_learn(facet);
> > +                facet_account(facet);
> > +                facet->accounted_bytes = facet->byte_count;
> > +            }
> >             facet_push_stats(facet);
> >         } else {
> >             if (!VLOG_DROP_WARN(&rl)) {
> > @@ -3294,42 +3295,43 @@ facet_remove(struct facet *facet)
> >     facet_free(facet);
> >  }
> >
> > +/* Feed information from 'facet' back into the learning table to keep it in
> > + * sync with what is actually flowing through the datapath. */
> >  static void
> > -facet_account(struct facet *facet, bool may_flow_mod)
> > +facet_learn(struct facet *facet)
> >  {
> >     struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
> > -    uint64_t n_bytes;
> > -    struct subfacet *subfacet;
> > -    const struct nlattr *a;
> > -    unsigned int left;
> > -    ovs_be16 vlan_tci;
> > +    struct action_xlate_ctx ctx;
> >
> > -    if (facet->byte_count <= facet->accounted_bytes) {
> > +    if (!facet->has_learn
> > +        && !facet->has_normal
> > +        && (!facet->has_fin_timeout
> > +            || !(facet->tcp_flags & (TCP_FIN | TCP_RST)))) {
> >         return;
> >     }
> > -    n_bytes = facet->byte_count - facet->accounted_bytes;
> > -    facet->accounted_bytes = facet->byte_count;
> > -
> > -    /* Feed information from the active flows back into the learning table to
> > -     * ensure that table is always in sync with what is actually flowing
> > -     * through the datapath. */
> > -    if (facet->has_learn || facet->has_normal
> > -        || (facet->has_fin_timeout
> > -            && facet->tcp_flags & (TCP_FIN | TCP_RST))) {
> > -        struct action_xlate_ctx ctx;
> >
> > -        action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
> > -                              facet->flow.vlan_tci,
> > -                              facet->rule, facet->tcp_flags, NULL);
> > -        ctx.may_learn_macs = true;
> > -        ctx.may_flow_mod = may_flow_mod;
> > -        ofpbuf_delete(xlate_actions(&ctx, facet->rule->up.actions,
> > -                                    facet->rule->up.n_actions));
> > -    }
> > +    action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
> > +                          facet->flow.vlan_tci,
> > +                          facet->rule, facet->tcp_flags, NULL);
> > +    ctx.may_learn = true;
> > +    ofpbuf_delete(xlate_actions(&ctx, facet->rule->up.actions,
> > +                                facet->rule->up.n_actions));
> > +}
> > +
> > +static void
> > +facet_account(struct facet *facet)
> > +{
> > +    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
> > +    struct subfacet *subfacet;
> > +    const struct nlattr *a;
> > +    unsigned int left;
> > +    ovs_be16 vlan_tci;
> > +    uint64_t n_bytes;
> >
> >     if (!facet->has_normal || !ofproto->has_bonded_bundles) {
> >         return;
> >     }
> > +    n_bytes = facet->byte_count - facet->accounted_bytes;
> >
> >     /* This loop feeds byte counters to bond_account() for rebalancing to use
> >      * as a basis.  We also need to track the actual VLAN on which the packet
> > @@ -3396,7 +3398,10 @@ facet_flush_stats(struct facet *facet)
> >     }
> >
> >     facet_push_stats(facet);
> > -    facet_account(facet, false);
> > +    if (facet->accounted_bytes < facet->byte_count) {
> > +        facet_account(facet);
> > +        facet->accounted_bytes = facet->byte_count;
> > +    }
> >
> >     if (ofproto->netflow && !facet_is_controller_flow(facet)) {
> >         struct ofexpired expired;
> > @@ -5025,7 +5030,7 @@ do_xlate_actions(const union ofp_action *in, size_t n_in,
> >
> >         case OFPUTIL_NXAST_LEARN:
> >             ctx->has_learn = true;
> > -            if (ctx->may_flow_mod) {
> > +            if (ctx->may_learn) {
> >                 xlate_learn_action(ctx, (const struct nx_action_learn *) ia);
> >             }
> >             break;
> > @@ -5078,8 +5083,7 @@ action_xlate_ctx_init(struct action_xlate_ctx *ctx,
> >     ctx->base_flow.vlan_tci = initial_tci;
> >     ctx->rule = rule;
> >     ctx->packet = packet;
> > -    ctx->may_learn_macs = packet != NULL;
> > -    ctx->may_flow_mod = packet != NULL;
> > +    ctx->may_learn = packet != NULL;
> >     ctx->tcp_flags = tcp_flags;
> >     ctx->resubmit_hook = NULL;
> >  }
> > @@ -5707,7 +5711,7 @@ xlate_normal(struct action_xlate_ctx *ctx)
> >     }
> >
> >     /* Learn source MAC. */
> > -    if (ctx->may_learn_macs) {
> > +    if (ctx->may_learn) {
> >         update_learning_table(ctx->ofproto, &ctx->flow, vlan, in_bundle);
> >     }
> >
> > --
> > 1.7.9
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list