Btw, this version is based on master, and is ready for review.
Thanks,
Ethan
On Wed, May 29, 2013 at 5:43 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 | 114
> +++++++++++++++++-------------------------------
> 1 file changed, 40 insertions(+), 74 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 386434d..40be3c8 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 *);
> @@ -420,7 +419,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 int subfacet_install(struct subfacet *,
> @@ -508,9 +506,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);
> @@ -4312,26 +4309,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);
> @@ -4387,7 +4387,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,
> @@ -4398,11 +4397,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) {
> @@ -4713,9 +4707,7 @@ facet_remove(struct facet *facet)
> static void
> facet_learn(struct facet *facet)
> {
> - struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
> long long int now = time_msec();
> - struct xlate_in xin;
>
> if (!facet->xout.has_fin_timeout && now < facet->learn_rl) {
> return;
> @@ -4730,10 +4722,7 @@ facet_learn(struct facet *facet)
> return;
> }
>
> - xlate_in_init(&xin, ofproto, &facet->flow, &facet->initial_vals,
> - facet->rule, facet->tcp_flags, NULL);
> - xin.may_learn = true;
> - xlate_actions_for_side_effects(&xin);
> + facet_push_stats(facet, true);
> }
>
> static void
> @@ -4820,7 +4809,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;
> @@ -5061,19 +5050,6 @@ facet_revalidate(struct facet *facet)
> return true;
> }
>
> -/* 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)
> {
> @@ -5085,7 +5061,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;
>
> @@ -5096,19 +5072,37 @@ 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 ofproto_dpif *ofproto =
> + ofproto_dpif_cast(facet->rule->up.ofproto);
> +
> + struct ofport_dpif *in_port;
> + struct xlate_in xin;
>
> - 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->xout.mirrors, stats.n_packets,
> + 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->xout.mirrors, stats.n_packets,
> stats.n_bytes);
> +
> + xlate_in_init(&xin, ofproto, &facet->flow, &facet->initial_vals,
> + facet->rule, stats.tcp_flags, NULL);
> + xin.resubmit_stats = &stats;
> + xin.may_learn = may_learn;
> + xlate_actions_for_side_effects(&xin);
> }
> }
>
> @@ -5126,7 +5120,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();
> }
> @@ -5149,23 +5143,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 xlate_in xin;
> -
> - ofproto_rule_update_used(&rule->up, stats->used);
> -
> - xlate_in_init(&xin, ofproto, &facet->flow, &facet->initial_vals, rule,
> - 0, NULL);
> - xin.resubmit_stats = stats;
> - xlate_actions_for_side_effects(&xin);
> -}
>
> /* Subfacets. */
>
> @@ -5386,17 +5363,6 @@ 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
> @@ -5410,11 +5376,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);
> }
> }
>
> --
> 1.7.9.5
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
