[ovs-dev] [PATCH 1/2] datapath: Refactor actions in terms of match fields.

Pravin B Shelar pshelar at nicira.com
Fri Oct 7 13:03:18 PDT 2011


Almost all current actions can be expressed in the form of
push/pop/set <field>, where field is one of the match fields. We can
create three base actions and take a field. This has both a nice
symmetry and avoids inconsistencies where we can match on the vlan
TPID but not set it.
Following patch converts all actions to this new format.

Bug #7115
---
 include/openvswitch/datapath-protocol.h |   38 +++---
 lib/dpif-linux.c                        |    4 +-
 lib/dpif-netdev.c                       |  218 +++++++++++++++------------
 lib/dpif.c                              |   16 ++-
 lib/odp-util.c                          |  247 ++++++++++++++++++++++--------
 lib/odp-util.h                          |   10 +-
 ofproto/in-band.c                       |    3 +-
 ofproto/ofproto-dpif.c                  |  226 ++++++++++++++++++++---------
 utilities/ovs-dpctl.c                   |   10 +-
 9 files changed, 515 insertions(+), 257 deletions(-)

diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
index 6c89411..071f02a 100644
--- a/include/openvswitch/datapath-protocol.h
+++ b/include/openvswitch/datapath-protocol.h
@@ -288,6 +288,7 @@ struct ovs_flow_stats {
 
 enum ovs_key_type {
 	OVS_KEY_ATTR_UNSPEC,
+	OVS_KEY_ATTR_NONE,
 	OVS_KEY_ATTR_TUN_ID,    /* 64-bit tunnel ID */
 	OVS_KEY_ATTR_IN_PORT,   /* 32-bit OVS dp port number */
 	OVS_KEY_ATTR_ETHERNET,  /* struct ovs_key_ethernet */
@@ -427,28 +428,27 @@ enum ovs_sample_attr {
 
 #define OVS_SAMPLE_ATTR_MAX (__OVS_SAMPLE_ATTR_MAX - 1)
 
-/* Action types. */
+/* Base action types. */
 enum ovs_action_type {
 	OVS_ACTION_ATTR_UNSPEC,
-	OVS_ACTION_ATTR_OUTPUT,	      /* Output to switch port. */
-	OVS_ACTION_ATTR_USERSPACE,    /* Send copy to userspace. */
-	OVS_ACTION_ATTR_PUSH_VLAN,    /* Set the 802.1q TCI value. */
-	OVS_ACTION_ATTR_POP_VLAN,     /* Strip the 802.1q header. */
-	OVS_ACTION_ATTR_SET_DL_SRC,   /* Ethernet source address. */
-	OVS_ACTION_ATTR_SET_DL_DST,   /* Ethernet destination address. */
-	OVS_ACTION_ATTR_SET_NW_SRC,   /* IPv4 source address. */
-	OVS_ACTION_ATTR_SET_NW_DST,   /* IPv4 destination address. */
-	OVS_ACTION_ATTR_SET_NW_TOS,   /* IP ToS/DSCP field (6 bits). */
-	OVS_ACTION_ATTR_SET_TP_SRC,   /* TCP/UDP source port. */
-	OVS_ACTION_ATTR_SET_TP_DST,   /* TCP/UDP destination port. */
-	OVS_ACTION_ATTR_SET_TUNNEL,   /* Set the encapsulating tunnel ID. */
-	OVS_ACTION_ATTR_SET_PRIORITY, /* Set skb->priority. */
-	OVS_ACTION_ATTR_POP_PRIORITY, /* Restore original skb->priority. */
-	OVS_ACTION_ATTR_SAMPLE,       /* Execute list of actions at given
-					 probability. */
-	__OVS_ACTION_ATTR_MAX
+	OVS_ACTION_ATTR_OUTPUT,			/* Output to switch port. */
+	OVS_ACTION_ATTR_USERSPACE,		/* Send copy to userspace. */
+	OVS_ACTION_ATTR_PUSH,			/* push pakcet header. */
+	OVS_ACTION_ATTR_POP,			/* Pop packet headerheader. */
+	OVS_ACTION_ATTR_SET,			/* Set packet field(s). */
+	OVS_ACTION_ATTR_SET_PRIORITY,		/* Set skb->priority. */
+	OVS_ACTION_ATTR_RESTORE_PRIORITY,	/* Restore skb->priority. */
+	OVS_ACTION_ATTR_SAMPLE, 		/* Sample action */
 };
 
-#define OVS_ACTION_ATTR_MAX (__OVS_ACTION_ATTR_MAX - 1)
+/**
+ * OVS ACTIONS are expressed using enum ovs_action_type and enum ovs_key_type.
+ * enum ovs_action_type is like base action and ovs_key_type specifies which
+ * field to act on. e.g. push/pop/set <field>.
+ * This has both a nice symmetry and avoids inconsistencies where we can match
+ * on a key but could not set it.
+ */
+
+#define OVS_ACT_TYPE(type, key_id)	((type << 8) | key_id)
 
 #endif  /* openvswitch/datapath-protocol.h */
diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 8981500..dcd5239 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -1234,6 +1234,7 @@ dpif_linux_vport_send(int dp_ifindex, uint32_t port_no,
     struct odputil_keybuf keybuf;
     struct flow flow;
     uint64_t action;
+    uint16_t type;
 
     ofpbuf_use_const(&packet, data, size);
     flow_extract(&packet, htonll(0), 0, &flow);
@@ -1242,7 +1243,8 @@ dpif_linux_vport_send(int dp_ifindex, uint32_t port_no,
     odp_flow_key_from_flow(&key, &flow);
 
     ofpbuf_use_stack(&actions, &action, sizeof action);
-    nl_msg_put_u32(&actions, OVS_ACTION_ATTR_OUTPUT, port_no);
+    type = OVS_ACT_TYPE(OVS_ACTION_ATTR_OUTPUT, OVS_KEY_ATTR_NONE);
+    nl_msg_put_u32(&actions, type, port_no);
 
     return dpif_linux_execute__(dp_ifindex, 0, key.data, key.size,
                                 actions.data, actions.size, &packet);
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index af62bf0..b5b805d 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -702,48 +702,48 @@ dpif_netdev_validate_actions(const struct nlattr *actions,
     NL_ATTR_FOR_EACH (a, left, actions, actions_len) {
         uint16_t type = nl_attr_type(a);
         int len = odp_action_len(type);
+        const struct ovs_key_8021q *q_key;
+        const struct ovs_key_ipv4 *ipv4_key;
 
         if (len != nl_attr_get_size(a)) {
             return EINVAL;
         }
-
         switch (type) {
-        case OVS_ACTION_ATTR_OUTPUT:
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_OUTPUT, OVS_KEY_ATTR_NONE):
             if (nl_attr_get_u32(a) >= MAX_PORTS) {
                 return EINVAL;
             }
             break;
 
-        case OVS_ACTION_ATTR_USERSPACE:
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_USERSPACE, OVS_KEY_ATTR_NONE):
             break;
 
-        case OVS_ACTION_ATTR_PUSH_VLAN:
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_PUSH, OVS_KEY_ATTR_8021Q):
             *mutates = true;
-            if (nl_attr_get_be16(a) & htons(VLAN_CFI)) {
+            q_key = nl_attr_get_unspec(a, sizeof(*q_key));
+            if (!q_key || q_key->q_tci & htons(VLAN_CFI)) {
                 return EINVAL;
             }
             break;
 
-        case OVS_ACTION_ATTR_SET_NW_TOS:
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_IPV4):
             *mutates = true;
-            if (nl_attr_get_u8(a) & IP_ECN_MASK) {
+            ipv4_key = nl_attr_get_unspec(a, sizeof (*ipv4_key));
+            if (ipv4_key->ipv4_tos & IP_ECN_MASK) {
                 return EINVAL;
             }
             break;
 
-        case OVS_ACTION_ATTR_POP_VLAN:
-        case OVS_ACTION_ATTR_SET_DL_SRC:
-        case OVS_ACTION_ATTR_SET_DL_DST:
-        case OVS_ACTION_ATTR_SET_NW_SRC:
-        case OVS_ACTION_ATTR_SET_NW_DST:
-        case OVS_ACTION_ATTR_SET_TP_SRC:
-        case OVS_ACTION_ATTR_SET_TP_DST:
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_POP, OVS_KEY_ATTR_8021Q):
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_ETHERNET):
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_TCP):
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_UDP):
             *mutates = true;
             break;
 
-        case OVS_ACTION_ATTR_SET_TUNNEL:
-        case OVS_ACTION_ATTR_SET_PRIORITY:
-        case OVS_ACTION_ATTR_POP_PRIORITY:
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_TUN_ID):
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET_PRIORITY, OVS_KEY_ATTR_NONE):
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_RESTORE_PRIORITY, OVS_KEY_ATTR_NONE):
         default:
             return EOPNOTSUPP;
         }
