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 <b...@nicira.com> wrote:
> From: Ben Pfaff <b...@hardrock.nicira.com>
>
> Signed-off-by: Ben Pfaff <b...@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
