diff mbox

[ovs-dev,v2,14/15] ofproto: Refactor packet_out handling.

Message ID 1471908701-102773-15-git-send-email-jarno@ovn.org
State Changes Requested
Headers show

Commit Message

Jarno Rajahalme Aug. 22, 2016, 11:31 p.m. UTC
Refactor handle_packet_out() to prepare for bundle support for packet
outs in a later patch.

Two new callbacks are introduced in ofproto-provider class:
->packet_xlate() and ->packet_execute().  ->packet_xlate() translates
the packet using the flow and actions provided by the caller, but
defers all OpenFlow-visible side-effects (stats, learn actions, actual
packet output, etc.) to be explicitly executed with the
->packet_execute() call.

Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
 ofproto/ofproto-dpif-upcall.c |  11 +-
 ofproto/ofproto-dpif-xlate.c  |  45 ++++----
 ofproto/ofproto-dpif-xlate.h  |   3 +-
 ofproto/ofproto-dpif.c        | 136 ++++++++++++++++++++----
 ofproto/ofproto-dpif.h        |  15 +--
 ofproto/ofproto-provider.h    |  90 ++++++++--------
 ofproto/ofproto.c             | 233 +++++++++++++++++++++++++++++++++---------
 7 files changed, 389 insertions(+), 144 deletions(-)

Comments

Ben Pfaff Aug. 30, 2016, 3:40 p.m. UTC | #1
On Mon, Aug 22, 2016 at 04:31:40PM -0700, Jarno Rajahalme wrote:
> Refactor handle_packet_out() to prepare for bundle support for packet
> outs in a later patch.
> 
> Two new callbacks are introduced in ofproto-provider class:
> ->packet_xlate() and ->packet_execute().  ->packet_xlate() translates
> the packet using the flow and actions provided by the caller, but
> defers all OpenFlow-visible side-effects (stats, learn actions, actual
> packet output, etc.) to be explicitly executed with the
> ->packet_execute() call.
> 
> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>

This adds a couple of comments of the form

    /* OUTPUT to a real port can not refer to versionable lookups (flow tables,
     * groups), so we do not need to worry about the version to use here. */

but I'm not sure that I believe it.  In particular,
compose_table_xlate(), which has this comment, outputs to OFPP_TABLE,
which isn't a real port at all but instead does a full flow table
traversal.

This line in packet_execute() needs an extra space:
    execute.actions =opo->odp_actions.data;

I'm surprised to see ofproto.c including ofproto-dpif-xlate.h.  That
seems like a layer violation.
Jarno Rajahalme Aug. 30, 2016, 6:28 p.m. UTC | #2
> On Aug 30, 2016, at 8:40 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Mon, Aug 22, 2016 at 04:31:40PM -0700, Jarno Rajahalme wrote:
>> Refactor handle_packet_out() to prepare for bundle support for packet
>> outs in a later patch.
>> 
>> Two new callbacks are introduced in ofproto-provider class:
>> ->packet_xlate() and ->packet_execute().  ->packet_xlate() translates
>> the packet using the flow and actions provided by the caller, but
>> defers all OpenFlow-visible side-effects (stats, learn actions, actual
>> packet output, etc.) to be explicitly executed with the
>> ->packet_execute() call.
>> 
>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
> 
> This adds a couple of comments of the form
> 
>    /* OUTPUT to a real port can not refer to versionable lookups (flow tables,
>     * groups), so we do not need to worry about the version to use here. */
> 
> but I'm not sure that I believe it.  In particular,
> compose_table_xlate(), which has this comment, outputs to OFPP_TABLE,
> which isn't a real port at all but instead does a full flow table
> traversal.
> 

Thanks for spotting this!

> This line in packet_execute() needs an extra space:
>    execute.actions =opo->odp_actions.data;
> 

OK.

> I'm surprised to see ofproto.c including ofproto-dpif-xlate.h.  That
> seems like a layer violation.

I think this was due to the plate cache only. The learn action entries in late cache use the struct ofproto_flow_mod, which the ofproto.c can operate on. Apart from bundle revert these could be handled from ofproto-dpif.c with calls to ofproto.c functions from there. To make the revert work I’d need to add a new ofproto-provider class member function and have it iterate through the late cache and then call back to ofproto.c to revert learned flows.

Also, making the late cache a module of it’s own would remove the inclusion of ofproto-dpif-xlate.h. Would this alleviate your concern with layer violation, or should I strive to keep the xlate cache opaque to ofproto.c?

  Jarno
Ben Pfaff Aug. 30, 2016, 7:59 p.m. UTC | #3
On Tue, Aug 30, 2016 at 11:28:45AM -0700, Jarno Rajahalme wrote:
> > On Aug 30, 2016, at 8:40 AM, Ben Pfaff <blp@ovn.org> wrote:
> > I'm surprised to see ofproto.c including ofproto-dpif-xlate.h.  That
> > seems like a layer violation.
> 
> I think this was due to the plate cache only. The learn action entries in late cache use the struct ofproto_flow_mod, which the ofproto.c can operate on. Apart from bundle revert these could be handled from ofproto-dpif.c with calls to ofproto.c functions from there. To make the revert work I’d need to add a new ofproto-provider class member function and have it iterate through the late cache and then call back to ofproto.c to revert learned flows.
> 
> Also, making the late cache a module of it’s own would remove the inclusion of ofproto-dpif-xlate.h. Would this alleviate your concern with layer violation, or should I strive to keep the xlate cache opaque to ofproto.c?

It would help a lot, if it's practical.
diff mbox

