From ba33dd0354e9accf909d8ccd920b319d4d476112 Mon Sep 17 00:00:00 2001 From: Ethan Jackson Date: Fri, 17 May 2013 13:39:03 -0700 Subject: [PATCH] ofproto-dpif: Ditch SLOW_MATCH slow path reason. Before this patch, datapath keys with ODP_FIT_TO_LITTLE, would be assigned subfacets and installed in the kernel with a SLOW_MATCH slow path reason. This is problematic, because these flow keys can't be reliable converted into a 'struct flow' thus breaking a fundamental assumption of ofproto-dpif. This patch circumvents the issue by skipping facet creation for these flows altogether. This approach has the added benefit of simplifying the code for future patches. Signed-off-by: Ethan Jackson --- lib/odp-util.c | 2 -- lib/odp-util.h | 4 ---- ofproto/ofproto-dpif.c | 34 +++++++++++++--------------------- tests/odp.at | 1 - 4 files changed, 13 insertions(+), 28 deletions(-) diff --git a/lib/odp-util.c b/lib/odp-util.c index 5bf94fe3d..aa50333d5 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -187,8 +187,6 @@ slow_path_reason_to_string(uint32_t data) return "bfd"; case SLOW_CONTROLLER: return "controller"; - case SLOW_MATCH: - return "match"; default: return NULL; } diff --git a/lib/odp-util.h b/lib/odp-util.h index a981d1760..a2f2b14db 100644 --- a/lib/odp-util.h +++ b/lib/odp-util.h @@ -184,10 +184,6 @@ enum slow_path_reason { /* Mutually exclusive with SLOW_BFD, SLOW_CFM, SLOW_LACP, SLOW_STP. * Could possibly appear with SLOW_IN_BAND. */ SLOW_CONTROLLER = 1 << 5, /* Packets must go to OpenFlow controller. */ - - /* This can appear on its own, or, theoretically at least, along with any - * other combination of reasons. */ - SLOW_MATCH = 1 << 6, /* Datapath can't match specifically enough. */ }; #endif /* odp-util.h */ diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 01eeed998..3aac7520f 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3810,7 +3810,13 @@ handle_flow_miss(struct flow_miss *miss, struct flow_miss_op *ops, if (!facet) { struct rule_dpif *rule = rule_dpif_lookup(ofproto, &miss->flow); - if (!flow_miss_should_make_facet(ofproto, miss, hash)) { + /* There does not exist a bijection between 'struct flow' and datapath + * flow keys with fitness ODP_FIT_TO_LITTLE. This breaks a fundamental + * assumption used throughout the facet and subfacet handling code. + * Since we have to handle these misses in userspace anyway, we simply + * skip facet creation, avoiding the problem alltogether. */ + if (miss->key_fitness == ODP_FIT_TOO_LITTLE + || !flow_miss_should_make_facet(ofproto, miss, hash)) { handle_flow_miss_without_facet(miss, rule, ops, n_ops); return; } @@ -5124,19 +5130,16 @@ 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) { - enum slow_path_reason slow; - action_xlate_ctx_init(&ctx, ofproto, &facet->flow, &subfacet->initial_vals, new_rule, 0, NULL); xlate_actions(&ctx, new_rule->up.ofpacts, new_rule->up.ofpacts_len, &odp_actions); - slow = (subfacet->slow & SLOW_MATCH) | ctx.slow; - if (subfacet_should_install(subfacet, slow, &odp_actions)) { + if (subfacet_should_install(subfacet, ctx.slow, &odp_actions)) { struct dpif_flow_stats stats; - subfacet_install(subfacet, - odp_actions.data, odp_actions.size, &stats, slow); + subfacet_install(subfacet, odp_actions.data, odp_actions.size, + &stats, ctx.slow); subfacet_update_stats(subfacet, &stats); if (!new_actions) { @@ -5166,7 +5169,7 @@ facet_revalidate(struct facet *facet) i = 0; LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) { - subfacet->slow = (subfacet->slow & SLOW_MATCH) | ctx.slow; + subfacet->slow = ctx.slow; if (new_actions && new_actions[i].odp_actions) { free(subfacet->actions); @@ -5365,9 +5368,7 @@ subfacet_create(struct facet *facet, struct flow_miss *miss, subfacet->dp_byte_count = 0; subfacet->actions_len = 0; subfacet->actions = NULL; - subfacet->slow = (subfacet->key_fitness == ODP_FIT_TOO_LITTLE - ? SLOW_MATCH - : 0); + subfacet->slow = 0; subfacet->path = SF_NOT_INSTALLED; subfacet->initial_vals = miss->initial_vals; subfacet->odp_in_port = miss->odp_in_port; @@ -5462,7 +5463,7 @@ subfacet_make_actions(struct subfacet *subfacet, const struct ofpbuf *packet, facet->nf_flow.output_iface = ctx.nf_output_iface; facet->mirrors = ctx.mirrors; - subfacet->slow = (subfacet->slow & SLOW_MATCH) | ctx.slow; + subfacet->slow = ctx.slow; if (subfacet->actions_len != odp_actions->size || memcmp(subfacet->actions, odp_actions->data, odp_actions->size)) { free(subfacet->actions); @@ -8329,19 +8330,10 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow, ds_put_cstr(ds, "\n\t- Sends \"packet-in\" messages " "to the OpenFlow controller."); 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/odp.at b/tests/odp.at index a6bcdf54c..fd6a192ee 100644 --- a/tests/odp.at +++ b/tests/odp.at @@ -87,7 +87,6 @@ userspace(pid=555666777) userspace(pid=6633,sFlow(vid=9,pcp=7,output=10)) userspace(pid=9765,slow_path()) userspace(pid=9765,slow_path(cfm)) -userspace(pid=9765,slow_path(cfm,match)) userspace(pid=1234567,userdata(0102030405060708090a0b0c0d0e0f)) userspace(pid=6633,flow_sample(probability=123,collector_set_id=1234,obs_domain_id=2345,obs_point_id=3456)) userspace(pid=6633,ipfix) -- 2.20.1