[ovs-dev] [PATCH] ofproto-dpif: Consolidate facet stat logic.

Ben Pfaff blp at nicira.com
Wed May 29 20:04:56 UTC 2013


OK, thanks.

On Wed, May 29, 2013 at 02:41:55PM -0500, Ethan Jackson wrote:
> Oops, this patch ends up deleteing, xlate_actions_for_side_effects(),
> but then I need it again in the xlate series.  I think instead of
> basing this on master, I'll base it on the xlate series once it's
> merged.  May as well hold off reviewing it until then.
> 
> Ethan
> 
> On Wed, May 29, 2013 at 2:38 PM, Ethan Jackson <ethan at nicira.com> wrote:
> > The logic for updating statistics at the facet level had been
> > spread through ofproto-dpif in a rather confusing manner.  This
> > patch consolidates as much of this logic as is reasonable into
> > facet_push_stats().
> >
> > On a side note, I'd expect this patch to have a marginal positive
> > performance impact when using learning (though I haven't bothered
> > to measure it).  It combines facet_learn() and facet_push_stats()
> > into one step allowing us to avoid a redundant xlate_actions().
> >
> > Signed-off-by: Ethan Jackson <ethan at nicira.com>
> > ---
> >  ofproto/ofproto-dpif.c |  148 ++++++++++++++++--------------------------------
> >  1 file changed, 50 insertions(+), 98 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index ae59eda..881b29f 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -121,7 +121,6 @@ static struct rule_dpif *rule_dpif_miss_rule(struct ofproto_dpif *ofproto,
> >
> >  static void rule_credit_stats(struct rule_dpif *,
> >                                const struct dpif_flow_stats *);
> > -static void flow_push_stats(struct facet *, const struct dpif_flow_stats *);
> >  static tag_type rule_calculate_tag(const struct flow *,
> >                                     const struct minimask *, uint32_t basis);
> >  static void rule_invalidate(const struct rule_dpif *);
> > @@ -325,9 +324,6 @@ static void action_xlate_ctx_init(struct action_xlate_ctx *,
> >  static void xlate_actions(struct action_xlate_ctx *,
> >                            const struct ofpact *ofpacts, size_t ofpacts_len,
> >                            struct ofpbuf *odp_actions);
> > -static void xlate_actions_for_side_effects(struct action_xlate_ctx *,
> > -                                           const struct ofpact *ofpacts,
> > -                                           size_t ofpacts_len);
> >  static void xlate_table_action(struct action_xlate_ctx *, uint16_t in_port,
> >                                 uint8_t table_id, bool may_packet_in);
> >
> > @@ -419,7 +415,6 @@ static void subfacet_destroy_batch(struct ofproto_dpif *,
> >                                     struct subfacet **, int n);
> >  static void subfacet_reset_dp_stats(struct subfacet *,
> >                                      struct dpif_flow_stats *);
> > -static void subfacet_update_time(struct subfacet *, long long int used);
> >  static void subfacet_update_stats(struct subfacet *,
> >                                    const struct dpif_flow_stats *);
> >  static void subfacet_make_actions(struct subfacet *,
> > @@ -520,9 +515,8 @@ static bool facet_check_consistency(struct facet *);
> >
> >  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_push_stats(struct facet *, bool may_learn);
> >  static void facet_learn(struct facet *);
> >  static void facet_account(struct facet *);
> >  static void push_all_stats(void);
> > @@ -4331,26 +4325,29 @@ update_subfacet_stats(struct subfacet *subfacet,
> >                        const struct dpif_flow_stats *stats)
> >  {
> >      struct facet *facet = subfacet->facet;
> > +    struct dpif_flow_stats diff;
> > +
> > +    diff.tcp_flags = stats->tcp_flags;
> > +    diff.used = stats->used;
> >
> >      if (stats->n_packets >= subfacet->dp_packet_count) {
> > -        uint64_t extra = stats->n_packets - subfacet->dp_packet_count;
> > -        facet->packet_count += extra;
> > +        diff.n_packets = stats->n_packets - subfacet->dp_packet_count;
> >      } else {
> >          VLOG_WARN_RL(&rl, "unexpected packet count from the datapath");
> > +        diff.n_packets = 0;
> >      }
> >
> >      if (stats->n_bytes >= subfacet->dp_byte_count) {
> > -        facet->byte_count += stats->n_bytes - subfacet->dp_byte_count;
> > +        diff.n_bytes = stats->n_bytes - subfacet->dp_byte_count;
> >      } else {
> >          VLOG_WARN_RL(&rl, "unexpected byte count from datapath");
> > +        diff.n_bytes = 0;
> >      }
> >
> >      subfacet->dp_packet_count = stats->n_packets;
> >      subfacet->dp_byte_count = stats->n_bytes;
> > +    subfacet_update_stats(subfacet, &diff);
> >
> > -    facet->tcp_flags |= stats->tcp_flags;
> > -
> > -    subfacet_update_time(subfacet, stats->used);
> >      if (facet->accounted_bytes < facet->byte_count) {
> >          facet_learn(facet);
> >          facet_account(facet);
> > @@ -4406,7 +4403,6 @@ update_stats(struct dpif_backer *backer)
> >      while (dpif_flow_dump_next(&dump, &key, &key_len, NULL, NULL, &stats)) {
> >          struct flow flow;
> >          struct subfacet *subfacet;
> > -        struct ofport_dpif *ofport;
> >          uint32_t key_hash;
> >
> >          if (ofproto_receive(backer, NULL, key, key_len, &flow, NULL, &ofproto,
> > @@ -4417,11 +4413,6 @@ update_stats(struct dpif_backer *backer)
> >          ofproto->total_subfacet_count += hmap_count(&ofproto->subfacets);
> >          ofproto->n_update_stats++;
> >
> > -        ofport = get_ofp_port(ofproto, flow.in_port);
> > -        if (ofport && ofport->tnl_port) {
> > -            netdev_vport_inc_rx(ofport->up.netdev, stats);
> > -        }
> > -
> >          key_hash = odp_flow_key_hash(key, key_len);
> >          subfacet = subfacet_find(ofproto, key, key_len, key_hash);
> >          switch (subfacet ? subfacet->path : SF_NOT_INSTALLED) {
> > @@ -4721,11 +4712,7 @@ facet_remove(struct facet *facet)
> >  static void
> >  facet_learn(struct facet *facet)
> >  {
> > -    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
> > -    struct subfacet *subfacet= CONTAINER_OF(list_front(&facet->subfacets),
> > -                                            struct subfacet, list_node);
> >      long long int now = time_msec();
> > -    struct action_xlate_ctx ctx;
> >
> >      if (!facet->has_fin_timeout && now < facet->learn_rl) {
> >          return;
> > @@ -4740,12 +4727,7 @@ facet_learn(struct facet *facet)
> >          return;
> >      }
> >
> > -    action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
> > -                          &subfacet->initial_vals,
> > -                          facet->rule, facet->tcp_flags, NULL);
> > -    ctx.may_learn = true;
> > -    xlate_actions_for_side_effects(&ctx, facet->rule->up.ofpacts,
> > -                                   facet->rule->up.ofpacts_len);
> > +    facet_push_stats(facet, true);
> >  }
> >
> >  static void
> > @@ -4833,7 +4815,7 @@ facet_flush_stats(struct facet *facet)
> >          ovs_assert(!subfacet->dp_packet_count);
> >      }
> >
> > -    facet_push_stats(facet);
> > +    facet_push_stats(facet, false);
> >      if (facet->accounted_bytes < facet->byte_count) {
> >          facet_account(facet);
> >          facet->accounted_bytes = facet->byte_count;
> > @@ -5187,19 +5169,6 @@ facet_revalidate(struct facet *facet)
> >      }
> >  }
> >
> > -/* Updates 'facet''s used time.  Caller is responsible for calling
> > - * facet_push_stats() to update the flows which 'facet' resubmits into. */
> > -static void
> > -facet_update_time(struct facet *facet, long long int used)
> > -{
> > -    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
> > -    if (used > facet->used) {
> > -        facet->used = used;
> > -        ofproto_rule_update_used(&facet->rule->up, used);
> > -        netflow_flow_update_time(ofproto->netflow, &facet->nf_flow, used);
> > -    }
> > -}
> > -
> >  static void
> >  facet_reset_counters(struct facet *facet)
> >  {
> > @@ -5211,7 +5180,7 @@ facet_reset_counters(struct facet *facet)
> >  }
> >
> >  static void
> > -facet_push_stats(struct facet *facet)
> > +facet_push_stats(struct facet *facet, bool may_learn)
> >  {
> >      struct dpif_flow_stats stats;
> >
> > @@ -5222,18 +5191,45 @@ facet_push_stats(struct facet *facet)
> >      stats.n_packets = facet->packet_count - facet->prev_packet_count;
> >      stats.n_bytes = facet->byte_count - facet->prev_byte_count;
> >      stats.used = facet->used;
> > -    stats.tcp_flags = 0;
> > +    stats.tcp_flags = facet->tcp_flags;
> > +
> > +    if (may_learn || stats.n_packets || stats.n_bytes
> > +        || facet->used > facet->prev_used) {
> > +        struct subfacet *subfacet = facet_get_subfacet(facet);
> > +        struct ofproto_dpif *ofproto =
> > +            ofproto_dpif_cast(facet->rule->up.ofproto);
> > +
> > +        uint64_t odp_actions_stub[1024 / 8];
> > +        struct ofpbuf odp_actions;
> > +        struct ofport_dpif *in_port;
> > +        struct action_xlate_ctx ctx;
> >
> > -    if (stats.n_packets || stats.n_bytes || facet->used > facet->prev_used) {
> >          facet->prev_packet_count = facet->packet_count;
> >          facet->prev_byte_count = facet->byte_count;
> >          facet->prev_used = facet->used;
> >
> > -        rule_credit_stats(facet->rule, &stats);
> > -        flow_push_stats(facet, &stats);
> > +        in_port = get_ofp_port(ofproto, facet->flow.in_port);
> > +        if (in_port && in_port->tnl_port) {
> > +            netdev_vport_inc_rx(in_port->up.netdev, &stats);
> > +        }
> >
> > -        update_mirror_stats(ofproto_dpif_cast(facet->rule->up.ofproto),
> > -                            facet->mirrors, stats.n_packets, stats.n_bytes);
> > +        rule_credit_stats(facet->rule, &stats);
> > +        netflow_flow_update_time(ofproto->netflow, &facet->nf_flow,
> > +                                 facet->used);
> > +        netflow_flow_update_flags(&facet->nf_flow, facet->tcp_flags);
> > +        update_mirror_stats(ofproto, facet->mirrors, stats.n_packets,
> > +                            stats.n_bytes);
> > +
> > +        ofpbuf_use_stub(&odp_actions, odp_actions_stub,
> > +                        sizeof odp_actions_stub);
> > +        action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
> > +                              &subfacet->initial_vals, facet->rule,
> > +                              stats.tcp_flags, NULL);
> > +        ctx.resubmit_stats = &stats;
> > +        ctx.may_learn = may_learn;
> > +        xlate_actions(&ctx, facet->rule->up.ofpacts,
> > +                      facet->rule->up.ofpacts_len, &odp_actions);
> > +        ofpbuf_uninit(&odp_actions);
> >      }
> >  }
> >
> > @@ -5251,7 +5247,7 @@ push_all_stats__(bool run_fast)
> >          struct facet *facet;
> >
> >          HMAP_FOR_EACH (facet, hmap_node, &ofproto->facets) {
> > -            facet_push_stats(facet);
> > +            facet_push_stats(facet, false);
> >              if (run_fast) {
> >                  run_fast_rl();
> >              }
> > @@ -5274,25 +5270,6 @@ rule_credit_stats(struct rule_dpif *rule, const struct dpif_flow_stats *stats)
> >      rule->byte_count += stats->n_bytes;
> >      ofproto_rule_update_used(&rule->up, stats->used);
> >  }
> > -
> > -/* Pushes flow statistics to the rules which 'facet->flow' resubmits
> > - * into given 'facet->rule''s actions and mirrors. */
> > -static void
> > -flow_push_stats(struct facet *facet, const struct dpif_flow_stats *stats)
> > -{
> > -    struct rule_dpif *rule = facet->rule;
> > -    struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> > -    struct subfacet *subfacet = facet_get_subfacet(facet);
> > -    struct action_xlate_ctx ctx;
> > -
> > -    ofproto_rule_update_used(&rule->up, stats->used);
> > -
> > -    action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
> > -                          &subfacet->initial_vals, rule, 0, NULL);
> > -    ctx.resubmit_stats = stats;
> > -    xlate_actions_for_side_effects(&ctx, rule->up.ofpacts,
> > -                                   rule->up.ofpacts_len);
> > -}
> >
> >  /* Subfacets. */
> >
> > @@ -5562,23 +5539,13 @@ subfacet_reset_dp_stats(struct subfacet *subfacet,
> >      subfacet->dp_byte_count = 0;
> >  }
> >
> > -/* Updates 'subfacet''s used time.  The caller is responsible for calling
> > - * facet_push_stats() to update the flows which 'subfacet' resubmits into. */
> > -static void
> > -subfacet_update_time(struct subfacet *subfacet, long long int used)
> > -{
> > -    if (used > subfacet->used) {
> > -        subfacet->used = used;
> > -        facet_update_time(subfacet->facet, used);
> > -    }
> > -}
> > -
> >  /* Folds the statistics from 'stats' into the counters in 'subfacet'.
> >   *
> >   * Because of the meaning of a subfacet's counters, it only makes sense to do
> >   * this if 'stats' are not tracked in the datapath, that is, if 'stats'
> >   * represents a packet that was sent by hand or if it represents statistics
> >   * that have been cleared out of the datapath. */
> > +/* TODO does this function have the right name? */
> >  static void
> >  subfacet_update_stats(struct subfacet *subfacet,
> >                        const struct dpif_flow_stats *stats)
> > @@ -5586,11 +5553,11 @@ subfacet_update_stats(struct subfacet *subfacet,
> >      if (stats->n_packets || stats->used > subfacet->used) {
> >          struct facet *facet = subfacet->facet;
> >
> > -        subfacet_update_time(subfacet, stats->used);
> > +        subfacet->used = MAX(subfacet->used, stats->used);
> > +        facet->used = MAX(facet->used, stats->used);
> >          facet->packet_count += stats->n_packets;
> >          facet->byte_count += stats->n_bytes;
> >          facet->tcp_flags |= stats->tcp_flags;
> > -        netflow_flow_update_flags(&facet->nf_flow, stats->tcp_flags);
> >      }
> >  }
> >
> > @@ -7185,21 +7152,6 @@ xlate_actions(struct action_xlate_ctx *ctx,
> >      ofpbuf_uninit(&ctx->stack);
> >  }
> >
> > -/* Translates the 'ofpacts_len' bytes of "struct ofpact"s starting at 'ofpacts'
> > - * into datapath actions, using 'ctx', and discards the datapath actions. */
> > -static void
> > -xlate_actions_for_side_effects(struct action_xlate_ctx *ctx,
> > -                               const struct ofpact *ofpacts,
> > -                               size_t ofpacts_len)
> > -{
> > -    uint64_t odp_actions_stub[1024 / 8];
> > -    struct ofpbuf odp_actions;
> > -
> > -    ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
> > -    xlate_actions(ctx, ofpacts, ofpacts_len, &odp_actions);
> > -    ofpbuf_uninit(&odp_actions);
> > -}
> > -
> >  static void
> >  xlate_report(struct action_xlate_ctx *ctx, const char *s)
> >  {
> > --
> > 1.7.9.5
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list