[ovs-dev] [slow path 09/11] ofproto-dpif: Introduce "internal flows" for handling flow table misses.
Ben Pfaff
blp at nicira.com
Thu May 10 16:06:23 UTC 2012
To allow facets to be created for packets that miss and therefore to
allow them to be installed in the kernel.
On Thu, May 10, 2012 at 08:12:27AM -0700, Joji Mtt wrote:
> Hi Ben,
>
> Could you clarify the use-case for these internal OpenFlow rules?
>
> Thanks,
> Joji.
>
> On Wed, May 9, 2012 at 10:19 AM, Ben Pfaff <blp at nicira.com> wrote:
>
> > The new message is:
> >
> > ofproto-dpif: Introduce "internal flows" for handling flow table misses.
> >
> > The ofproto-dpif implementation of "facet"s requires a facet to be
> > associated with an OpenFlow rule. Until now, this meant that packets
> > that miss in the OpenFlow table (and thus didn't have OpenFlow rules)
> > couldn't be set up as facets and thus couldn't be installed in the
> > kernel. This commit changes that, by introducing "internal" OpenFlow
> > rules to associate with such packets.
> >
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> >
> > On Tue, May 08, 2012 at 02:40:52PM -0700, Ethan Jackson wrote:
> > > 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 <blp at nicira.com> wrote:
> > > > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > > > ---
> > > > 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
> > > > dev at openvswitch.org
> > > > http://openvswitch.org/mailman/listinfo/dev
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
More information about the dev
mailing list