@@ -1159,93 +1159,114 @@ dp_netdev_pop_vlan(struct ofpbuf *packet)
 }
 
 static void
-dp_netdev_set_dl_src(struct ofpbuf *packet, const uint8_t dl_addr[ETH_ADDR_LEN])
+dp_netdev_set_dl(struct ofpbuf *packet, const struct ovs_key_ethernet *eth_key)
 {
     struct eth_header *eh = packet->l2;
-    memcpy(eh->eth_src, dl_addr, sizeof eh->eth_src);
+    if (!eth_addr_equals(eth_key->eth_src, eh->eth_src)) {
+        memcpy(eh->eth_src, eth_key->eth_src, sizeof eh->eth_src);
+    }
+    if (!eth_addr_equals(eth_key->eth_dst, eh->eth_dst)) {
+        memcpy(eh->eth_dst, eth_key->eth_dst, sizeof eh->eth_dst);
+    }
+}
+
+static bool
+is_ip(const struct ofpbuf *packet, const struct flow *flow)
+{
+    return flow->dl_type == htons(ETH_TYPE_IP) && packet->l4;
 }
 
 static void
-dp_netdev_set_dl_dst(struct ofpbuf *packet, const uint8_t dl_addr[ETH_ADDR_LEN])
+dp_netdev_set_ip_addr(struct ofpbuf *packet, uint8_t nw_proto, ovs_be32 *addr,
+                      ovs_be32 new_addr)
 {
-    struct eth_header *eh = packet->l2;
-    memcpy(eh->eth_dst, dl_addr, sizeof eh->eth_dst);
+    struct ip_header *nh = packet->l3;
+
+    if (nw_proto == IPPROTO_TCP && packet->l7) {
+        struct tcp_header *th = packet->l4;
+        th->tcp_csum = recalc_csum32(th->tcp_csum, *addr, new_addr);
+    } else if (nw_proto == IPPROTO_UDP && packet->l7) {
+        struct udp_header *uh = packet->l4;
+        if (uh->udp_csum) {
+            uh->udp_csum = recalc_csum32(uh->udp_csum, *addr, new_addr);
+            if (!uh->udp_csum) {
+                uh->udp_csum = htons(0xffff);
+            }
+        }
+    }
+    nh->ip_csum = recalc_csum32(nh->ip_csum, *addr, new_addr);
+    *addr = new_addr;
 }
 
-static bool
-is_ip(const struct ofpbuf *packet, const struct flow *key)
+static void
+dp_netdev_set_ip_tos(struct ip_header *nh, uint8_t new_tos)
 {
-    return key->dl_type == htons(ETH_TYPE_IP) && packet->l4;
+    uint8_t *field = &nh->ip_tos;
+
+    /* Set the DSCP bits and preserve the ECN bits. */
+    uint8_t new = new_tos | (nh->ip_tos & IP_ECN_MASK);
+
+    nh->ip_csum = recalc_csum16(nh->ip_csum, htons((uint16_t)*field),
+			            htons((uint16_t) new));
+    *field = new;
 }
 
 static void