Patch

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 042a50a..ad891be 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1058,7 +1058,9 @@  upcall_xlate(struct udpif *udpif, struct upcall *upcall,
     stats.used = time_msec();
     stats.tcp_flags = ntohs(upcall->flow->tcp_flags);
 
-    xlate_in_init(&xin, upcall->ofproto, upcall->flow, upcall->in_port, NULL,
+    xlate_in_init(&xin, upcall->ofproto,
+                  ofproto_dpif_get_tables_version(upcall->ofproto),
+                  upcall->flow, upcall->in_port, NULL,
                   stats.tcp_flags, upcall->packet, wc, odp_actions);
 
     if (upcall->type == DPIF_UC_MISS) {
@@ -1849,7 +1851,8 @@  revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
         ukey->xcache = xlate_cache_new();
     }
 
-    xlate_in_init(&xin, ofproto, &flow, ofp_in_port, NULL, push.tcp_flags,
+    xlate_in_init(&xin, ofproto, ofproto_dpif_get_tables_version(ofproto),
+                  &flow, ofp_in_port, NULL, push.tcp_flags,
                   NULL, need_revalidate ? &wc : NULL, odp_actions);
     if (push.n_packets) {
         xin.resubmit_stats = &push;
@@ -2025,7 +2028,9 @@  push_dp_ops(struct udpif *udpif, struct ukey_op *ops, size_t n_ops)
             if (!error) {
                 struct xlate_in xin;
 
-                xlate_in_init(&xin, ofproto, &flow, ofp_in_port, NULL,
+                xlate_in_init(&xin, ofproto,
+                              ofproto_dpif_get_tables_version(ofproto),
+                              &flow, ofp_in_port, NULL,
                               push->tcp_flags, NULL, NULL, NULL);
                 xin.resubmit_stats = push->n_packets ? push : NULL;
                 xin.may_learn = push->n_packets > 0;
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 83fdff7..13dab4a 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -169,9 +169,6 @@  struct xlate_ctx {
 
     const struct xbridge *xbridge;
 
-    /* Flow tables version at the beginning of the translation. */
-    ovs_version_t tables_version;
-
     /* Flow at the last commit. */
     struct flow base_flow;
 
@@ -1431,7 +1428,7 @@  group_is_alive(const struct xlate_ctx *ctx, uint32_t group_id, int depth)
     struct group_dpif *group;
 
     group = group_dpif_lookup(ctx->xbridge->ofproto, group_id,
-                              ctx->tables_version, false);
+                              ctx->xin->tables_version, false);
     if (group) {
         return group_first_live_bucket(ctx, group, depth) != NULL;
     }
@@ -2786,10 +2783,12 @@  compose_table_xlate(struct xlate_ctx *ctx, const struct xport *out_dev,
     output.port = OFPP_TABLE;
     output.max_len = 0;
 
-    return ofproto_dpif_execute_actions__(xbridge->ofproto, &flow, NULL,
-                                          &output.ofpact, sizeof output,
-                                          ctx->indentation, ctx->depth,
-                                          ctx->resubmits, packet);
+    /* OUTPUT to a real port can not refer to versionable lookups (flow tables,
+     * groups), so we do not need to worry about the version to use here. */
+    return ofproto_dpif_execute_actions__(xbridge->ofproto, OVS_VERSION_MAX,
+                                          &flow, NULL, &output.ofpact,
+                                          sizeof output, ctx->indentation,
+                                          ctx->depth, ctx->resubmits, packet);
 }
 
 static void
@@ -2975,7 +2974,7 @@  compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
         struct flow old_flow = ctx->xin->flow;
         bool old_conntrack = ctx->conntracked;
         bool old_was_mpls = ctx->was_mpls;
-        ovs_version_t old_version = ctx->tables_version;
+        ovs_version_t old_version = ctx->xin->tables_version;
         struct ofpbuf old_stack = ctx->stack;
         union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)];
         struct ofpbuf old_action_set = ctx->action_set;
@@ -2993,7 +2992,7 @@  compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
         clear_conntrack(flow);
 
         /* The bridge is now known so obtain its table version. */
-        ctx->tables_version
+        ctx->xin->tables_version
             = ofproto_dpif_get_tables_version(ctx->xbridge->ofproto);
 
         if (!process_special(ctx, peer) && may_receive(peer, ctx)) {
@@ -3030,7 +3029,7 @@  compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
         ctx->stack = old_stack;
 
         /* Restore calling bridge's lookup version. */
-        ctx->tables_version = old_version;
+        ctx->xin->tables_version = old_version;
 
         /* The peer bridge popping MPLS should have no effect on the original
          * bridge. */
@@ -3271,7 +3270,7 @@  xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
         ctx->table_id = table_id;
 
         rule = rule_dpif_lookup_from_table(ctx->xbridge->ofproto,
-                                           ctx->tables_version,
+                                           ctx->xin->tables_version,
                                            &ctx->xin->flow, ctx->wc,
                                            ctx->xin->resubmit_stats,
                                            &ctx->table_id, in_port,
@@ -3519,7 +3518,7 @@  xlate_group_action(struct xlate_ctx *ctx, uint32_t group_id)
 
         /* Take ref only if xcache exists. */
         group = group_dpif_lookup(ctx->xbridge->ofproto, group_id,
-                                  ctx->tables_version, ctx->xin->xcache);
+                                  ctx->xin->tables_version, ctx->xin->xcache);
         if (!group) {
             /* XXX: Should set ctx->error ? */
             return true;
@@ -5042,12 +5041,13 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 
 void
 xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto,
-              const struct flow *flow, ofp_port_t in_port,
-              struct rule_dpif *rule, uint16_t tcp_flags,
+              ovs_version_t version, const struct flow *flow,
+              ofp_port_t in_port, struct rule_dpif *rule, uint16_t tcp_flags,
               const struct dp_packet *packet, struct flow_wildcards *wc,
               struct ofpbuf *odp_actions)
 {
     xin->ofproto = ofproto;
+    xin->tables_version = version;
     xin->flow = *flow;
     xin->flow.in_port.ofp_port = in_port;
     xin->flow.actset_output = OFPP_UNSET;
@@ -5400,6 +5400,9 @@  xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
                 goto exit;
             }
             ctx.xbridge = new_bridge;
+            /* The bridge is now known so obtain its table version. */
+            ctx.xin->tables_version
+                = ofproto_dpif_get_tables_version(ctx.xbridge->ofproto);
         }
 
         /* Set the thawed table id.  Note: A table lookup is done only if there
@@ -5450,12 +5453,10 @@  xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         ctx.error = XLATE_NO_RECIRCULATION_CONTEXT;
         goto exit;
     }
-    /* The bridge is now known so obtain its table version. */
-    ctx.tables_version = ofproto_dpif_get_tables_version(ctx.xbridge->ofproto);
 
     if (!xin->ofpacts && !ctx.rule) {
         ctx.rule = rule_dpif_lookup_from_table(
-            ctx.xbridge->ofproto, ctx.tables_version, flow, ctx.wc,
+            ctx.xbridge->ofproto, ctx.xin->tables_version, flow, ctx.wc,
             ctx.xin->resubmit_stats, &ctx.table_id,
             flow->in_port.ofp_port, true, true, ctx.xin->xcache);
         if (ctx.xin->resubmit_stats) {
@@ -5638,7 +5639,8 @@  xlate_resume(struct ofproto_dpif *ofproto,
     flow_extract(&packet, &flow);
 
     struct xlate_in xin;
-    xlate_in_init(&xin, ofproto, &flow, 0, NULL, ntohs(flow.tcp_flags),
+    xlate_in_init(&xin, ofproto, ofproto_dpif_get_tables_version(ofproto),
+                  &flow, 0, NULL, ntohs(flow.tcp_flags),
                   &packet, NULL, odp_actions);
 
     struct ofpact_note noop;
@@ -5719,7 +5721,10 @@  xlate_send_packet(const struct ofport_dpif *ofport, bool oam,
 
     ofpact_put_OUTPUT(&ofpacts)->port = xport->ofp_port;
 
-    return ofproto_dpif_execute_actions(xport->xbridge->ofproto, &flow, NULL,
+    /* Actions here are not referring to anything versionable (flow tables or
+     * groups) so we don't need to worry about the version here. */
+    return ofproto_dpif_execute_actions(xport->xbridge->ofproto,
+                                        OVS_VERSION_MAX, &flow, NULL,
                                         ofpacts.data, ofpacts.size, packet);
 }
 
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 14d81f6..a130d9a 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -130,6 +130,7 @@  struct xlate_out {
 
 struct xlate_in {
     struct ofproto_dpif *ofproto;
+    ovs_version_t        tables_version;   /* Lookup in this version. */
 
     /* Flow to which the OpenFlow actions apply.  xlate_actions() will modify
      * this flow when actions change header fields. */
@@ -285,7 +286,7 @@  const char *xlate_strerror(enum xlate_error error);
 
 enum xlate_error xlate_actions(struct xlate_in *, struct xlate_out *);
 
-void xlate_in_init(struct xlate_in *, struct ofproto_dpif *,
+void xlate_in_init(struct xlate_in *, struct ofproto_dpif *, ovs_version_t,
                    const struct flow *, ofp_port_t in_port, struct rule_dpif *,
                    uint16_t tcp_flags, const struct dp_packet *packet,
                    struct flow_wildcards *, struct ofpbuf *odp_actions);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 53b4c28..7961965 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3747,7 +3747,7 @@  ofproto_dpif_set_packet_odp_port(const struct ofproto_dpif *ofproto,
 
 int
 ofproto_dpif_execute_actions__(struct ofproto_dpif *ofproto,
-                               const struct flow *flow,
+                               ovs_version_t version, const struct flow *flow,
                                struct rule_dpif *rule,
                                const struct ofpact *ofpacts, size_t ofpacts_len,
                                int indentation, int depth, int resubmits,
@@ -3769,7 +3769,7 @@  ofproto_dpif_execute_actions__(struct ofproto_dpif *ofproto,
 
     uint64_t odp_actions_stub[1024 / 8];
     struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
-    xlate_in_init(&xin, ofproto, flow, flow->in_port.ofp_port, rule,
+    xlate_in_init(&xin, ofproto, version, flow, flow->in_port.ofp_port, rule,
                   stats.tcp_flags, packet, NULL, &odp_actions);
     xin.ofpacts = ofpacts;
     xin.ofpacts_len = ofpacts_len;
@@ -3807,13 +3807,14 @@  out:
  * 'flow' must reflect the data in 'packet'. */
 int
 ofproto_dpif_execute_actions(struct ofproto_dpif *ofproto,
-                             const struct flow *flow,
+                             ovs_version_t version, const struct flow *flow,
                              struct rule_dpif *rule,
                              const struct ofpact *ofpacts, size_t ofpacts_len,
                              struct dp_packet *packet)
 {
-    return ofproto_dpif_execute_actions__(ofproto, flow, rule, ofpacts,
-                                          ofpacts_len, 0, 0, 0, packet);
+    return ofproto_dpif_execute_actions__(ofproto, version, flow, rule,
+                                          ofpacts, ofpacts_len, 0, 0, 0,
+                                          packet);
 }
 
 static void
@@ -3892,7 +3893,7 @@  rule_set_recirc_id(struct rule *rule_, uint32_t id)
 }
 
 ovs_version_t
-ofproto_dpif_get_tables_version(struct ofproto_dpif *ofproto OVS_UNUSED)
+ofproto_dpif_get_tables_version(struct ofproto_dpif *ofproto)
 {
     ovs_version_t version;
 
@@ -4270,6 +4271,107 @@  rule_get_stats(struct rule *rule_, uint64_t *packets, uint64_t *bytes,
     ovs_mutex_unlock(&rule->stats_mutex);
 }
 
+static enum ofperr
+packet_xlate(struct ofproto *ofproto_, struct ofproto_packet_out *opo)
+{
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
+    struct xlate_out xout;
+    struct xlate_in xin;
+    enum ofperr error = 0;
+
+    xlate_in_init(&xin, ofproto, opo->version, opo->flow,
+                  opo->flow->in_port.ofp_port, NULL, 0, opo->packet, NULL,
+                  &opo->odp_actions);
+    xin.ofpacts = opo->ofpacts;
+    xin.ofpacts_len = opo->ofpacts_len;
+    /* No learning or stats, but collect side effects to xcache. */
+    xin.may_learn = false;
+    xin.resubmit_stats = NULL;
+    xin.xcache = opo->xcache;
+
+    if (xlate_actions(&xin, &xout) != XLATE_OK) {
+        xlate_cache_clear(opo->xcache);
+        ofpbuf_clear(&opo->odp_actions);
+        error = OFPERR_OFPFMFC_UNKNOWN;   /* Error processing actions. */
+    } else {
+        opo->needs_help = (xout.slow & SLOW_ACTION) != 0;
+        recirc_refs_swap(&opo->rr, &xout.recircs); /* Hold recirc refs. */
+    }
+    xlate_out_uninit(&xout);
+
+    return error;
+}
+
+/* Push stats and perform side effects of flow translation. */
+static void
+ofproto_dpif_xcache_execute(struct xlate_cache *xcache,
+                            const struct dpif_flow_stats *stats)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    struct xc_entry *entry;
+    struct ofpbuf entries = xcache->entries;
+
+    XC_ENTRY_FOR_EACH (entry, &entries) {
+        switch (entry->type) {
+        case XC_LEARN:
+            /* Taken care of by the 'ofproto' layer. */
+            break;
+        case XC_FIN_TIMEOUT:
+            if (stats->tcp_flags & (TCP_FIN | TCP_RST)) {
+                /* 'ofproto_mutex' already held */
+                ofproto_rule_reduce_timeouts__(&entry->u.fin.rule->up,
+                                               entry->u.fin.idle,
+                                               entry->u.fin.hard);
+            }
+            break;
+            /* All the rest can be dealt with by the xlate layer. */
+        case XC_TABLE:
+        case XC_RULE:
+        case XC_BOND:
+        case XC_NETDEV:
+        case XC_NETFLOW:
+        case XC_MIRROR:
+        case XC_NORMAL:
+        case XC_GROUP:
+        case XC_TNL_NEIGH:
+        case XC_CONTROLLER:
+            xlate_push_stats_entry(entry, stats);
+            break;
+        default:
+            OVS_NOT_REACHED();
+        }
+    }
+}
+
+static void
+packet_execute(struct ofproto *ofproto_, struct ofproto_packet_out *opo)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
+    struct dpif_flow_stats stats;
+    struct dpif_execute execute;
+
+    /* Run the side effects from the xcache. */
+    dpif_flow_stats_extract(opo->flow, opo->packet, time_msec(), &stats);
+    ofproto_dpif_xcache_execute(opo->xcache, &stats);
+
+    execute.actions =opo->odp_actions.data;
+    execute.actions_len = opo->odp_actions.size;
+
+    pkt_metadata_from_flow(&opo->packet->md, opo->flow);
+    execute.packet = opo->packet;
+    execute.flow = opo->flow;
+    execute.needs_help = opo->needs_help;
+    execute.probe = false;
+    execute.mtu = 0;
+
+    /* Fix up in_port. */
+    ofproto_dpif_set_packet_odp_port(ofproto, opo->flow->in_port.ofp_port,
+                                     opo->packet);
+
+    dpif_execute(ofproto->backer->dpif, &execute);
+}
+
 static struct group_dpif *group_dpif_cast(const struct ofgroup *group)
 {
     return group ? CONTAINER_OF(group, struct group_dpif, up) : NULL;
@@ -4468,18 +4570,6 @@  set_frag_handling(struct ofproto *ofproto_,
 }
 
 static enum ofperr
-packet_out(struct ofproto *ofproto_, struct dp_packet *packet,
-           const struct flow *flow,
-           const struct ofpact *ofpacts, size_t ofpacts_len)
-{
-    struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
-
-    ofproto_dpif_execute_actions(ofproto, flow, NULL, ofpacts,
-                                 ofpacts_len, packet);
-    return 0;
-}
-
-static enum ofperr
 nxt_resume(struct ofproto *ofproto_,
            const struct ofputil_packet_in_private *pin)
 {
@@ -5160,9 +5250,10 @@  ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
     trace.result = ds;
     trace.key = flow; /* Original flow key, used for megaflow. */
     trace.flow = *flow; /* May be modified by actions. */
-    xlate_in_init(&trace.xin, ofproto, flow, flow->in_port.ofp_port, NULL,
-                  ntohs(flow->tcp_flags), packet, &trace.wc,
-                  &trace.odp_actions);
+    xlate_in_init(&trace.xin, ofproto,
+                  ofproto_dpif_get_tables_version(ofproto), flow,
+                  flow->in_port.ofp_port, NULL, ntohs(flow->tcp_flags),
+                  packet, &trace.wc, &trace.odp_actions);
     trace.xin.ofpacts = ofpacts;
     trace.xin.ofpacts_len = ofpacts_len;
     trace.xin.resubmit_hook = trace_resubmit;
@@ -5655,8 +5746,9 @@  const struct ofproto_class ofproto_dpif_class = {
     rule_destruct,
     rule_dealloc,
     rule_get_stats,
+    packet_xlate,
+    packet_execute,
     set_frag_handling,
-    packet_out,
     nxt_resume,
     set_netflow,
     get_netflow_ids,
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 54c8703..ec8830d 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -152,13 +152,14 @@  const char *group_dpif_get_selection_method(const struct group_dpif *group);
 uint64_t group_dpif_get_selection_method_param(const struct group_dpif *group);
 const struct field_array *group_dpif_get_fields(const struct group_dpif *group);
 
-int ofproto_dpif_execute_actions(struct ofproto_dpif *, const struct flow *,
-                                 struct rule_dpif *, const struct ofpact *,
-                                 size_t ofpacts_len, struct dp_packet *);
-int ofproto_dpif_execute_actions__(struct ofproto_dpif *, const struct flow *,
-                                   struct rule_dpif *, const struct ofpact *,
-                                   size_t ofpacts_len, int indentation,
-                                   int depth, int resubmits,
+int ofproto_dpif_execute_actions(struct ofproto_dpif *, ovs_version_t,
+                                 const struct flow *, struct rule_dpif *,
+                                 const struct ofpact *, size_t ofpacts_len,
+                                 struct dp_packet *);
+int ofproto_dpif_execute_actions__(struct ofproto_dpif *, ovs_version_t,
+                                   const struct flow *, struct rule_dpif *,
+                                   const struct ofpact *, size_t ofpacts_len,
+                                   int indentation, int depth, int resubmits,
                                    struct dp_packet *);
 void ofproto_dpif_send_async_msg(struct ofproto_dpif *,
                                  struct ofproto_async_msg *);
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 725f434..b02c450 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -40,6 +40,7 @@ 
 #include "hindex.h"
 #include "object-collection.h"
 #include "ofproto/ofproto.h"
+#include "ofproto/ofproto-dpif-rid.h"
 #include "openvswitch/list.h"
 #include "openvswitch/ofp-actions.h"
 #include "openvswitch/ofp-util.h"
@@ -57,6 +58,8 @@  struct ofputil_flow_mod;
 struct bfd_cfg;
 struct meter;
 struct ofoperation;
+struct ofproto_packet_out;
+struct xlate_cache;
 
 extern struct ovs_mutex ofproto_mutex;
 
@@ -506,6 +509,9 @@  void ofproto_rule_expire(struct rule *rule, uint8_t reason)
     OVS_REQUIRES(ofproto_mutex);
 void ofproto_rule_delete(struct ofproto *, struct rule *)
     OVS_EXCLUDED(ofproto_mutex);
+void ofproto_rule_reduce_timeouts__(struct rule *rule, uint16_t idle_timeout,
+                                    uint16_t hard_timeout)
+    OVS_REQUIRES(ofproto_mutex) OVS_EXCLUDED(rule->mutex);
 void ofproto_rule_reduce_timeouts(struct rule *rule, uint16_t idle_timeout,
                                   uint16_t hard_timeout)
     OVS_EXCLUDED(ofproto_mutex);
@@ -1284,6 +1290,30 @@  struct ofproto_class {
                            uint64_t *byte_count, long long int *used)
         /* OVS_EXCLUDED(ofproto_mutex) */;
 
+    /* Translates actions in 'opo->ofpacts', for 'opo->packet' in flow tables
+     * in version 'opo->version'.  This is useful for OpenFlow OFPT_PACKET_OUT.
+     *
+     * This function must validate that it can correctly translate
+     * 'opo->ofpacts'.  If not, then it should return an OpenFlow error code.
+     *
+     * 'opo->flow' reflects the flow information for 'opo->packet'.  All of the
+     * information in 'opo->flow' is extracted from 'opo->packet', except for
+     * 'in_port', which is assigned to the correct value for the incoming
+     * packet.  'tunnel' and register values should be zeroed.  'packet''s
+     * header pointers and offsets (e.g. packet->l3) are appropriately
+     * initialized.  packet->l3 is aligned on a 32-bit boundary.
+     *
+     * Returns 0 if successful, otherwise an OpenFlow error code.
+     *
+     * This function may be called with 'ofproto_mutex' held. */
+    enum ofperr (*packet_xlate)(struct ofproto *,
+                                struct ofproto_packet_out *opo);
+
+    /* Executes the datapath actions, translation side-effects, and stats as
+     * produced by ->packet_xlate().  The caller retains ownership of 'opo'.
+     */
+    void (*packet_execute)(struct ofproto *, struct ofproto_packet_out *opo);
+
     /* Changes the OpenFlow IP fragment handling policy to 'frag_handling',
      * which takes one of the following values, with the corresponding
      * meanings:
@@ -1315,49 +1345,6 @@  struct ofproto_class {
     bool (*set_frag_handling)(struct ofproto *ofproto,
                               enum ofputil_frag_handling frag_handling);
 
-    /* Implements the OpenFlow OFPT_PACKET_OUT command.  The datapath should
-     * execute the 'ofpacts_len' bytes of "struct ofpacts" in 'ofpacts'.
-     *
-     * The caller retains ownership of 'packet' and of 'ofpacts', so
-     * ->packet_out() should not modify or free them.
-     *
-     * This function must validate that it can correctly implement 'ofpacts'.
-     * If not, then it should return an OpenFlow error code.
-     *
-     * 'flow' reflects the flow information for 'packet'.  All of the
-     * information in 'flow' is extracted from 'packet', except for
-     * flow->in_port (see below).  flow->tunnel and its register values are
-     * zeroed.
-     *
-     * flow->in_port comes from the OpenFlow OFPT_PACKET_OUT message.  The
-     * implementation should reject invalid flow->in_port values by returning
-     * OFPERR_OFPBRC_BAD_PORT.  (If the implementation called
-     * ofproto_init_max_ports(), then the client will reject these ports
-     * itself.)  For consistency, the implementation should consider valid for
-     * flow->in_port any value that could possibly be seen in a packet that it
-     * passes to connmgr_send_packet_in().  Ideally, even an implementation
-     * that never generates packet-ins (e.g. due to hardware limitations)
-     * should still allow flow->in_port values for every possible physical port
-     * and OFPP_LOCAL.  The only virtual ports (those above OFPP_MAX) that the
-     * caller will ever pass in as flow->in_port, other than OFPP_LOCAL, are
-     * OFPP_NONE and OFPP_CONTROLLER.  The implementation should allow both of
-     * these, treating each of them as packets generated by the controller as
-     * opposed to packets originating from some switch port.
-     *
-     * (Ordinarily the only effect of flow->in_port is on output actions that
-     * involve the input port, such as actions that output to OFPP_IN_PORT,
-     * OFPP_FLOOD, or OFPP_ALL.  flow->in_port can also affect Nicira extension
-     * "resubmit" actions.)
-     *
-     * 'packet' is not matched against the OpenFlow flow table, so its
-     * statistics should not be included in OpenFlow flow statistics.
-     *
-     * Returns 0 if successful, otherwise an OpenFlow error code. */
-    enum ofperr (*packet_out)(struct ofproto *ofproto, struct dp_packet *packet,
-                              const struct flow *flow,
-                              const struct ofpact *ofpacts,
-                              size_t ofpacts_len);
-
     enum ofperr (*nxt_resume)(struct ofproto *ofproto,
                               const struct ofputil_packet_in_private *);
 
@@ -1891,6 +1878,23 @@  struct ofproto_group_mod {
     struct group_collection old_groups; /* Affected groups. */
 };
 
+/* packet_out with execution context. */
+struct ofproto_packet_out {
+    ovs_version_t version;
+    struct dp_packet *packet;
+    struct flow *flow;
+    struct ofpact *ofpacts;
+    size_t ofpacts_len;
+    struct xlate_cache *xcache;
+
+    /* These are private members to be managed by the ofproto provider. */
+    struct ofpbuf odp_actions;
+    struct recirc_refs rr;
+    bool needs_help;
+};
+
+void ofproto_packet_out_uninit(struct ofproto_packet_out *);
+
 enum ofperr ofproto_flow_mod(struct ofproto *, const struct ofputil_flow_mod *)
     OVS_EXCLUDED(ofproto_mutex);
 enum ofperr ofproto_flow_mod_init_for_learn(struct ofproto *,
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index d13ddcd..e6f9b19 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -35,6 +35,7 @@ 
 #include "netdev.h"
 #include "nx-match.h"
 #include "ofproto.h"
+#include "ofproto-dpif-xlate.h"
 #include "ofproto-provider.h"
 #include "openflow/nicira-ext.h"
 #include "openflow/openflow.h"
@@ -259,6 +260,9 @@  static enum ofperr ofproto_flow_mod_init(struct ofproto *,
 static enum ofperr ofproto_flow_mod_start(struct ofproto *,
                                           struct ofproto_flow_mod *)
     OVS_REQUIRES(ofproto_mutex);
+static void ofproto_flow_mod_revert(struct ofproto *,
+                                    struct ofproto_flow_mod *)
+    OVS_REQUIRES(ofproto_mutex);
 static void ofproto_flow_mod_finish(struct ofproto *,
                                     struct ofproto_flow_mod *,
                                     const struct openflow_mod_requester *)
@@ -3342,10 +3346,13 @@  reject_slave_controller(struct ofconn *ofconn)
  *
  *    - If they use any groups, then 'ofproto' has that group configured.
  *
- * Returns 0 if successful, otherwise an OpenFlow error. */
+ * Returns 0 if successful, otherwise an OpenFlow error.  Caller must hold
+ * 'ofproto_mutex' for the result to be valid also after this function
+ * returns. */
 enum ofperr
 ofproto_check_ofpacts(struct ofproto *ofproto,
                       const struct ofpact ofpacts[], size_t ofpacts_len)
+    OVS_REQUIRES(ofproto_mutex)
 {
     uint32_t mid;
 
@@ -3364,71 +3371,185 @@  ofproto_check_ofpacts(struct ofproto *ofproto,
     return 0;
 }
 
+void
+ofproto_packet_out_uninit(struct ofproto_packet_out *opo)
+{
+    dp_packet_delete(opo->packet);
+    opo->packet = NULL;
+    free(opo->flow);
+    opo->flow = NULL;
+    free(opo->ofpacts);
+    opo->ofpacts = NULL;
+    opo->ofpacts_len = 0;
+    xlate_cache_delete(opo->xcache);
+    opo->xcache = NULL;
+
+    ofpbuf_uninit(&opo->odp_actions);
+    recirc_refs_unref(&opo->rr);
+}
+
+/* Takes ownership of po->ofpacts, which must have been malloc'ed. */
+static enum ofperr
+ofproto_packet_out_init(struct ofproto *ofproto,
+                        struct ofconn *ofconn,
+                        struct ofproto_packet_out *opo,
+                        const struct ofputil_packet_out *po)
+{
+    enum ofperr error;
+
+    if (ofp_to_u16(po->in_port) >= ofproto->max_ports
+        && ofp_to_u16(po->in_port) < ofp_to_u16(OFPP_MAX)) {
+        return OFPERR_OFPBRC_BAD_PORT;
+    }
+
+    /* Get payload. */
+    if (po->buffer_id != UINT32_MAX) {
+        return OFPERR_OFPBRC_BUFFER_UNKNOWN;
+    }
+
+    /* Ensure that the L3 header is 32-bit aligned. */
+    opo->packet = dp_packet_clone_data_with_headroom(po->packet,
+                                                     po->packet_len, 2);
+    /* Store struct flow. */
+    opo->flow = xmalloc(sizeof *opo->flow);
+    flow_extract(opo->packet, opo->flow);
+    opo->flow->in_port.ofp_port = po->in_port;
+
+    /* Check actions like for flow mods.  We pass a 'table_id' of 0 to
+     * ofproto_check_consistency(), which isn't strictly correct because these
+     * actions aren't in any table.  This is OK as 'table_id' is only used to
+     * check instructions (e.g., goto-table), which can't appear on the action
+     * list of a packet-out. */
+    error = ofpacts_check_consistency(po->ofpacts, po->ofpacts_len,
+                                      opo->flow,
+                                      u16_to_ofp(ofproto->max_ports), 0,
+                                      ofproto->n_tables,
+                                      ofconn_get_protocol(ofconn));
+    if (error) {
+        dp_packet_delete(opo->packet);
+        free(opo->flow);
+        return error;
+    }
+
+    opo->ofpacts = po->ofpacts;
+    opo->ofpacts_len = po->ofpacts_len;
+
+    opo->xcache = xlate_cache_new();
+    ofpbuf_init(&opo->odp_actions, 64);
+    opo->rr = RECIRC_REFS_EMPTY_INITIALIZER;
+    return 0;
+}
+
+static enum ofperr
+ofproto_packet_out_start(struct ofproto *ofproto,
+                         struct ofproto_packet_out *opo)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    enum ofperr error;
+
+    error = ofproto_check_ofpacts(ofproto, opo->ofpacts, opo->ofpacts_len);
+    if (error) {
+        return error;
+    }
+
+    error = ofproto->ofproto_class->packet_xlate(ofproto, opo);
+    if (!error) {
+        struct xc_entry *entry;
+        struct ofpbuf entries = opo->xcache->entries;
+
+        XC_ENTRY_FOR_EACH (entry, &entries) {
+            if (entry->type == XC_LEARN) {
+                struct ofproto_flow_mod *ofm = entry->u.learn.ofm;
+                struct rule *rule = ofm->temp_rule;
+                ovs_assert(rule);
+
+                /* If learning on a different bridge, must use its next
+                 * version number. */
+                ofm->version = (rule->ofproto == ofproto)
+                    ? opo->version : rule->ofproto->tables_version + 1;
+
+                error = ofproto_flow_mod_start(rule->ofproto, ofm);
+                if (error) {
+                    break;
+                }
+            }
+        }
+    }
+    return error;
+}
+
+static void
+ofproto_packet_out_finish(struct ofproto *ofproto,
+                          struct ofproto_packet_out *opo)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    /* Finish the learned flows. */
+    struct xc_entry *entry;
+    struct ofpbuf entries = opo->xcache->entries;
+
+    XC_ENTRY_FOR_EACH (entry, &entries) {
+        if (entry->type == XC_LEARN) {
+            struct ofproto_flow_mod *ofm = entry->u.learn.ofm;
+            struct rule *rule = rule_collection_rules(&ofm->new_rules)[0];
+            ovs_assert(rule);
+
+            /* If learning on a different bridge, must bump its version
+             * number and flush connmgr afterwards. */
+            if (rule->ofproto != ofproto) {
+                ofproto_bump_tables_version(rule->ofproto);
+            }
+            ofproto_flow_mod_finish(rule->ofproto, ofm, NULL);
+            if (rule->ofproto != ofproto) {
+                ofmonitor_flush(rule->ofproto->connmgr);
+            }
+        }
+    }
+
+    ofproto->ofproto_class->packet_execute(ofproto, opo);
+}
+
 static enum ofperr
 handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
+    OVS_EXCLUDED(ofproto_mutex)
 {
     struct ofproto *p = ofconn_get_ofproto(ofconn);
     struct ofputil_packet_out po;
-    struct dp_packet *payload;
+    struct ofproto_packet_out opo;
     uint64_t ofpacts_stub[1024 / 8];
     struct ofpbuf ofpacts;
-    struct flow flow;
     enum ofperr error;
 
     COVERAGE_INC(ofproto_packet_out);
 
     error = reject_slave_controller(ofconn);
     if (error) {
-        goto exit;
+        return error;
     }
 
     /* Decode message. */
     ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
     error = ofputil_decode_packet_out(&po, oh, &ofpacts);
     if (error) {
-        goto exit_free_ofpacts;
-    }
-    if (ofp_to_u16(po.in_port) >= p->max_ports
-        && ofp_to_u16(po.in_port) < ofp_to_u16(OFPP_MAX)) {
-        error = OFPERR_OFPBRC_BAD_PORT;
-        goto exit_free_ofpacts;
+        ofpbuf_uninit(&ofpacts);
+        return error;
     }
 
-    /* Get payload. */
-    if (po.buffer_id != UINT32_MAX) {
-        error = OFPERR_OFPBRC_BUFFER_UNKNOWN;
-        goto exit_free_ofpacts;
+    po.ofpacts = ofpbuf_steal_data(&ofpacts);   /* Move to heap. */
+    error = ofproto_packet_out_init(p, ofconn, &opo, &po);
+    if (error) {
+        free(po.ofpacts);
+        return error;
     }
-    /* Ensure that the L3 header is 32-bit aligned. */
-    payload = dp_packet_clone_data_with_headroom(po.packet, po.packet_len, 2);
-
-    /* Verify actions against packet, then send packet if successful. */
-    flow_extract(payload, &flow);
-    flow.in_port.ofp_port = po.in_port;
 
-    /* Check actions like for flow mods.  We pass a 'table_id' of 0 to
-     * ofproto_check_consistency(), which isn't strictly correct because these
-     * actions aren't in any table.  This is OK as 'table_id' is only used to
-     * check instructions (e.g., goto-table), which can't appear on the action
-     * list of a packet-out. */
-    error = ofpacts_check_consistency(po.ofpacts, po.ofpacts_len,
-                                      &flow, u16_to_ofp(p->max_ports),
-                                      0, p->n_tables,
-                                      ofconn_get_protocol(ofconn));
+    ovs_mutex_lock(&ofproto_mutex);
+    opo.version = p->tables_version;
+    error = ofproto_packet_out_start(p, &opo);
     if (!error) {
-        /* Should hold ofproto_mutex to guarantee state don't
-         * change between the check and the execution. */
-        error = ofproto_check_ofpacts(p, po.ofpacts, po.ofpacts_len);
-        if (!error) {
-            error = p->ofproto_class->packet_out(p, payload, &flow,
-                                                 po.ofpacts, po.ofpacts_len);
-        }
+        ofproto_packet_out_finish(p, &opo);
     }
-    dp_packet_delete(payload);
+    ovs_mutex_unlock(&ofproto_mutex);
 
-exit_free_ofpacts:
-    ofpbuf_uninit(&ofpacts);
-exit:
+    ofproto_packet_out_uninit(&opo);
     return error;
 }
 
@@ -5125,7 +5246,6 @@  modify_flows_start__(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     } else if (ofm->modify_may_add_flow) {
         /* No match, add a new flow, consumes 'temp'. */
         error = add_flow_start(ofproto, ofm);
-        new_rules->collection.n = 1;
     } else {
         error = 0;
     }
@@ -5215,7 +5335,6 @@  modify_flows_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
         }
         learned_cookies_flush(ofproto, &dead_cookies);
         remove_rules_postponed(old_rules);
-        rule_collection_destroy(new_rules);
     }
 }
 
@@ -5387,8 +5506,7 @@  delete_flows_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
 }
 
 static void
-delete_flows_finish(struct ofproto *ofproto,
-                    struct ofproto_flow_mod *ofm,
+delete_flows_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
                     const struct openflow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
 {
@@ -5411,8 +5529,7 @@  delete_flows_init_strict(struct ofproto *ofproto OVS_UNUSED,
 
 /* Implements OFPFC_DELETE_STRICT. */
 static enum ofperr
-delete_flow_start_strict(struct ofproto *ofproto,
-                         struct ofproto_flow_mod *ofm)
+delete_flow_start_strict(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
     struct rule_collection *rules = &ofm->old_rules;
@@ -5487,6 +5604,26 @@  reduce_timeout(uint16_t max, uint16_t *timeout)
  *
  * Suitable for implementing OFPACT_FIN_TIMEOUT. */
 void
+ofproto_rule_reduce_timeouts__(struct rule *rule,
+                               uint16_t idle_timeout, uint16_t hard_timeout)
+    OVS_REQUIRES(ofproto_mutex)
+    OVS_EXCLUDED(rule->mutex)
+{
+    if (!idle_timeout && !hard_timeout) {
+        return;
+    }
+
+    if (ovs_list_is_empty(&rule->expirable)) {
+        ovs_list_insert(&rule->ofproto->expirable, &rule->expirable);
+    }
+
+    ovs_mutex_lock(&rule->mutex);
+    reduce_timeout(idle_timeout, &rule->idle_timeout);
+    reduce_timeout(hard_timeout, &rule->hard_timeout);
+    ovs_mutex_unlock(&rule->mutex);
+}
+
+void
 ofproto_rule_reduce_timeouts(struct rule *rule,
                              uint16_t idle_timeout, uint16_t hard_timeout)
     OVS_EXCLUDED(ofproto_mutex, rule->mutex)
@@ -7254,13 +7391,13 @@  ofproto_flow_mod_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     default:
         break;
     }
+
     rule_collection_destroy(&ofm->old_rules);
     rule_collection_destroy(&ofm->new_rules);
 }
 
 static void
-ofproto_flow_mod_finish(struct ofproto *ofproto,
-                        struct ofproto_flow_mod *ofm,
+ofproto_flow_mod_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
                         const struct openflow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
 {