On Wed, Mar 12, 2014 at 03:21:34PM +0900, Simon Horman wrote:
> This reworks lookup of rules for both table 0 and table action translation.
> The result is that Table Mod settings, which can alter the miss-behaviour
> of tables, including table 0, on a per-table basis may be honoured.
>
> Previous patches proposed by myself which build on earlier merged patches
> by Andy Zhou implement the ofproto side of Table Mod. So with this patch
> the feature should be complete.
>
> Neither this patch, nor any other patches it builds on, alter the default
> behaviour of Open vSwitch. And in particular the OpenFlow1.1 behaviour is
> the default regardless of which OpenFlow version is negotiated between the
> switch and the controller.
>
> An implementation detail, which lends itself to future work, is the
> handling of OFPTC_TABLE_MISS_CONTINUE. If a table has this behaviour set by
> Table Mod and a miss occurs then a loop is created, skipping to the next
> table. It is quite easy to create a situation where this loop covers ~255
> tables which is very expensive as the lookup for each table involves taking
> locks, amongst other things.
>
> Cc: Andy Zhou <az...@nicira.com>
> Signed-off-by: Simon Horman <ho...@verge.net.au>
Thanks Simon.
I found the comment on rule_dpif_lookup_from_table() a little hard to
follow, and the naming of force_controller_on_miss kind of confusing, so
I changed both (for the latter, I reversed the meaning), and I found a
few other ways to make the code easier to understand. I'm folding in
the following diff. I'm going to run the testsuite one more time and
then apply this.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 379d8e7..9d7c752 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -230,7 +230,7 @@ static void xlate_actions__(struct xlate_in *, struct
xlate_out *)
static void xlate_report(struct xlate_ctx *, const char *);
static void xlate_table_action(struct xlate_ctx *, ofp_port_t in_port,
uint8_t table_id, bool may_packet_in,
- bool force_controller_on_miss);
+ bool honor_table_miss);
static bool input_vid_is_valid(uint16_t vid, struct xbundle *, bool warn);
static uint16_t input_vid_to_vlan(const struct xbundle *, uint16_t vid);
static void output_normal(struct xlate_ctx *, const struct xbundle *,
@@ -1737,14 +1737,14 @@ compose_output_action__(struct xlate_ctx *ctx,
ofp_port_t ofp_port,
ctx->xout->slow |= special;
} else if (may_receive(peer, ctx)) {
if (xport_stp_forward_state(peer)) {
- xlate_table_action(ctx, flow->in_port.ofp_port, 0, true,
false);
+ xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true);
} else {
/* Forwarding is disabled by STP. Let OFPP_NORMAL and the
* learning action look at the packet, then drop it. */
struct flow old_base_flow = ctx->base_flow;
size_t old_size = ctx->xout->odp_actions.size;
mirror_mask_t old_mirrors = ctx->xout->mirrors;
- xlate_table_action(ctx, flow->in_port.ofp_port, 0, true,
false);
+ xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true);
ctx->xout->mirrors = old_mirrors;
ctx->base_flow = old_base_flow;
ctx->xout->odp_actions.size = old_size;
@@ -1880,7 +1880,7 @@ xlate_resubmit_resource_check(struct xlate_ctx *ctx)
static void
xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
- bool may_packet_in, bool force_controller_on_miss)
+ bool may_packet_in, bool honor_table_miss)
{
if (xlate_resubmit_resource_check(ctx)) {
ofp_port_t old_in_port = ctx->xin->flow.in_port.ofp_port;
@@ -1900,7 +1900,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t
in_port, uint8_t table_id,
&ctx->xin->flow,
!skip_wildcards
? &ctx->xout->wc : NULL,
- force_controller_on_miss,
+ honor_table_miss,
&ctx->table_id, &rule);
ctx->xin->flow.in_port.ofp_port = old_in_port;
@@ -1917,7 +1917,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t
in_port, uint8_t table_id,
xport = get_ofp_port(ctx->xbridge,
ctx->xin->flow.in_port.ofp_port);
- config = xport->config;
+ config = xport ? xport->config : 0;
break;
}
/* Fall through to drop */
@@ -2092,7 +2092,7 @@ xlate_ofpact_resubmit(struct xlate_ctx *ctx,
table_id = ctx->table_id;
}
- xlate_table_action(ctx, in_port, table_id, false, true);
+ xlate_table_action(ctx, in_port, table_id, false, false);
}
static void
@@ -2300,7 +2300,7 @@ xlate_output_action(struct xlate_ctx *ctx,
break;
case OFPP_TABLE:
xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
- 0, may_packet_in, false);
+ 0, may_packet_in, true);
break;
case OFPP_NORMAL:
xlate_normal(ctx);
@@ -2813,7 +2813,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
ovs_assert(ctx->table_id < ogt->table_id);
xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
- ogt->table_id, true, false);
+ ogt->table_id, true, true);
break;
}
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 17d7584..cb30142 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1095,6 +1095,7 @@ add_internal_flow(struct ofproto_dpif *ofproto, int id,
const struct ofpbuf *ofpacts, struct rule_dpif **rulep)
{
struct ofputil_flow_mod fm;
+ struct classifier *cls;
int error;
match_init_catchall(&fm.match);
@@ -1121,12 +1122,12 @@ add_internal_flow(struct ofproto_dpif *ofproto, int id,
return error;
}
- if (rule_dpif_lookup_in_table(ofproto, &fm.match.flow, NULL, TBL_INTERNAL,
- rulep)) {
- rule_dpif_unref(*rulep);
- } else {
- OVS_NOT_REACHED();
- }
+ cls = &ofproto->up.tables[TBL_INTERNAL].cls;
+ fat_rwlock_rdlock(&cls->rwlock);
+ *rulep = rule_dpif_cast(rule_from_cls_rule(
+ classifier_lookup(cls, &fm.match.flow, NULL)));
+ ovs_assert(*rulep != NULL);
+ fat_rwlock_unlock(&cls->rwlock);
return 0;
}
@@ -3071,7 +3072,7 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, const
struct flow *flow,
enum ofputil_port_config config = 0;
uint8_t table_id = 0;
- verdict = rule_dpif_lookup_from_table(ofproto, flow, wc, false,
+ verdict = rule_dpif_lookup_from_table(ofproto, flow, wc, true,
&table_id, rule);
switch (verdict) {
@@ -3085,7 +3086,7 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, const
struct flow *flow,
VLOG_WARN_RL(&rl, "packet-in on unknown OpenFlow port %"PRIu16,
flow->in_port.ofp_port);
}
- config = port->up.pp.config;
+ config = port ? port->up.pp.config : 0;
break;
}
case RULE_DPIF_LOOKUP_VERDICT_DROP:
@@ -3100,121 +3101,106 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, const
struct flow *flow,
return table_id;
}
-bool
-rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto,
- const struct flow *flow, struct flow_wildcards *wc,
- uint8_t table_id, struct rule_dpif **rule)
+static struct rule_dpif *
+rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, uint8_t table_id,
+ const struct flow *flow, struct flow_wildcards *wc)
{
+ struct classifier *cls = &ofproto->up.tables[table_id].cls;
const struct cls_rule *cls_rule;
- struct classifier *cls;
- bool frag;
-
- *rule = NULL;
- if (table_id >= N_TABLES) {
- return false;
- }
+ struct rule_dpif *rule;
- if (wc) {
- memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
- if (is_ip_any(flow)) {
- wc->masks.nw_frag |= FLOW_NW_FRAG_MASK;
+ fat_rwlock_rdlock(&cls->rwlock);
+ if (ofproto->up.frag_handling != OFPC_FRAG_NX_MATCH) {
+ if (wc) {
+ memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
+ if (is_ip_any(flow)) {
+ wc->masks.nw_frag |= FLOW_NW_FRAG_MASK;
+ }
}
- }
- cls = &ofproto->up.tables[table_id].cls;
- fat_rwlock_rdlock(&cls->rwlock);
- frag = (flow->nw_frag & FLOW_NW_FRAG_ANY) != 0;
- if (frag && ofproto->up.frag_handling == OFPC_FRAG_NORMAL) {
- /* We must pretend that transport ports are unavailable. */
- struct flow ofpc_normal_flow = *flow;
- ofpc_normal_flow.tp_src = htons(0);
- ofpc_normal_flow.tp_dst = htons(0);
- cls_rule = classifier_lookup(cls, &ofpc_normal_flow, wc);
- } else if (frag && ofproto->up.frag_handling == OFPC_FRAG_DROP) {
- cls_rule = &ofproto->drop_frags_rule->up.cr;
- /* Frag mask in wc already set above. */
+ if (flow->nw_frag & FLOW_NW_FRAG_ANY) {
+ if (ofproto->up.frag_handling == OFPC_FRAG_NORMAL) {
+ /* We must pretend that transport ports are unavailable. */
+ struct flow ofpc_normal_flow = *flow;
+ ofpc_normal_flow.tp_src = htons(0);
+ ofpc_normal_flow.tp_dst = htons(0);
+ cls_rule = classifier_lookup(cls, &ofpc_normal_flow, wc);
+ } else {
+ /* Must be OFPC_FRAG_DROP (we don't have OFPC_FRAG_REASM). */
+ cls_rule = &ofproto->drop_frags_rule->up.cr;
+ }
+ } else {
+ cls_rule = classifier_lookup(cls, flow, wc);
+ }
} else {
cls_rule = classifier_lookup(cls, flow, wc);
}
- *rule = rule_dpif_cast(rule_from_cls_rule(cls_rule));
- rule_dpif_ref(*rule);
+ rule = rule_dpif_cast(rule_from_cls_rule(cls_rule));
+ rule_dpif_ref(rule);
fat_rwlock_unlock(&cls->rwlock);
- return *rule != NULL;
+ return rule;
}
-/* Lookup 'flow' in 'ofproto''s classifier starting at 'table_id'.
+/* Look up 'flow' in 'ofproto''s classifier starting from table '*table_id'.
+ * Stores the rule that was found in '*rule', or NULL if none was found.
+ * Updates 'wc', if nonnull, to reflect the fields that were used during the
+ * lookup.
+ *
+ * If 'honor_table_miss' is true, the first lookup occurs in '*table_id', but
+ * if none is found then the table miss configuration for that table is
+ * honored, which can result in additional lookups in other OpenFlow tables.
+ * In this case the function updates '*table_id' to reflect the final OpenFlow
+ * table that was searched.
*
- * If 'wc' is non-null, sets the fields that were relevant as part
- * of the lookup.
+ * If 'honor_table_miss' is false, then only one table lookup occurs, in
+ * '*table_id'.
*
- * 'table_id' is set to the table where a match or miss occurred.
- * This value will be the input value of 'table_id' unless there was
- * a miss and OFPTC_TABLE_MISS_CONTINUE is in effect for the sequence of
- * tables where misses occur.
+ * Returns:
*
- * If 'force_controller_on_miss' is true then if a miss occurs
- * then RULE_OFPTC_TABLE_MISS_CONTROLLER will be returned regarless
- * of which OFPTC_TABLE_* setting is in effect.
+ * - RULE_DPIF_LOOKUP_VERDICT_MATCH if a rule (in '*rule') was found.
*
- * The return value is:
- * RULE_DPIF_LOOKUP_VERDICT_MATCH: If a match occurred
- * RULE_OFPTC_TABLE_MISS_CONTROLLER: If a miss occurred and the packet
- * should be forwarded to the controller.
- * RULE_OFPTC_TABLE_MISS_DROP: If a miss occurred and the packet
- * should be dropped. */
+ * - RULE_DPIF_LOOKUP_VERDICT_DROP if no rule was found and a table miss
+ * configuration specified that the packet should be dropped in this
+ * case. (This occurs only if 'honor_table_miss' is true, because only in
+ * this case does the table miss configuration matter.)
+ *
+ * - RULE_DPIF_LOOKUP_VERDICT_CONTROLLER if no rule was found otherwise. */
enum rule_dpif_lookup_verdict
rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
const struct flow *flow,
struct flow_wildcards *wc,
- bool force_controller_on_miss,
+ bool honor_table_miss,
uint8_t *table_id, struct rule_dpif **rule)
{
- uint8_t next_id = *table_id;
-
- while (next_id < ofproto->up.n_tables) {
- enum ofp_table_config config;
+ uint8_t next_id;
+ for (next_id = *table_id;
+ next_id < ofproto->up.n_tables;
+ next_id++, next_id += (next_id == TBL_INTERNAL))
+ {
*table_id = next_id;
-
- if (rule_dpif_lookup_in_table(ofproto, flow, wc, *table_id, rule)) {
+ *rule = rule_dpif_lookup_in_table(ofproto, *table_id, flow, wc);
+ if (*rule) {
return RULE_DPIF_LOOKUP_VERDICT_MATCH;
- }
-
- if (force_controller_on_miss) {
- break;
- }
-
- /* XXX
- * This does not take into account different
- * behaviour for different OpenFlow versions
- *
- * OFPTC11_TABLE_MISS_CONTINUE: Behaviour of OpenFlow1.0
- * OFPTC11_TABLE_MISS_CONTROLLER: Default for OpenFlow1.1+
- * OFPTC11_TABLE_MISS_DROP: Default for OpenFlow1.3+
- *
- * Instead the global default is OFPTC_TABLE_MISS_CONTROLLER
- * which may be configured globally using Table Mod. */
- config = table_get_config(&ofproto->up, *table_id);
- switch (config & OFPTC11_TABLE_MISS_MASK) {
- case OFPTC11_TABLE_MISS_CONTINUE:
- break;
- case OFPTC11_TABLE_MISS_CONTROLLER:
+ } else if (!honor_table_miss) {
return RULE_DPIF_LOOKUP_VERDICT_CONTROLLER;
- case OFPTC11_TABLE_MISS_DROP:
- return RULE_DPIF_LOOKUP_VERDICT_DROP;
- }
+ } else {
+ switch (table_get_config(&ofproto->up, *table_id)
+ & OFPTC11_TABLE_MISS_MASK) {
+ case OFPTC11_TABLE_MISS_CONTINUE:
+ break;
- /* Go on to next table. */
- ++next_id;
- if (next_id == TBL_INTERNAL) {
- ++next_id;
+ case OFPTC11_TABLE_MISS_CONTROLLER:
+ return RULE_DPIF_LOOKUP_VERDICT_CONTROLLER;
+
+ case OFPTC11_TABLE_MISS_DROP:
+ return RULE_DPIF_LOOKUP_VERDICT_DROP;
+ }
}
}
- /* Either we fell off the end or
- * a miss occured with force_controller_on_miss set */
return RULE_DPIF_LOOKUP_VERDICT_CONTROLLER;
}
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index c09aee6..5409ce7 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -86,10 +86,6 @@ enum rule_dpif_lookup_verdict
rule_dpif_lookup_from_table(struct ofproto_dpif *,
uint8_t *table_id,
struct rule_dpif
**rule);
-bool rule_dpif_lookup_in_table(struct ofproto_dpif *, const struct flow *,
- struct flow_wildcards *, uint8_t table_id,
- struct rule_dpif **rule);
-
void rule_dpif_ref(struct rule_dpif *);
void rule_dpif_unref(struct rule_dpif *);
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