-dp_netdev_set_nw_addr(struct ofpbuf *packet, const struct flow *key,
-                      const struct nlattr *a)
+dp_netdev_set_nw_addr(struct ofpbuf *packet, const struct flow *flow,
+                      const struct ovs_key_ipv4 *ipv4_key)
 {
-    if (is_ip(packet, key)) {
+    if (is_ip(packet, flow)) {
         struct ip_header *nh = packet->l3;
-        ovs_be32 ip = nl_attr_get_be32(a);
-        uint16_t type = nl_attr_type(a);
-        ovs_be32 *field;
-
-        field = type == OVS_ACTION_ATTR_SET_NW_SRC ? &nh->ip_src : &nh->ip_dst;
-        if (key->nw_proto == IPPROTO_TCP && packet->l7) {
-            struct tcp_header *th = packet->l4;
-            th->tcp_csum = recalc_csum32(th->tcp_csum, *field, ip);
-        } else if (key->nw_proto == IPPROTO_UDP && packet->l7) {
-            struct udp_header *uh = packet->l4;
-            if (uh->udp_csum) {
-                uh->udp_csum = recalc_csum32(uh->udp_csum, *field, ip);
-                if (!uh->udp_csum) {
-                    uh->udp_csum = htons(0xffff);
-                }
-            }
+
+        if (nh->ip_src != ipv4_key->ipv4_src) {
+            dp_netdev_set_ip_addr(packet, flow->nw_proto, &nh->ip_src,
+                                  ipv4_key->ipv4_src);
+        }
+        if (nh->ip_dst != ipv4_key->ipv4_dst) {
+            dp_netdev_set_ip_addr(packet, flow->nw_proto, &nh->ip_dst,
+                                  ipv4_key->ipv4_dst);
+        }
+        if (nh->ip_tos != ipv4_key->ipv4_tos) {
+            dp_netdev_set_ip_tos(nh, ipv4_key->ipv4_tos);
         }
-        nh->ip_csum = recalc_csum32(nh->ip_csum, *field, ip);
-        *field = ip;
     }
 }
 
 static void
-dp_netdev_set_nw_tos(struct ofpbuf *packet, const struct flow *key,
-                     uint8_t nw_tos)
+dp_netdev_set_port(ovs_be16 *port, ovs_be16 new_port, ovs_be16 *csum)
 {
-    if (is_ip(packet, key)) {
-        struct ip_header *nh = packet->l3;
-        uint8_t *field = &nh->ip_tos;
+    *csum = recalc_csum16(*csum, *port, new_port);
+    *port = new_port;
+}
 
-        /* Set the DSCP bits and preserve the ECN bits. */
-        uint8_t new = nw_tos | (nh->ip_tos & IP_ECN_MASK);
+static void
+dp_netdev_set_tcp_port(struct ofpbuf *packet, const struct flow *flow,
+                       const struct ovs_key_tcp *tcp_key)
+{
+    if (is_ip(packet, flow)) {
+        struct tcp_header *th = packet->l4;
 
-        nh->ip_csum = recalc_csum16(nh->ip_csum, htons((uint16_t)*field),
-                htons((uint16_t) new));
-        *field = new;
+        if (th->tcp_src != tcp_key->tcp_src) {
+            dp_netdev_set_port(&th->tcp_src, tcp_key->tcp_src, &th->tcp_csum);
+        }
+        if (th->tcp_dst != tcp_key->tcp_dst) {
+            dp_netdev_set_port(&th->tcp_dst, tcp_key->tcp_dst, &th->tcp_csum);
+        }
     }
 }
 
 static void
-dp_netdev_set_tp_port(struct ofpbuf *packet, const struct flow *key,
-                      const struct nlattr *a)
+dp_netdev_set_udp_port(struct ofpbuf *packet, const struct flow *flow,
+                       const struct ovs_key_udp *udp_key)
 {
-	if (is_ip(packet, key)) {
-        uint16_t type = nl_attr_type(a);
-        ovs_be16 port = nl_attr_get_be16(a);
-        ovs_be16 *field;
-
-        if (key->nw_proto == IPPROTO_TCP && packet->l7) {
-            struct tcp_header *th = packet->l4;
-            field = (type == OVS_ACTION_ATTR_SET_TP_SRC
-                     ? &th->tcp_src : &th->tcp_dst);
-            th->tcp_csum = recalc_csum16(th->tcp_csum, *field, port);
-            *field = port;
-        } else if (key->nw_proto == IPPROTO_UDP && packet->l7) {
-            struct udp_header *uh = packet->l4;
-            field = (type == OVS_ACTION_ATTR_SET_TP_SRC
-                     ? &uh->udp_src : &uh->udp_dst);
-            uh->udp_csum = recalc_csum16(uh->udp_csum, *field, port);
-            *field = port;
-        } else {
-            return;
+    if (is_ip(packet, flow)) {
+        struct udp_header *uh = packet->l4;
+
+        if (uh->udp_dst != udp_key->udp_src) {
+            dp_netdev_set_port(&uh->udp_src, udp_key->udp_src, &uh->udp_csum);
+        }
+        if (uh->udp_dst != udp_key->udp_dst) {
+            dp_netdev_set_port(&uh->udp_dst, udp_key->udp_dst, &uh->udp_csum);
         }
     }
 }
@@ -1303,45 +1324,52 @@ dp_netdev_execute_actions(struct dp_netdev *dp,
     unsigned int left;
 
     NL_ATTR_FOR_EACH_UNSAFE (a, left, actions, actions_len) {
+        const struct ovs_key_8021q *q_key;
+        const struct ovs_key_tcp *tcp_port_key;
+        const struct ovs_key_udp *udp_port_key;
+        const struct ovs_key_ipv4 *ipv4_key;
+        const struct ovs_key_ethernet *eth_key;
+
         switch (nl_attr_type(a)) {
-        case OVS_ACTION_ATTR_OUTPUT:
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_OUTPUT, OVS_KEY_ATTR_NONE):
             dp_netdev_output_port(dp, packet, nl_attr_get_u32(a));
             break;
 
-        case OVS_ACTION_ATTR_USERSPACE:
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_USERSPACE, OVS_KEY_ATTR_NONE):
             dp_netdev_output_userspace(dp, packet, DPIF_UC_ACTION,
                                      key, nl_attr_get_u64(a));
             break;
 
-        case OVS_ACTION_ATTR_PUSH_VLAN:
-            eth_push_vlan(packet, nl_attr_get_be16(a));
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_PUSH, OVS_KEY_ATTR_8021Q):
+            q_key = nl_attr_get_unspec(a, sizeof (*q_key));
+            eth_push_vlan(packet, q_key->q_tci);
             break;
 
-        case OVS_ACTION_ATTR_POP_VLAN:
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_POP, OVS_KEY_ATTR_8021Q):
             dp_netdev_pop_vlan(packet);
             break;
 
-        case OVS_ACTION_ATTR_SET_DL_SRC:
-            dp_netdev_set_dl_src(packet, nl_attr_get_unspec(a, ETH_ADDR_LEN));
-            break;
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_ETHERNET):
+            eth_key = nl_attr_get_unspec(a, sizeof (*eth_key));
 
-        case OVS_ACTION_ATTR_SET_DL_DST:
-            dp_netdev_set_dl_dst(packet, nl_attr_get_unspec(a, ETH_ADDR_LEN));
+            dp_netdev_set_dl(packet, eth_key);
             break;
 
-        case OVS_ACTION_ATTR_SET_NW_SRC:
-        case OVS_ACTION_ATTR_SET_NW_DST:
-            dp_netdev_set_nw_addr(packet, key, a);
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_IPV4):
+            ipv4_key = nl_attr_get_unspec(a, sizeof (*ipv4_key));
+            dp_netdev_set_nw_addr(packet, key, ipv4_key);
             break;
 
-        case OVS_ACTION_ATTR_SET_NW_TOS:
-            dp_netdev_set_nw_tos(packet, key, nl_attr_get_u8(a));
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_TCP):
+            tcp_port_key = nl_attr_get_unspec(a, sizeof (*tcp_port_key));
+            dp_netdev_set_tcp_port(packet, key, tcp_port_key);
             break;
 
-        case OVS_ACTION_ATTR_SET_TP_SRC:
-        case OVS_ACTION_ATTR_SET_TP_DST:
-            dp_netdev_set_tp_port(packet, key, a);
+         case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_UDP):
+            udp_port_key = nl_attr_get_unspec(a, sizeof (*udp_port_key));
+            dp_netdev_set_udp_port(packet, key, udp_port_key);
             break;
+
         }
     }
     return 0;
