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 <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
