As discussed offline, the commit message could use a bit more detail. Otherwise looks good, thanks.
Ethan On Sat, May 5, 2012 at 11:10 AM, Ben Pfaff <[email protected]> wrote: > Signed-off-by: Ben Pfaff <[email protected]> > --- > ofproto/ofproto-dpif.c | 202 > ++++++++++++++++++++++++++++++------------------ > tests/ofproto.at | 6 +- > 2 files changed, 132 insertions(+), 76 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 3df6f27..9dcfccd 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -56,7 +56,6 @@ VLOG_DEFINE_THIS_MODULE(ofproto_dpif); > > COVERAGE_DEFINE(ofproto_dpif_ctlr_action); > COVERAGE_DEFINE(ofproto_dpif_expired); > -COVERAGE_DEFINE(ofproto_dpif_no_packet_in); > COVERAGE_DEFINE(ofproto_dpif_xlate); > COVERAGE_DEFINE(facet_changed_rule); > COVERAGE_DEFINE(facet_invalidated); > @@ -70,7 +69,8 @@ COVERAGE_DEFINE(facet_suppress); > > /* Number of implemented OpenFlow tables. */ > enum { N_TABLES = 255 }; > -BUILD_ASSERT_DECL(N_TABLES >= 1 && N_TABLES <= 255); > +enum { TBL_INTERNAL = N_TABLES - 1 }; /* Used for internal hidden rules. > */ > +BUILD_ASSERT_DECL(N_TABLES >= 2 && N_TABLES <= 255); > > struct ofport_dpif; > struct ofproto_dpif; > @@ -105,7 +105,10 @@ static struct rule_dpif *rule_dpif_cast(const struct > rule *rule) > } > > static struct rule_dpif *rule_dpif_lookup(struct ofproto_dpif *, > - const struct flow *, uint8_t > table); > + const struct flow *); > +static struct rule_dpif *rule_dpif_lookup__(struct ofproto_dpif *, > + const struct flow *, > + uint8_t table); > > static void rule_credit_stats(struct rule_dpif *, > const struct dpif_flow_stats *); > @@ -425,7 +428,7 @@ static struct facet *facet_find(struct ofproto_dpif *, > const struct flow *, uint32_t hash); > static struct facet *facet_lookup_valid(struct ofproto_dpif *, > const struct flow *, uint32_t hash); > -static bool facet_revalidate(struct facet *); > +static void facet_revalidate(struct facet *); > static bool facet_check_consistency(struct facet *); > > static void facet_flush_stats(struct facet *); > @@ -532,6 +535,10 @@ struct ofproto_dpif { > struct dpif *dpif; > int max_ports; > > + /* Special OpenFlow rules. */ > + struct rule_dpif *miss_rule; /* Sends flow table misses to controller. */ > + struct rule_dpif *no_packet_in_rule; /* Drops flow table misses. */ > + > /* Statistics. */ > uint64_t n_matches; > > @@ -651,6 +658,8 @@ del(const char *type, const char *name) > > /* Basic life-cycle. */ > > +static int add_internal_flows(struct ofproto_dpif *); > + > static struct ofproto * > alloc(void) > { > @@ -733,10 +742,73 @@ construct(struct ofproto *ofproto_) > memset(&ofproto->stats, 0, sizeof ofproto->stats); > > ofproto_init_tables(ofproto_, N_TABLES); > + error = add_internal_flows(ofproto); > + ofproto->up.tables[TBL_INTERNAL].flags = OFTABLE_HIDDEN | > OFTABLE_READONLY; > + > + return error; > +} > + > +static int > +add_internal_flow(struct ofproto_dpif *ofproto, int id, > + const struct ofpbuf *actions, struct rule_dpif **rulep) > +{ > + struct ofputil_flow_mod fm; > + int error; > + > + cls_rule_init_catchall(&fm.cr, 0); > + cls_rule_set_reg(&fm.cr, 0, id); > + fm.cookie = htonll(0); > + fm.cookie_mask = htonll(0); > + fm.table_id = TBL_INTERNAL; > + fm.command = OFPFC_ADD; > + fm.idle_timeout = 0; > + fm.hard_timeout = 0; > + fm.buffer_id = 0; > + fm.out_port = 0; > + fm.flags = 0; > + fm.actions = actions->data; > + fm.n_actions = actions->size / sizeof(union ofp_action); > + > + error = ofproto_flow_mod(&ofproto->up, &fm); > + if (error) { > + VLOG_ERR_RL(&rl, "failed to add internal flow %d (%s)", > + id, ofperr_to_string(error)); > + return error; > + } > + > + *rulep = rule_dpif_lookup__(ofproto, &fm.cr.flow, TBL_INTERNAL); > + assert(*rulep != NULL); > > return 0; > } > > +static int > +add_internal_flows(struct ofproto_dpif *ofproto) > +{ > + struct nx_action_controller *nac; > + uint64_t actions_stub[128 / 8]; > + struct ofpbuf actions; > + int error; > + int id; > + > + ofpbuf_use_stack(&actions, actions_stub, sizeof actions_stub); > + id = 1; > + > + nac = ofputil_put_NXAST_CONTROLLER(&actions); > + nac->max_len = htons(UINT16_MAX); > + nac->controller_id = htons(0); > + nac->reason = OFPR_NO_MATCH; > + error = add_internal_flow(ofproto, id++, &actions, &ofproto->miss_rule); > + if (error) { > + return error; > + } > + > + ofpbuf_clear(&actions); > + error = add_internal_flow(ofproto, id++, &actions, > + &ofproto->no_packet_in_rule); > + return error; > +} > + > static void > complete_operations(struct ofproto_dpif *ofproto) > { > @@ -861,13 +933,13 @@ run(struct ofproto *ofproto_) > || !tag_set_is_empty(&ofproto->revalidate_set)) { > struct tag_set revalidate_set = ofproto->revalidate_set; > bool revalidate_all = ofproto->need_revalidate; > - struct facet *facet, *next; > + struct facet *facet; > > /* Clear the revalidation flags. */ > tag_set_init(&ofproto->revalidate_set); > ofproto->need_revalidate = false; > > - HMAP_FOR_EACH_SAFE (facet, next, hmap_node, &ofproto->facets) { > + HMAP_FOR_EACH (facet, hmap_node, &ofproto->facets) { > if (revalidate_all > || tag_set_intersects(&revalidate_set, facet->tags)) { > facet_revalidate(facet); > @@ -1082,7 +1154,8 @@ port_reconfigured(struct ofport *port_, enum > ofputil_port_config old_config) > enum ofputil_port_config changed = old_config ^ port->up.pp.config; > > if (changed & (OFPUTIL_PC_NO_RECV | OFPUTIL_PC_NO_RECV_STP | > - OFPUTIL_PC_NO_FWD | OFPUTIL_PC_NO_FLOOD)) { > + OFPUTIL_PC_NO_FWD | OFPUTIL_PC_NO_FLOOD | > + OFPUTIL_PC_NO_PACKET_IN)) { > ofproto->need_revalidate = true; > > if (changed & OFPUTIL_PC_NO_FLOOD && port->bundle) { > @@ -2793,30 +2866,6 @@ handle_flow_miss_with_facet(struct flow_miss *miss, > struct facet *facet, > } > } > > -/* Handles flow miss 'miss' on 'ofproto'. The flow does not match any flow > in > - * the OpenFlow flow table. */ > -static void > -handle_flow_miss_no_rule(struct ofproto_dpif *ofproto, struct flow_miss > *miss) > -{ > - uint16_t in_port = miss->flow.in_port; > - struct ofport_dpif *port = get_ofp_port(ofproto, in_port); > - > - if (!port) { > - VLOG_WARN_RL(&rl, "packet-in on unknown port %"PRIu16, in_port); > - } > - > - if (port && port->up.pp.config & OFPUTIL_PC_NO_PACKET_IN) { > - /* XXX install 'drop' flow entry */ > - COVERAGE_INC(ofproto_dpif_no_packet_in); > - } else { > - const struct ofpbuf *packet; > - > - LIST_FOR_EACH (packet, list_node, &miss->packets) { > - send_packet_in_miss(ofproto, packet, &miss->flow); > - } > - } > -} > - > /* Handles flow miss 'miss' on 'ofproto'. May add any required datapath > * operations to 'ops', incrementing '*n_ops' for each new op. */ > static void > @@ -2832,11 +2881,9 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct > flow_miss *miss, > > facet = facet_lookup_valid(ofproto, &miss->flow, hash); > if (!facet) { > - struct rule_dpif *rule = rule_dpif_lookup(ofproto, &miss->flow, 0); > - if (!rule) { > - handle_flow_miss_no_rule(ofproto, miss); > - return; > - } else if (!flow_miss_should_make_facet(ofproto, miss, hash)) { > + struct rule_dpif *rule = rule_dpif_lookup(ofproto, &miss->flow); > + > + if (!flow_miss_should_make_facet(ofproto, miss, hash)) { > handle_flow_miss_without_facet(miss, rule, ops, n_ops); > return; > } > @@ -3676,16 +3723,13 @@ static struct facet * > facet_lookup_valid(struct ofproto_dpif *ofproto, const struct flow *flow, > uint32_t hash) > { > - struct facet *facet = facet_find(ofproto, flow, hash); > + struct facet *facet; > > - /* The facet we found might not be valid, since we could be in need of > - * revalidation. If it is not valid, don't return it. */ > + facet = facet_find(ofproto, flow, hash); > if (facet > && (ofproto->need_revalidate > - || tag_set_intersects(&ofproto->revalidate_set, facet->tags)) > - && !facet_revalidate(facet)) { > - COVERAGE_INC(facet_invalidated); > - return NULL; > + || tag_set_intersects(&ofproto->revalidate_set, facet->tags))) { > + facet_revalidate(facet); > } > > return facet; > @@ -3707,17 +3751,10 @@ facet_check_consistency(struct facet *facet) > bool ok; > > /* Check the rule for consistency. */ > - rule = rule_dpif_lookup(ofproto, &facet->flow, 0); > - if (!rule) { > - if (!VLOG_DROP_WARN(&rl)) { > - char *s = flow_to_string(&facet->flow); > - VLOG_WARN("%s: facet should not exist", s); > - free(s); > - } > - return false; > - } else if (rule != facet->rule) { > + rule = rule_dpif_lookup(ofproto, &facet->flow); > + ok = rule == facet->rule; > + if (!ok) { > may_log = !VLOG_DROP_WARN(&rl); > - ok = false; > if (may_log) { > struct ds s; > > @@ -3734,8 +3771,6 @@ facet_check_consistency(struct facet *facet) > VLOG_WARN("%s", ds_cstr(&s)); > ds_destroy(&s); > } > - } else { > - ok = true; > } > > /* Check the datapath actions for consistency. */ > @@ -3819,12 +3854,8 @@ facet_check_consistency(struct facet *facet) > * 'facet' to the new rule and recompiles its actions. > * > * - If the rule found is the same as 'facet''s current rule, leaves 'facet' > - * where it is and recompiles its actions anyway. > - * > - * - If there is none, destroys 'facet'. > - * > - * Returns true if 'facet' still exists, false if it has been destroyed. */ > -static bool > + * where it is and recompiles its actions anyway. */ > +static void > facet_revalidate(struct facet *facet) > { > struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto); > @@ -3845,13 +3876,7 @@ facet_revalidate(struct facet *facet) > > COVERAGE_INC(facet_revalidate); > > - /* Determine the new rule. */ > - new_rule = rule_dpif_lookup(ofproto, &facet->flow, 0); > - if (!new_rule) { > - /* No new rule, so delete the facet. */ > - facet_remove(facet); > - return false; > - } > + new_rule = rule_dpif_lookup(ofproto, &facet->flow); > > /* Calculate new datapath actions. > * > @@ -3934,8 +3959,6 @@ facet_revalidate(struct facet *facet) > facet->used = new_rule->up.created; > facet->prev_used = facet->used; > } > - > - return true; > } > > /* Updates 'facet''s used time. Caller is responsible for calling > @@ -4298,8 +4321,31 @@ subfacet_update_stats(struct subfacet *subfacet, > /* Rules. */ > > static struct rule_dpif * > -rule_dpif_lookup(struct ofproto_dpif *ofproto, const struct flow *flow, > - uint8_t table_id) > +rule_dpif_lookup(struct ofproto_dpif *ofproto, const struct flow *flow) > +{ > + struct ofport_dpif *port; > + struct rule_dpif *rule; > + > + rule = rule_dpif_lookup__(ofproto, flow, 0); > + if (rule) { > + return rule; > + } > + > + port = get_ofp_port(ofproto, flow->in_port); > + if (!port) { > + VLOG_WARN_RL(&rl, "packet-in on unknown port %"PRIu16, > flow->in_port); > + return ofproto->miss_rule; > + } > + > + if (port->up.pp.config & OFPUTIL_PC_NO_PACKET_IN) { > + return ofproto->no_packet_in_rule; > + } > + return ofproto->miss_rule; > +} > + > +static struct rule_dpif * > +rule_dpif_lookup__(struct ofproto_dpif *ofproto, const struct flow *flow, > + uint8_t table_id) > { > struct cls_rule *cls_rule; > struct classifier *cls; > @@ -4707,7 +4753,7 @@ xlate_table_action(struct action_xlate_ctx *ctx, > /* Look up a flow with 'in_port' as the input port. */ > old_in_port = ctx->flow.in_port; > ctx->flow.in_port = in_port; > - rule = rule_dpif_lookup(ofproto, &ctx->flow, table_id); > + rule = rule_dpif_lookup__(ofproto, &ctx->flow, table_id); > > /* Tag the flow. */ > if (table_id > 0 && table_id < N_TABLES) { > @@ -6502,8 +6548,16 @@ ofproto_trace(struct ofproto_dpif *ofproto, const > struct flow *flow, > flow_format(ds, flow); > ds_put_char(ds, '\n'); > > - rule = rule_dpif_lookup(ofproto, flow, 0); > + rule = rule_dpif_lookup(ofproto, flow); > + > trace_format_rule(ds, 0, 0, rule); > + if (rule == ofproto->miss_rule) { > + ds_put_cstr(ds, "\nNo match, flow generates \"packet in\"s.\n"); > + } else if (rule == ofproto->no_packet_in_rule) { > + ds_put_cstr(ds, "\nNo match, packets dropped because " > + "OFPPC_NO_PACKET_IN is set on in_port.\n"); > + } > + > if (rule) { > uint64_t odp_actions_stub[1024 / 8]; > struct ofpbuf odp_actions; > diff --git a/tests/ofproto.at b/tests/ofproto.at > index 82f2f9c..474edc8 100644 > --- a/tests/ofproto.at > +++ b/tests/ofproto.at > @@ -214,12 +214,14 @@ OVS_VSWITCHD_START > 0: classifier: wild=0x3fffff, max=1000000, active=0 > lookup=0, matched=0" > x=1 > - while test $x -lt 255; do > + while test $x -lt 254; do > printf " %d: %-8s: wild=0x3fffff, max=1000000, active=0 > lookup=0, matched=0 > " $x table$x > x=`expr $x + 1` > - done) > expout > + done > + echo " 254: table254: wild=0x3fffff, max=1000000, active=2 > + lookup=0, matched=0") > expout > AT_CHECK([ovs-ofctl dump-tables br0], [0], [expout]) > # Change the configuration. > AT_CHECK( > -- > 1.7.2.5 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