diff --git a/lib/dpif.c b/lib/dpif.c
index c6940b1..7b99ba1 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -958,8 +958,15 @@ dpif_execute(struct dpif *dpif,
     if (!(error ? VLOG_DROP_WARN(&error_rl) : VLOG_DROP_DBG(&dpmsg_rl))) {
         struct ds ds = DS_EMPTY_INITIALIZER;
         char *packet = ofp_packet_to_string(buf->data, buf->size, buf->size);
+        struct flow flow;
+
         ds_put_format(&ds, "%s: execute ", dpif_name(dpif));
-        format_odp_actions(&ds, actions, actions_len);
+        if ((error = odp_flow_key_to_flow(key, key_len, &flow))) {
+            ds_put_cstr(&ds, "invalid key");
+            return error;
+        }
+
+        format_odp_actions(&ds, actions, actions_len, &flow);
         if (error) {
             ds_put_format(&ds, " failed (%s)", strerror(error));
         }
@@ -1188,8 +1195,13 @@ log_flow_message(const struct dpif *dpif, int error, const char *operation,
         dpif_flow_stats_format(stats, &ds);
     }
     if (actions || actions_len) {
+        struct flow flow;
         ds_put_cstr(&ds, ", actions:");
-        format_odp_actions(&ds, actions, actions_len);
+        if (odp_flow_key_to_flow(key, key_len, &flow)) {
+            ds_put_cstr(&ds, "invalid key");
+            return;
+        }
+        format_odp_actions(&ds, actions, actions_len, &flow);
     }
     vlog(THIS_MODULE, flow_message_log_level(error), "%s", ds_cstr(&ds));
     ds_destroy(&ds);
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 7f5158f..d537d09 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -33,6 +33,11 @@
 #include "timeval.h"
 #include "util.h"
 
+static void
+format_odp_actions__(struct ds *ds,
+                     const struct nlattr *actions, size_t actions_len,
+                     struct flow *flow);
+
 /* The interface between userspace and kernel uses an "OVS_*" prefix.
  * Since this is fairly non-specific for the OVS userspace components,
  * "ODP_*" (Open vSwitch Datapath) is used as the prefix for
@@ -42,29 +47,31 @@
 int
 odp_action_len(uint16_t type)
 {
-    if (type > OVS_ACTION_ATTR_MAX) {
-        return -1;
-    }
-
-    switch ((enum ovs_action_type) type) {
-    case OVS_ACTION_ATTR_OUTPUT: return 4;
-    case OVS_ACTION_ATTR_USERSPACE: return 8;
-    case OVS_ACTION_ATTR_PUSH_VLAN: return 2;
-    case OVS_ACTION_ATTR_POP_VLAN: return 0;
-    case OVS_ACTION_ATTR_SET_DL_SRC: return ETH_ADDR_LEN;
-    case OVS_ACTION_ATTR_SET_DL_DST: return ETH_ADDR_LEN;
-    case OVS_ACTION_ATTR_SET_NW_SRC: return 4;
-    case OVS_ACTION_ATTR_SET_NW_DST: return 4;
-    case OVS_ACTION_ATTR_SET_NW_TOS: return 1;
-    case OVS_ACTION_ATTR_SET_TP_SRC: return 2;
-    case OVS_ACTION_ATTR_SET_TP_DST: return 2;
-    case OVS_ACTION_ATTR_SET_TUNNEL: return 8;
-    case OVS_ACTION_ATTR_SET_PRIORITY: return 4;
-    case OVS_ACTION_ATTR_POP_PRIORITY: return 0;
-
-    case OVS_ACTION_ATTR_SAMPLE:
-    case OVS_ACTION_ATTR_UNSPEC:
-    case __OVS_ACTION_ATTR_MAX:
+    switch (type) {
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_OUTPUT, OVS_KEY_ATTR_NONE):
+        return sizeof(uint32_t);
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_USERSPACE, OVS_KEY_ATTR_NONE):
+        return sizeof(uint64_t);
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_PUSH, OVS_KEY_ATTR_8021Q):
+        return sizeof(struct ovs_key_8021q);
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_POP, OVS_KEY_ATTR_8021Q):
+        return 0;
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_ETHERNET):
+        return sizeof(struct ovs_key_ethernet);
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_IPV4):
+        return sizeof(struct ovs_key_ipv4);
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_TCP):
+        return sizeof(struct ovs_key_tcp);
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_UDP):
+        return sizeof(struct ovs_key_udp);
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_TUN_ID):
+        return sizeof(ovs_be64);
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET_PRIORITY, OVS_KEY_ATTR_NONE):
+        return sizeof(uint32_t);
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_RESTORE_PRIORITY, OVS_KEY_ATTR_NONE):
+        return 0;
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SAMPLE, OVS_KEY_ATTR_NONE):
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_UNSPEC, OVS_KEY_ATTR_NONE):
         return -1;
     }
 
@@ -91,7 +98,8 @@ format_generic_odp_action(struct ds *ds, const struct nlattr *a)
 }
 
 static void
-format_odp_sample_action(struct ds *ds, const struct nlattr *attr)
+format_odp_sample_action(struct ds *ds, const struct nlattr *attr,
+                         struct flow *flow)
 {
     static const struct nl_policy ovs_sample_policy[] = {
         [OVS_SAMPLE_ATTR_PROBABILITY] = { .type = NL_A_U32 },
@@ -117,20 +125,20 @@ format_odp_sample_action(struct ds *ds, const struct nlattr *attr)
     ds_put_cstr(ds, "actions(");
     nla_acts = nl_attr_get(a[OVS_SAMPLE_ATTR_ACTIONS]);
     len = nl_attr_get_size(a[OVS_SAMPLE_ATTR_ACTIONS]);
-    format_odp_actions(ds, nla_acts, len);
+    format_odp_actions__(ds, nla_acts, len, flow);
     ds_put_format(ds, "))");
 }
 
-void
-format_odp_action(struct ds *ds, const struct nlattr *a)
+static void
+format_odp_action(struct ds *ds, const struct nlattr *a,
+                  struct flow *flow)
 {
-    const uint8_t *eth;
-    ovs_be32 ip;
     struct user_action_cookie cookie;
     uint64_t data;
 
     if (nl_attr_get_size(a) != odp_action_len(nl_attr_type(a)) &&
-            nl_attr_type(a) != OVS_ACTION_ATTR_SAMPLE) {
+            nl_attr_type(a) != OVS_ACT_TYPE(OVS_ACTION_ATTR_SAMPLE,
+                                            OVS_KEY_ATTR_NONE)) {
         ds_put_format(ds, "bad length %zu, expected %d for: ",
                       nl_attr_get_size(a), odp_action_len(nl_attr_type(a)));
         format_generic_odp_action(ds, a);
@@ -138,10 +146,18 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
     }
 
     switch (nl_attr_type(a)) {
-    case OVS_ACTION_ATTR_OUTPUT:
+    const struct ovs_key_8021q *q_key;
+    const struct ovs_key_tcp *tp_key;
+    const struct ovs_key_udp *up_key;
+    const struct ovs_key_ipv4 *ipv4_key;
+    const struct ovs_key_ethernet *eth_key;
+    bool action_set = false;
+
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_OUTPUT, OVS_KEY_ATTR_NONE):
         ds_put_format(ds, "%"PRIu16, nl_attr_get_u32(a));
         break;
-    case OVS_ACTION_ATTR_USERSPACE:
+
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_USERSPACE, OVS_KEY_ATTR_NONE):
         data = nl_attr_get_u64(a);
         memcpy(&cookie, &data, sizeof(cookie));
 
@@ -158,61 +174,147 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
                           nl_attr_get_u64(a));
         }
         break;
-    case OVS_ACTION_ATTR_SET_TUNNEL:
+
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_TUN_ID):
         ds_put_format(ds, "set_tunnel(%#"PRIx64")",
                       ntohll(nl_attr_get_be64(a)));
         break;
-    case OVS_ACTION_ATTR_PUSH_VLAN:
+
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_PUSH, OVS_KEY_ATTR_8021Q):
+        q_key = nl_attr_get_unspec(a, sizeof (*q_key));
         ds_put_format(ds, "push_vlan(vid=%"PRIu16",pcp=%d)",
-                      vlan_tci_to_vid(nl_attr_get_be16(a)),
-                      vlan_tci_to_pcp(nl_attr_get_be16(a)));
+                      vlan_tci_to_vid(q_key->q_tci),
+                      vlan_tci_to_pcp(q_key->q_tci));
         break;
-    case OVS_ACTION_ATTR_POP_VLAN:
+
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_POP, OVS_KEY_ATTR_8021Q):
         ds_put_format(ds, "pop_vlan");
         break;
-    case OVS_ACTION_ATTR_SET_DL_SRC:
-        eth = nl_attr_get_unspec(a, ETH_ADDR_LEN);
-        ds_put_format(ds, "set_dl_src("ETH_ADDR_FMT")", ETH_ADDR_ARGS(eth));
-        break;
-    case OVS_ACTION_ATTR_SET_DL_DST:
-        eth = nl_attr_get_unspec(a, ETH_ADDR_LEN);
-        ds_put_format(ds, "set_dl_dst("ETH_ADDR_FMT")", ETH_ADDR_ARGS(eth));
-        break;
-    case OVS_ACTION_ATTR_SET_NW_SRC:
-        ip = nl_attr_get_be32(a);
-        ds_put_format(ds, "set_nw_src("IP_FMT")", IP_ARGS(&ip));
-        break;
-    case OVS_ACTION_ATTR_SET_NW_DST:
-        ip = nl_attr_get_be32(a);
-        ds_put_format(ds, "set_nw_dst("IP_FMT")", IP_ARGS(&ip));
+
+    case  OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_ETHERNET):
+        eth_key = nl_attr_get_unspec(a, sizeof (*eth_key));
+        if (!eth_addr_equals(flow->dl_src, eth_key->eth_src)) {
+            ds_put_format(ds, "set_dl_src("ETH_ADDR_FMT")",
+                           ETH_ADDR_ARGS(eth_key->eth_src));
+            memcpy(flow->dl_src, eth_key->eth_src, ETH_ADDR_LEN);
+            action_set = true;
+        }
+
+        if (!eth_addr_equals(flow->dl_dst, eth_key->eth_dst)) {
+            if (action_set) {
+                ds_put_format(ds, ",");
+            }
+            ds_put_format(ds, "set_dl_dst("ETH_ADDR_FMT")",
+                           ETH_ADDR_ARGS(eth_key->eth_dst));
+            memcpy(flow->dl_dst, eth_key->eth_dst, ETH_ADDR_LEN);
+            action_set = true;
+        }
+        if (!action_set) {
+            ds_put_format(ds, "set_dl(error)");
+        }
         break;
-    case OVS_ACTION_ATTR_SET_NW_TOS:
-        ds_put_format(ds, "set_nw_tos(%"PRIu8")", nl_attr_get_u8(a));
+
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_IPV4):
+        ipv4_key = nl_attr_get_unspec(a, sizeof (*ipv4_key));
+        if (flow->nw_src != ipv4_key->ipv4_src) {
+            ds_put_format(ds, "set_nw_src("IP_FMT")",
+                               IP_ARGS(&ipv4_key->ipv4_src));
+            flow->nw_src = ipv4_key->ipv4_src;
+            action_set = true;
+        }
+
+        if (flow->nw_dst != ipv4_key->ipv4_dst) {
+            if (action_set) {
+                ds_put_format(ds, ",");
+            }
+
+            ds_put_format(ds, "set_nw_dst("IP_FMT")",
+                               IP_ARGS(&ipv4_key->ipv4_dst));
+            flow->nw_dst = ipv4_key->ipv4_dst;
+            action_set = true;
+        }
+
+        if (flow->nw_tos != ipv4_key->ipv4_tos) {
+            if (action_set) {
+                ds_put_format(ds, ",");
+            }
+
+            ds_put_format(ds, "set_nw_tos(%"PRIu8")", ipv4_key->ipv4_tos);
+            flow->nw_tos = ipv4_key->ipv4_tos;
+            action_set = true;
+        }
+        if (!action_set) {
+            ds_put_format(ds, "set_nw(error)");
+        }
         break;
-    case OVS_ACTION_ATTR_SET_TP_SRC:
-        ds_put_format(ds, "set_tp_src(%"PRIu16")", ntohs(nl_attr_get_be16(a)));
+
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_TCP):
+        tp_key = nl_attr_get_unspec(a, sizeof (*tp_key));
+
+        if (flow->tp_src != tp_key->tcp_src) {
+            ds_put_format(ds, "set_tcp_src(%"PRIu16")", ntohs(tp_key->tcp_src));
+            flow->tp_src = tp_key->tcp_src;
+            action_set = true;
+        }
+
+        if (flow->tp_dst != tp_key->tcp_dst) {
+            if (action_set) {
+                ds_put_format(ds, ",");
+            }
+
+            ds_put_format(ds, "set_tcp_dst(%"PRIu16")", ntohs(tp_key->tcp_dst));
+            flow->tp_dst = tp_key->tcp_dst;
+            action_set = true;
+        }
+        if (!action_set) {
+            ds_put_format(ds, "set_tcp(error)");
+        }
         break;
-    case OVS_ACTION_ATTR_SET_TP_DST:
-        ds_put_format(ds, "set_tp_dst(%"PRIu16")", ntohs(nl_attr_get_be16(a)));
+
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_UDP):
+        up_key = nl_attr_get_unspec(a, sizeof (*up_key));
+        if (flow->tp_src != up_key->udp_src) {
+            ds_put_format(ds, "set_udp_src(%"PRIu16")", ntohs(up_key->udp_src));
+            flow->tp_src = up_key->udp_src;
+            action_set = true;
+        }
+
+        if (flow->tp_dst != up_key->udp_dst) {
+            if (action_set) {
+                ds_put_format(ds, ",");
+            }
+
+            ds_put_format(ds, "set_udp_dst(%"PRIu16")", ntohs(up_key->udp_dst));
+            flow->tp_dst = up_key->udp_dst;
+            action_set = true;
+        }
+        if (!action_set) {
+            ds_put_format(ds, "set_udp(error)");
+        }
         break;
-    case OVS_ACTION_ATTR_SET_PRIORITY:
+
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SET_PRIORITY, OVS_KEY_ATTR_NONE):
         ds_put_format(ds, "set_priority(%#"PRIx32")", nl_attr_get_u32(a));
         break;
-    case OVS_ACTION_ATTR_POP_PRIORITY:
+
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_RESTORE_PRIORITY, OVS_KEY_ATTR_NONE):
         ds_put_cstr(ds, "pop_priority");
         break;
-    case OVS_ACTION_ATTR_SAMPLE:
-        format_odp_sample_action(ds, a);
+
+    case OVS_ACT_TYPE(OVS_ACTION_ATTR_SAMPLE, OVS_KEY_ATTR_NONE):
+        format_odp_sample_action(ds, a, flow);
         break;
+
     default:
         format_generic_odp_action(ds, a);
         break;
     }
 }
 
-void
-format_odp_actions(struct ds *ds, const struct nlattr *actions,
-                   size_t actions_len)
+static void
+format_odp_actions__(struct ds *ds,
+                     const struct nlattr *actions, size_t actions_len,
+                     struct flow *flow)
 {
     if (actions_len) {
         const struct nlattr *a;
@@ -222,7 +324,7 @@ format_odp_actions(struct ds *ds, const struct nlattr *actions,
             if (a != actions) {
                 ds_put_char(ds, ',');
             }
-            format_odp_action(ds, a);
+            format_odp_action(ds, a, flow);
         }
         if (left) {
             if (left == actions_len) {
@@ -234,6 +336,16 @@ format_odp_actions(struct ds *ds, const struct nlattr *actions,
         ds_put_cstr(ds, "drop");
     }
 }
+
+void
+format_odp_actions(struct ds *ds,
+                   const struct nlattr *actions, size_t actions_len,
+                   const struct flow *flow)
+{
+    struct flow temp_flow = *flow;
+    format_odp_actions__(ds, actions, actions_len, &temp_flow);
+}
+
 
 /* Returns the correct length of the payload for a flow key attribute of the
  * specified 'type', or -1 if 'type' is unknown. */
@@ -259,6 +371,7 @@ odp_flow_key_attr_len(uint16_t type)
     case OVS_KEY_ATTR_ARP: return sizeof(struct ovs_key_arp);
     case OVS_KEY_ATTR_ND: return sizeof(struct ovs_key_nd);
 
+    case OVS_KEY_ATTR_NONE:
     case OVS_KEY_ATTR_UNSPEC:
     case __OVS_KEY_ATTR_MAX:
         return -1;
@@ -1053,6 +1166,8 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
     switch (prev_type) {
     case OVS_KEY_ATTR_UNSPEC:
         return EINVAL;
+    case OVS_KEY_ATTR_NONE:
+        return EINVAL;
 
     case OVS_KEY_ATTR_TUN_ID:
     case OVS_KEY_ATTR_IN_PORT:
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 0e4bc0b..bb0fd29 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -59,10 +59,10 @@ odp_port_to_ofp_port(uint16_t odp_port)
 }
 
 int odp_action_len(uint16_t type);
-void format_odp_action(struct ds *, const struct nlattr *);
-void format_odp_actions(struct ds *, const struct nlattr *odp_actions,
-                        size_t actions_len);
-
+void
+format_odp_actions(struct ds *ds,
+                   const struct nlattr *actions, size_t actions_len,
+                   const struct flow * flow);
 /* Upper bound on the length of a nlattr-formatted flow key.  The longest
  * nlattr-formatted flow key would be:
  *
@@ -84,7 +84,7 @@ void format_odp_actions(struct ds *, const struct nlattr *odp_actions,
 /* This is an imperfect sanity-check that ODPUTIL_FLOW_KEY_BYTES doesn't
  * need to be updated, but will at least raise awareness when new OVS
  * datapath key types are added. */
-BUILD_ASSERT_DECL(__OVS_KEY_ATTR_MAX == 14);
+BUILD_ASSERT_DECL(__OVS_KEY_ATTR_MAX == 15);
 
 /* A buffer with sufficient size and alignment to hold an nlattr-formatted flow
  * key.  An array of "struct nlattr" might not, in theory, be sufficiently
diff --git a/ofproto/in-band.c b/ofproto/in-band.c
index cd9c050..8ec1357 100644
--- a/ofproto/in-band.c
+++ b/ofproto/in-band.c
@@ -266,7 +266,8 @@ in_band_rule_check(const struct flow *flow,
         unsigned int left;
 
         NL_ATTR_FOR_EACH_UNSAFE (a, left, actions, actions_len) {
-            if (nl_attr_type(a) == OVS_ACTION_ATTR_OUTPUT
+            if (nl_attr_type(a) == OVS_ACT_TYPE(OVS_ACTION_ATTR_OUTPUT,
+                                                OVS_KEY_ATTR_NONE)
                 && nl_attr_get_u32(a) == OVSP_LOCAL) {
                 return true;
             }
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 9e8b1fb..62f4bc6 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2207,7 +2207,8 @@ execute_odp_actions(struct ofproto_dpif *ofproto, const struct flow *flow,
     int error;
 
     if (actions_len == NLA_ALIGN(NLA_HDRLEN + sizeof(uint64_t))
-        && odp_actions->nla_type == OVS_ACTION_ATTR_USERSPACE) {
+        && odp_actions->nla_type == OVS_ACT_TYPE(OVS_ACTION_ATTR_USERSPACE,
+                                                 OVS_KEY_ATTR_NONE)) {
         const struct user_action_cookie *cookie;
         struct dpif_upcall upcall;
 
@@ -2400,9 +2401,10 @@ facet_account(struct ofproto_dpif *ofproto, struct facet *facet)
     vlan_tci = facet->flow.vlan_tci;
     NL_ATTR_FOR_EACH_UNSAFE (a, left, facet->actions, facet->actions_len) {
         struct ofport_dpif *port;
+        const struct ovs_key_8021q *q_key;
 
         switch (nl_attr_type(a)) {
-        case OVS_ACTION_ATTR_OUTPUT:
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_OUTPUT, OVS_KEY_ATTR_NONE):
             port = get_odp_port(ofproto, nl_attr_get_u32(a));
             if (port && port->bundle && port->bundle->bond) {
                 bond_account(port->bundle->bond, &facet->flow,
@@ -2410,12 +2412,13 @@ facet_account(struct ofproto_dpif *ofproto, struct facet *facet)
             }
             break;
 
-        case OVS_ACTION_ATTR_POP_VLAN:
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_POP, OVS_KEY_ATTR_8021Q):
             vlan_tci = htons(0);
             break;
 
-        case OVS_ACTION_ATTR_PUSH_VLAN:
-            vlan_tci = nl_attr_get_be16(a);
+        case OVS_ACT_TYPE(OVS_ACTION_ATTR_PUSH, OVS_KEY_ATTR_8021Q):
+            q_key = nl_attr_get_unspec(a, sizeof(*q_key));
+            vlan_tci = q_key->q_tci;
             break;
         }
     }
@@ -2936,6 +2939,7 @@ send_packet(struct ofproto_dpif *ofproto, uint32_t odp_port,
     struct odputil_keybuf keybuf;
     struct flow flow;
     int error;
+    uint16_t type;
 
     flow_extract((struct ofpbuf *) packet, 0, 0, &flow);
     ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
@@ -2944,7 +2948,8 @@ send_packet(struct ofproto_dpif *ofproto, uint32_t odp_port,
     ofpbuf_init(&odp_actions, 32);
     compose_sflow_action(ofproto, &odp_actions, &flow, odp_port);
 
-    nl_msg_put_u32(&odp_actions, OVS_ACTION_ATTR_OUTPUT, odp_port);
+    type = OVS_ACT_TYPE(OVS_ACTION_ATTR_OUTPUT, OVS_KEY_ATTR_NONE);
+    nl_msg_put_u32(&odp_actions, type, odp_port);
     error = dpif_execute(ofproto->dpif,
                          key.data, key.size,
                          odp_actions.data, odp_actions.size,
@@ -2976,6 +2981,7 @@ compose_sflow_action(const struct ofproto_dpif *ofproto,
     struct user_action_cookie *cookie;
     size_t sample_offset, actions_offset;
     int user_cookie_offset, n_output;
+    uint16_t user_action, sample_action;
 
     if (!ofproto->sflow || flow->in_port == OFPP_NONE) {
         return 0;
@@ -2989,7 +2995,8 @@ compose_sflow_action(const struct ofproto_dpif *ofproto,
         n_output = 1;
     }
 
-    sample_offset = nl_msg_start_nested(odp_actions, OVS_ACTION_ATTR_SAMPLE);
+    sample_action = OVS_ACT_TYPE(OVS_ACTION_ATTR_SAMPLE, OVS_KEY_ATTR_NONE);
+    sample_offset = nl_msg_start_nested(odp_actions, sample_action);
 
     /* Number of packets out of UINT_MAX to sample. */
     probability = dpif_sflow_get_probability(ofproto->sflow);
@@ -2997,7 +3004,8 @@ compose_sflow_action(const struct ofproto_dpif *ofproto,
 
     actions_offset = nl_msg_start_nested(odp_actions, OVS_SAMPLE_ATTR_ACTIONS);
 
-    cookie = nl_msg_put_unspec_uninit(odp_actions, OVS_ACTION_ATTR_USERSPACE,
+    user_action = OVS_ACT_TYPE(OVS_ACTION_ATTR_USERSPACE, OVS_KEY_ATTR_NONE);
+    cookie = nl_msg_put_unspec_uninit(odp_actions, user_action,
 						 sizeof(*cookie));
     cookie->type = USER_ACTION_COOKIE_SFLOW;
     cookie->data = port_ifindex;
@@ -3054,91 +3062,173 @@ fix_sflow_action(struct action_xlate_ctx *ctx)
 }
 
 static void
-commit_vlan_tci(struct action_xlate_ctx *ctx, ovs_be16 vlan_tci)
+commit_vlan_action(struct action_xlate_ctx *ctx, ovs_be16 new_tci)
 {
     struct flow *base = &ctx->base_flow;
-    struct ofpbuf *odp_actions = ctx->odp_actions;
+    uint16_t type;
 
-    if (base->vlan_tci != vlan_tci) {
-        if (!(vlan_tci & htons(VLAN_CFI))) {
-            nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN);
-        } else {
-            if (base->vlan_tci != htons(0)) {
-                nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN);
-            }
-            nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_PUSH_VLAN,
-                            vlan_tci & ~htons(VLAN_CFI));
+    if (base->vlan_tci == new_tci) {
+        return;
+    }
+
+    if (!(new_tci & htons(VLAN_CFI))) {
+        type = OVS_ACT_TYPE(OVS_ACTION_ATTR_POP, OVS_KEY_ATTR_8021Q);
+        nl_msg_put_flag(ctx->odp_actions, type);
+    } else {
+        struct ovs_key_8021q q_key;
+
+        if (base->vlan_tci != htons(0)) {
+            type = OVS_ACT_TYPE(OVS_ACTION_ATTR_POP, OVS_KEY_ATTR_8021Q);
+            nl_msg_put_flag(ctx->odp_actions, type);
         }
-        base->vlan_tci = vlan_tci;
+        type = OVS_ACT_TYPE(OVS_ACTION_ATTR_PUSH, OVS_KEY_ATTR_8021Q);
+        q_key.q_tpid = 0;
+        q_key.q_tci = new_tci & ~htons(VLAN_CFI);
+        nl_msg_put_unspec(ctx->odp_actions, type, &q_key, sizeof(q_key));
     }
+    base->vlan_tci = new_tci;
 }
 
 static void
-commit_odp_actions(struct action_xlate_ctx *ctx)
+commit_set_tun_id_action(const struct flow *flow, struct flow *base,
+                         struct ofpbuf *odp_actions)
 {
-    const struct flow *flow = &ctx->flow;
-    struct flow *base = &ctx->base_flow;
-    struct ofpbuf *odp_actions = ctx->odp_actions;
+    uint16_t type;
 
-    if (base->tun_id != flow->tun_id) {
-        nl_msg_put_be64(odp_actions, OVS_ACTION_ATTR_SET_TUNNEL, flow->tun_id);
-        base->tun_id = flow->tun_id;
+    if (base->tun_id == flow->tun_id) {
+        return;
     }
+    base->tun_id = flow->tun_id;
+    type = OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_TUN_ID);
+    nl_msg_put_be64(odp_actions, type, base->tun_id);
+}
 
-    if (base->nw_src != flow->nw_src) {
-        nl_msg_put_be32(odp_actions, OVS_ACTION_ATTR_SET_NW_SRC, flow->nw_src);
-        base->nw_src = flow->nw_src;
-    }
+static void
+commit_set_port_action(const struct flow *flow, struct flow *base,
+                       struct ofpbuf *odp_actions)
+{
+    uint16_t type;
 
-    if (base->nw_dst != flow->nw_dst) {
-        nl_msg_put_be32(odp_actions, OVS_ACTION_ATTR_SET_NW_DST, flow->nw_dst);
-        base->nw_dst = flow->nw_dst;
+    if (base->tp_src == flow->tp_src &&
+        base->tp_dst == flow->tp_dst) {
+        return;
     }
 
-    if (base->nw_tos != flow->nw_tos) {
-        nl_msg_put_u8(odp_actions, OVS_ACTION_ATTR_SET_NW_TOS, flow->nw_tos);
-        base->nw_tos = flow->nw_tos;
-    }
+    base->tp_src = flow->tp_src;
+    base->tp_dst = flow->tp_dst;
 
-    commit_vlan_tci(ctx, flow->vlan_tci);
+    if (flow->nw_proto == IPPROTO_TCP) {
+        struct ovs_key_tcp port_key;
 
-    if (base->tp_src != flow->tp_src) {
-        nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_SET_TP_SRC, flow->tp_src);
-        base->tp_src = flow->tp_src;
-    }
+        port_key.tcp_src = base->tp_src;
+        port_key.tcp_dst = base->tp_dst;
+
+        type = OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_TCP);
+        nl_msg_put_unspec(odp_actions, type, &port_key, sizeof(port_key));
+    } else if (flow->nw_proto == IPPROTO_UDP) {
+        struct ovs_key_udp port_key;
+
+        port_key.udp_src = base->tp_src ;
+        port_key.udp_dst = base->tp_dst;
 
-    if (base->tp_dst != flow->tp_dst) {
-        nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_SET_TP_DST, flow->tp_dst);
-        base->tp_dst = flow->tp_dst;
+        type = OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_UDP);
+        nl_msg_put_unspec(odp_actions, type, &port_key, sizeof(port_key));
+   } else {
+        VLOG_WARN_RL(&rl, "Unknow protocol : %"PRIu16, flow->nw_proto);
+   }
+
+}
+
+static void
+commit_set_nw_action(const struct flow *flow, struct flow *base,
+                     struct ofpbuf *odp_actions)
+{
+    struct ovs_key_ipv4 ipv4_key;
+    uint16_t type;
+
+    if (base->nw_src == flow->nw_src &&
+        base->nw_dst == flow->nw_dst &&
+        base->nw_tos == flow->nw_tos) {
+        return ;
     }
 
-    if (!eth_addr_equals(base->dl_src, flow->dl_src)) {
-        nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_SET_DL_SRC,
-                          flow->dl_src, ETH_ADDR_LEN);
-        memcpy(base->dl_src, flow->dl_src, ETH_ADDR_LEN);
+    base->nw_src = flow->nw_src;
+    base->nw_dst = flow->nw_dst;
+    base->nw_tos = flow->nw_tos;
+
+    ipv4_key.ipv4_src = base->nw_src;
+    ipv4_key.ipv4_dst = base->nw_dst;
+    ipv4_key.ipv4_tos = base->nw_tos;
+
+    type = OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_IPV4);
+    nl_msg_put_unspec(odp_actions, type, &ipv4_key, sizeof(ipv4_key));
+}
+
+static void
+commit_set_ether_addr_action(const struct flow *flow, struct flow *base,
+                             struct ofpbuf *odp_actions)
+{
+    struct ovs_key_ethernet eth_key;
+    uint16_t type;
+
+    if (eth_addr_equals(base->dl_src, flow->dl_src) &&
+        eth_addr_equals(base->dl_dst, flow->dl_dst)) {
+        return;
     }
+    memcpy(base->dl_src, flow->dl_src, ETH_ADDR_LEN);
+    memcpy(base->dl_dst, flow->dl_dst, ETH_ADDR_LEN);
+
+    memcpy(eth_key.eth_src, base->dl_src, ETH_ADDR_LEN);
+    memcpy(eth_key.eth_dst, base->dl_dst, ETH_ADDR_LEN);
+
+    type = OVS_ACT_TYPE(OVS_ACTION_ATTR_SET, OVS_KEY_ATTR_ETHERNET);
+    nl_msg_put_unspec(odp_actions, type, &eth_key, sizeof(eth_key));
+}
+
+static void
+commit_priority_action(struct action_xlate_ctx *ctx)
+{
+    struct ofpbuf *odp_actions;
+    uint16_t type;
 
-    if (!eth_addr_equals(base->dl_dst, flow->dl_dst)) {
-        nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_SET_DL_DST,
-                          flow->dl_dst, ETH_ADDR_LEN);
-        memcpy(base->dl_dst, flow->dl_dst, ETH_ADDR_LEN);
+    if (ctx->base_priority == ctx->priority) {
+        return;
     }
 
-    if (ctx->base_priority != ctx->priority) {
-        if (ctx->priority) {
-            nl_msg_put_u32(odp_actions, OVS_ACTION_ATTR_SET_PRIORITY,
-                           ctx->priority);
-        } else {
-            nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_PRIORITY);
-        }
-        ctx->base_priority = ctx->priority;
+    odp_actions = ctx->odp_actions;
+    if (ctx->priority) {
+        type = OVS_ACT_TYPE(OVS_ACTION_ATTR_SET_PRIORITY, OVS_KEY_ATTR_NONE);
+        nl_msg_put_u32(odp_actions, type, ctx->priority);
+    } else {
+        type = OVS_ACT_TYPE(OVS_ACTION_ATTR_RESTORE_PRIORITY,
+				 OVS_KEY_ATTR_NONE);
+        nl_msg_put_flag(odp_actions, type);
     }
+    ctx->base_priority = ctx->priority;
+}
+
+static void
+commit_odp_actions(struct action_xlate_ctx *ctx)
+{
+    const struct flow *flow = &ctx->flow;
+    struct flow *base = &ctx->base_flow;
+    struct ofpbuf *odp_actions = ctx->odp_actions;
+
+    commit_set_tun_id_action(flow, base, odp_actions);
+    commit_set_nw_action(flow, base, odp_actions);
+    commit_set_port_action(flow, base, odp_actions);
+    commit_set_ether_addr_action(flow, base, odp_actions);
+    commit_priority_action(ctx);
+    commit_vlan_action(ctx, flow->vlan_tci);
 }
 
 static void
 compose_output_action(struct action_xlate_ctx *ctx, uint16_t odp_port)
 {
-    nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_OUTPUT, odp_port);
+    uint16_t type;
+
+    type = OVS_ACT_TYPE(OVS_ACTION_ATTR_OUTPUT, OVS_KEY_ATTR_NONE);
+    nl_msg_put_u32(ctx->odp_actions, type, odp_port);
     ctx->sflow_odp_port = odp_port;
     ctx->sflow_n_outputs++;
 }
