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 <et...@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 <et...@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@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
