[ovs-dev] [slow path 10/11] ofproto-dpif: Introduce "slow path" datapath flows.
Ethan Jackson
ethan at nicira.com
Tue May 8 23:57:31 UTC 2012
High level comment:
Any particular reason we need to use a bitmask to store the slow path
reason instead of a simple enum? It seems to me that every slow path
reason is mutually exclusive. Would doing this simplify the code? I
suppose the one exception I see is that perhaps something could be
both lacp and match, for example. But in this case, just reporting it
as lacp seems fine to me. Of course, if the code is simpler as it is
here, the current approach seems fine as well.
> + for (bit = 1; ; bit <<= 1) {
Here I think we want "for (bit = 1; bit; bit <<= 1) {"
> + /* The actions for slow-path flows may legitimately vary from one
> + * packet to the next. We're done. */
Is this true? Seems to me that it's either something like a
lacp/stp/cfm packet in which case the actions are simply "userspace",
or its a slow_match in which case the actions are dictated by the
openflow table and very on a per flow not per-packet basis. I may be
missing something though.
Looks good, thank you.
Ethan
> continue;
> }
>
> - actions_changed = (subfacet->actions_len != odp_actions.size
> - || memcmp(subfacet->actions, odp_actions.data,
> - subfacet->actions_len));
> - if (should_install == subfacet->installed && !actions_changed) {
> + if (!subfacet_should_install(subfacet, subfacet->slow, &odp_actions)) {
> continue;
> }
>
> @@ -3818,16 +3925,15 @@ facet_check_consistency(struct facet *facet)
> odp_flow_key_format(key.data, key.size, &s);
>
> ds_put_cstr(&s, ": inconsistency in subfacet");
> - if (should_install != subfacet->installed) {
> + if (want_path != subfacet->path) {
> enum odp_key_fitness fitness = subfacet->key_fitness;
>
> - ds_put_format(&s, " (should%s have been installed)",
> - should_install ? "" : " not");
> - ds_put_format(&s, " (may_set_up_flow=%s, fitness=%s)",
> - ctx.may_set_up_flow ? "true" : "false",
> + ds_put_format(&s, " (%s, fitness=%s)",
> + subfacet_path_to_string(subfacet->path),
> odp_key_fitness_to_string(fitness));
> - }
> - if (actions_changed) {
> + ds_put_format(&s, " (should have been %s)",
> + subfacet_path_to_string(want_path));
> + } else if (want_path == SF_FAST_PATH) {
> ds_put_cstr(&s, " (actions were: ");
> format_odp_actions(&s, subfacet->actions,
> subfacet->actions_len);
> @@ -3871,7 +3977,6 @@ facet_revalidate(struct facet *facet)
>
> struct rule_dpif *new_rule;
> struct subfacet *subfacet;
> - bool actions_changed;
> int i;
>
> COVERAGE_INC(facet_revalidate);
> @@ -3891,28 +3996,20 @@ facet_revalidate(struct facet *facet)
> memset(&ctx, 0, sizeof ctx);
> ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
> LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
> - bool should_install;
> + enum slow_path_reason slow;
>
> action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
> subfacet->initial_tci, new_rule, 0, NULL);
> xlate_actions(&ctx, new_rule->up.actions, new_rule->up.n_actions,
> &odp_actions);
> - actions_changed = (subfacet->actions_len != odp_actions.size
> - || memcmp(subfacet->actions, odp_actions.data,
> - subfacet->actions_len));
> -
> - should_install = (ctx.may_set_up_flow
> - && subfacet->key_fitness != ODP_FIT_TOO_LITTLE);
> - if (actions_changed || should_install != subfacet->installed) {
> - if (should_install) {
> - struct dpif_flow_stats stats;
>
> - subfacet_install(subfacet,
> - odp_actions.data, odp_actions.size, &stats);
> - subfacet_update_stats(subfacet, &stats);
> - } else {
> - subfacet_uninstall(subfacet);
> - }
> + slow = (subfacet->slow & SLOW_MATCH) | ctx.slow;
> + if (subfacet_should_install(subfacet, slow, &odp_actions)) {
> + struct dpif_flow_stats stats;
> +
> + subfacet_install(subfacet,
> + odp_actions.data, odp_actions.size, &stats, slow);
> + subfacet_update_stats(subfacet, &stats);
>
> if (!new_actions) {
> new_actions = xcalloc(list_size(&facet->subfacets),
> @@ -3934,23 +4031,24 @@ facet_revalidate(struct facet *facet)
> /* Update 'facet' now that we've taken care of all the old state. */
> facet->tags = ctx.tags;
> facet->nf_flow.output_iface = ctx.nf_output_iface;
> - facet->may_install = ctx.may_set_up_flow;
> facet->has_learn = ctx.has_learn;
> facet->has_normal = ctx.has_normal;
> facet->has_fin_timeout = ctx.has_fin_timeout;
> facet->mirrors = ctx.mirrors;
> - if (new_actions) {
> - i = 0;
> - LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
> - if (new_actions[i].odp_actions) {
> - free(subfacet->actions);
> - subfacet->actions = new_actions[i].odp_actions;
> - subfacet->actions_len = new_actions[i].actions_len;
> - }
> - i++;
> +
> + i = 0;
> + LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
> + subfacet->slow = (subfacet->slow & SLOW_MATCH) | ctx.slow;
> +
> + if (new_actions && new_actions[i].odp_actions) {
> + free(subfacet->actions);
> + subfacet->actions = new_actions[i].odp_actions;
> + subfacet->actions_len = new_actions[i].actions_len;
> }
> - free(new_actions);
> + i++;
> }
> + free(new_actions);
> +
> if (facet->rule != new_rule) {
> COVERAGE_INC(facet_changed_rule);
> list_remove(&facet->list_node);
> @@ -4102,7 +4200,10 @@ subfacet_create(struct facet *facet, enum odp_key_fitness key_fitness,
> subfacet->dp_byte_count = 0;
> subfacet->actions_len = 0;
> subfacet->actions = NULL;
> - subfacet->installed = false;
> + subfacet->slow = (subfacet->key_fitness == ODP_FIT_TOO_LITTLE
> + ? SLOW_MATCH
> + : 0);
> + subfacet->path = SF_NOT_INSTALLED;
> subfacet->initial_tci = initial_tci;
>
> return subfacet;
> @@ -4191,13 +4292,13 @@ subfacet_make_actions(struct subfacet *subfacet, const struct ofpbuf *packet,
> rule, 0, packet);
> xlate_actions(&ctx, rule->up.actions, rule->up.n_actions, odp_actions);
> facet->tags = ctx.tags;
> - facet->may_install = ctx.may_set_up_flow;
> facet->has_learn = ctx.has_learn;
> facet->has_normal = ctx.has_normal;
> facet->has_fin_timeout = ctx.has_fin_timeout;
> facet->nf_flow.output_iface = ctx.nf_output_iface;
> facet->mirrors = ctx.mirrors;
>
> + subfacet->slow = (subfacet->slow & SLOW_MATCH) | ctx.slow;
> if (subfacet->actions_len != odp_actions->size
> || memcmp(subfacet->actions, odp_actions->data, odp_actions->size)) {
> free(subfacet->actions);
> @@ -4215,10 +4316,13 @@ subfacet_make_actions(struct subfacet *subfacet, const struct ofpbuf *packet,
> static int
> subfacet_install(struct subfacet *subfacet,
> const struct nlattr *actions, size_t actions_len,
> - struct dpif_flow_stats *stats)
> + struct dpif_flow_stats *stats,
> + enum slow_path_reason slow)
> {
> struct facet *facet = subfacet->facet;
> struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
> + enum subfacet_path path = subfacet_want_path(slow);
> + uint64_t slow_path_stub[128 / 8];
> struct odputil_keybuf keybuf;
> enum dpif_flow_put_flags flags;
> struct ofpbuf key;
> @@ -4229,6 +4333,12 @@ subfacet_install(struct subfacet *subfacet,
> flags |= DPIF_FP_ZERO_STATS;
> }
>
> + if (path == SF_SLOW_PATH) {
> + compose_slow_path(ofproto, &facet->flow, slow,
> + slow_path_stub, sizeof slow_path_stub,
> + &actions, &actions_len);
> + }
> +
> subfacet_get_key(subfacet, &keybuf, &key);
> ret = dpif_flow_put(ofproto->dpif, flags, key.data, key.size,
> actions, actions_len, stats);
> @@ -4237,14 +4347,24 @@ subfacet_install(struct subfacet *subfacet,
> subfacet_reset_dp_stats(subfacet, stats);
> }
>
> + if (!ret) {
> + subfacet->path = path;
> + }
> return ret;
> }
>
> +static int
> +subfacet_reinstall(struct subfacet *subfacet, struct dpif_flow_stats *stats)
> +{
> + return subfacet_install(subfacet, subfacet->actions, subfacet->actions_len,
> + stats, subfacet->slow);
> +}
> +
> /* If 'subfacet' is installed in the datapath, uninstalls it. */
> static void
> subfacet_uninstall(struct subfacet *subfacet)
> {
> - if (subfacet->installed) {
> + if (subfacet->path != SF_NOT_INSTALLED) {
> struct rule_dpif *rule = subfacet->facet->rule;
> struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> struct odputil_keybuf keybuf;
> @@ -4258,7 +4378,7 @@ subfacet_uninstall(struct subfacet *subfacet)
> if (!error) {
> subfacet_update_stats(subfacet, &stats);
> }
> - subfacet->installed = false;
> + subfacet->path = SF_NOT_INSTALLED;
> } else {
> assert(subfacet->dp_packet_count == 0);
> assert(subfacet->dp_byte_count == 0);
> @@ -4575,6 +4695,35 @@ static void do_xlate_actions(const union ofp_action *in, size_t n_in,
> struct action_xlate_ctx *ctx);
> static void xlate_normal(struct action_xlate_ctx *);
>
> +/* Composes an ODP action for a "slow path" action for 'flow' within 'ofproto'.
> + * The action will state 'slow' as the reason that the action is in the slow
> + * path. (This is purely informational: it allows a human viewing "ovs-dpctl
> + * dump-flows" output to see why a flow is in the slow path.)
> + *
> + * The 'stub_size' bytes in 'stub' will be used to store the action.
> + * 'stub_size' must be large enough for the action.
> + *
> + * The action and its size will be stored in '*actionsp' and '*actions_lenp',
> + * respectively. */
> +static void
> +compose_slow_path(const struct ofproto_dpif *ofproto, const struct flow *flow,
> + enum slow_path_reason slow,
> + uint64_t *stub, size_t stub_size,
> + const struct nlattr **actionsp, size_t *actions_lenp)
> +{
> + union user_action_cookie cookie;
> + struct ofpbuf buf;
> +
> + cookie.type = USER_ACTION_COOKIE_SLOW_PATH;
> + cookie.slow_path.unused = 0;
> + cookie.slow_path.reason = slow;
> +
> + ofpbuf_use_stack(&buf, stub, stub_size);
> + put_userspace_action(ofproto, &buf, flow, &cookie);
> + *actionsp = buf.data;
> + *actions_lenp = buf.size;
> +}
> +
> static size_t
> put_userspace_action(const struct ofproto_dpif *ofproto,
> struct ofpbuf *odp_actions,
> @@ -4844,7 +4993,7 @@ execute_controller_action(struct action_xlate_ctx *ctx, int len,
> struct ofputil_packet_in pin;
> struct ofpbuf *packet;
>
> - ctx->may_set_up_flow = false;
> + ctx->slow |= SLOW_CONTROLLER;
> if (!ctx->packet) {
> return;
> }
> @@ -5397,6 +5546,8 @@ xlate_actions(struct action_xlate_ctx *ctx,
> * tracing purposes. */
> static bool hit_resubmit_limit;
>
> + enum slow_path_reason special;
> +
> COVERAGE_INC(ofproto_dpif_xlate);
>
> ofpbuf_clear(odp_actions);
> @@ -5404,7 +5555,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
>
> ctx->odp_actions = odp_actions;
> ctx->tags = 0;
> - ctx->may_set_up_flow = true;
> + ctx->slow = 0;
> ctx->has_learn = false;
> ctx->has_normal = false;
> ctx->has_fin_timeout = false;
> @@ -5449,8 +5600,9 @@ xlate_actions(struct action_xlate_ctx *ctx,
> }
> }
>
> - if (process_special(ctx->ofproto, &ctx->flow, ctx->packet)) {
> - ctx->may_set_up_flow = false;
> + special = process_special(ctx->ofproto, &ctx->flow, ctx->packet);
> + if (special) {
> + ctx->slow |= special;
> } else {
> static struct vlog_rate_limit trace_rl = VLOG_RATE_LIMIT_INIT(1, 1);
> ovs_be16 initial_tci = ctx->base_flow.vlan_tci;
> @@ -5477,7 +5629,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
> if (!connmgr_may_set_up_flow(ctx->ofproto->up.connmgr, &ctx->flow,
> ctx->odp_actions->data,
> ctx->odp_actions->size)) {
> - ctx->may_set_up_flow = false;
> + ctx->slow |= SLOW_IN_BAND;
> if (ctx->packet
> && connmgr_msg_in_hook(ctx->ofproto->up.connmgr, &ctx->flow,
> ctx->packet)) {
> @@ -6278,11 +6430,10 @@ send_active_timeout(struct ofproto_dpif *ofproto, struct facet *facet)
> struct ofexpired expired;
>
> LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
> - if (subfacet->installed) {
> + if (subfacet->path == SF_FAST_PATH) {
> struct dpif_flow_stats stats;
>
> - subfacet_install(subfacet, subfacet->actions,
> - subfacet->actions_len, &stats);
> + subfacet_reinstall(subfacet, &stats);
> subfacet_update_stats(subfacet, &stats);
> }
> }
> @@ -6582,12 +6733,49 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
> format_odp_actions(ds, odp_actions.data, odp_actions.size);
> ofpbuf_uninit(&odp_actions);
>
> - if (!trace.ctx.may_set_up_flow) {
> - if (packet) {
> - ds_put_cstr(ds, "\nThis flow is not cachable.");
> - } else {
> - ds_put_cstr(ds, "\nThe datapath actions are incomplete--"
> - "for complete actions, please supply a packet.");
> + if (trace.ctx.slow) {
> + enum slow_path_reason slow;
> +
> + ds_put_cstr(ds, "\nThis flow is handled by the userspace "
> + "slow path because it:");
> + for (slow = trace.ctx.slow; slow; ) {
> + enum slow_path_reason bit = rightmost_1bit(slow);
> +
> + switch (bit) {
> + case SLOW_CONTROLLER:
> + ds_put_cstr(ds, "\n\t- Sends \"packet-in\" messages "
> + "to the OpenFlow controller.");
> + break;
> + case SLOW_CFM:
> + ds_put_cstr(ds, "\n\t- Consists of CFM packets.");
> + break;
> + case SLOW_LACP:
> + ds_put_cstr(ds, "\n\t- Consists of LACP packets.");
> + break;
> + case SLOW_STP:
> + ds_put_cstr(ds, "\n\t- Consists of STP packets.");
> + break;
> + case SLOW_IN_BAND:
> + ds_put_cstr(ds, "\n\t- Needs in-band special case "
> + "processing.");
> + if (!packet) {
> + ds_put_cstr(ds, "\n\t (The datapath actions are "
> + "incomplete--for complete actions, "
> + "please supply a packet.)");
> + }
> + break;
> + case SLOW_MATCH:
> + ds_put_cstr(ds, "\n\t- Needs more specific matching "
> + "than the datapath supports.");
> + break;
> + }
> +
> + slow &= ~bit;
> + }
> +
> + if (slow & ~SLOW_MATCH) {
> + ds_put_cstr(ds, "\nThe datapath actions above do not reflect "
> + "the special slow-path processing.");
> }
> }
> }
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 2c5df96..3dab43e 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -78,9 +78,10 @@ table=1 in_port=1 action=dec_ttl,output:3
> ])
> AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> AT_CHECK([ovs-appctl ofproto/trace br0 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=2,frag=no)' -generate], [0], [stdout])
> -AT_CHECK([tail -2 stdout], [0],
> +AT_CHECK([tail -3 stdout], [0],
> [Datapath actions: set(ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=1,frag=no)),2,4
> -This flow is not cachable.
> +This flow is handled by the userspace slow path because it:
> + - Sends "packet-in" messages to the OpenFlow controller.
> ])
> AT_CHECK([ovs-appctl ofproto/trace br0 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=3,frag=no)'], [0], [stdout])
> AT_CHECK([tail -1 stdout], [0],
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list