@@ -3255,14 +3345,15 @@ static void
 compose_controller_action(struct ofpbuf *odp_actions, int len)
 {
     struct user_action_cookie cookie;
+    uint16_t type;
 
     cookie.type = USER_ACTION_COOKIE_CONTROLLER;
     cookie.data = len;
     cookie.n_output = 0;
     cookie.vlan_tci = 0;
 
-    nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_USERSPACE,
-                                       &cookie, sizeof(cookie));
+    type = OVS_ACT_TYPE(OVS_ACTION_ATTR_USERSPACE, OVS_KEY_ATTR_NONE);
+    nl_msg_put_unspec(odp_actions, type, &cookie, sizeof(cookie));
 }
 
 static void
@@ -4010,7 +4101,7 @@ compose_actions(struct action_xlate_ctx *ctx, uint16_t vlan,
             if (tci) {
                 tci |= htons(VLAN_CFI);
             }
-            commit_vlan_tci(ctx, tci);
+            commit_vlan_action(ctx, tci);
 
             cur_vid = dst->vid;
         }
@@ -4643,7 +4734,8 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, const char *args_,
         ds_put_char(&result, '\n');
         trace_format_flow(&result, 0, "Final flow", &trace);
         ds_put_cstr(&result, "Datapath actions: ");
