[ovs-dev] [xlate v2 4/6] ofproto-dpif: Move odp_actions from subfacet to facet.

[Ben Pfaff]

On Thu, May 23, 2013 at 05:15:18PM -0700, Ethan Jackson wrote:
> Upon close inspection, it appears that it's not possible for
> actions to differ between subfacets belonging to a given facet.
> Given this fact, it makes sense to move datapath actions from
> subfacets to their parent facets.  It's both conceptually more
> straightforward, and necessary for future threading and megaflow
> work.
> 
> Co-authored-by: Justin Pettit <jpettit at nicira.com>
> Signed-off-by: Ethan Jackson <ethan at nicira.com>

There's something funny going on with rule_dpif_lookup() calls here.
handle_flow_miss() calls rule_dpif_lookup().  It only uses it in the
handle_flow_miss_without_facet() case, and only to pass it to
handle_flow_miss_without_facet().  In the
handle_flow_miss_with_facet() case, it still calls rule_dpif_lookup(),
but it throws away the result without ever looking at it at all, and
then facet_create() calls rule_dpif_lookup() again.

Previously, in creating a facet+subfacet for a slow-path flow due to
one packet showing up, we would do exactly one xlate.  Now, I believe
we do one xlate in facet_create() and then one xlate in the loop in
handle_flow_miss_with_facet().  I don't think the difference in
performance is significant, because xlate of slow-path flows should be
cheap, but I thought it'd mention anyway in case you hadn't noticed
the change.

I found the new version of facet_check_consistency() hard to follow.
How about something more like this?

static bool
facet_check_consistency(struct facet *facet)
{
    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 15);

    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);

    uint64_t odp_actions_stub[1024 / 8];
    struct ofpbuf odp_actions;

    struct action_xlate_ctx ctx;
    struct rule_dpif *rule;
    bool same_actions, same_slow;
    bool ok;

    /* Check the rule for consistency. */
    rule = rule_dpif_lookup(ofproto, &facet->flow);
    if (rule != facet->rule) {
        if (!VLOG_DROP_WARN(&rl)) {
            struct ds s = DS_EMPTY_INITIALIZER;

            flow_format(&s, &facet->flow);
            ds_put_format(&s, ": facet associated with wrong rule (was "
                          "table=%"PRIu8",", facet->rule->up.table_id);
            cls_rule_format(&facet->rule->up.cr, &s);
            ds_put_format(&s, ") (should have been table=%"PRIu8",",
                          rule->up.table_id);
            cls_rule_format(&rule->up.cr, &s);
            ds_put_cstr(&s, ")\n");

            ds_destroy(&s);
        }
        return false;
    }

    /* Check the datapath actions for consistency. */
    ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
    action_xlate_ctx_init(&ctx, ofproto, &facet->flow, &facet->initial_vals,
                          rule, 0, NULL);
    xlate_actions(&ctx, rule->up.ofpacts, rule->up.ofpacts_len, &odp_actions);

    same_actions = ofpbuf_equal(&facet->odp_actions, &odp_actions);
    same_slow = facet->slow == ctx.slow;
    ok = same_actions && same_slow;
    if (!ok && !VLOG_DROP_WARN(&rl)) {
        struct ds s = DS_EMPTY_INITIALIZER;

        flow_format(&s, &facet->flow);
        ds_put_cstr(&s, ": inconsistency in facet");

        if (!ofpbuf_equal(&facet->odp_actions, &odp_actions)) {
            ds_put_cstr(&s, " (actions were: ");
            format_odp_actions(&s, facet->odp_actions.data,
                               facet->odp_actions.size);
            ds_put_cstr(&s, ") (correct actions: ");
            format_odp_actions(&s, odp_actions.data, odp_actions.size);
            ds_put_cstr(&s, ")");
        }

        if (facet->slow != ctx.slow) {
            ds_put_format(&s, " slow path incorrect. should be %s",
                          ctx.slow ? "true" : "false");
        }

        ds_destroy(&s);
    }
    ofpbuf_uninit(&odp_actions);
    return ok;
}

On the same topic, when facet->slow != ctx.slow, it would probably be
better to tell the user what the values were, since slow has multiple
possible values, instead of just that it was wrong and whether it
should have been nonzero.

I think I see an opportunity for improvement in facet_lookup_valid():
it would be cheaper if facet_revalidate()'s return value indicated
whether the facet had been removed.  (This isn't new.)  Something like
this:

    facet = facet_find(ofproto, flow, hash);
    if (facet
        && (ofproto->backer->need_revalidate
            || tag_set_intersects(&ofproto->backer->revalidate_set,
                                  facet->tags))
        && !facet_revalidate(facet)) {
        facet = NULL;
    }

Thanks,

Ben.