-        format_odp_actions(&result, odp_actions->data, odp_actions->size);
+        format_odp_actions(&result, odp_actions->data, odp_actions->size,
+                           &trace.ctx.base_flow);
         ofpbuf_delete(odp_actions);
 
         if (!trace.ctx.may_set_up_flow) {
diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index 4d0d3c2..aab0695 100644
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -42,6 +42,7 @@
 #include "timeval.h"
 #include "util.h"
 #include "vlog.h"
+#include "lib/flow.h"
 
 VLOG_DEFINE_THIS_MODULE(dpctl);
 
@@ -569,12 +570,19 @@ do_dump_flows(int argc OVS_UNUSED, char *argv[])
     dpif_flow_dump_start(&dump, dpif);
     while (dpif_flow_dump_next(&dump, &key, &key_len,
                                &actions, &actions_len, &stats)) {
+        struct flow flow;
+
         ds_clear(&ds);
         odp_flow_key_format(key, key_len, &ds);
         ds_put_cstr(&ds, ", ");
         dpif_flow_stats_format(stats, &ds);
         ds_put_cstr(&ds, ", actions:");
-        format_odp_actions(&ds, actions, actions_len);
+        if (odp_flow_key_to_flow(key, key_len, &flow)) {
+            ds_put_cstr(&ds, "invalid key");
+            printf("%s\n", ds_cstr(&ds));
+            continue;
+        }
+        format_odp_actions(&ds, actions, actions_len, &flow);
         printf("%s\n", ds_cstr(&ds));
     }
     dpif_flow_dump_done(&dump);
-- 
1.7.1




More information about the dev mailing list