[ovs-dev] [PATCH v2 5/6] lib/odp, ofproto xlate: Meter execution support.

Jarno Rajahalme jrajahalme at nicira.com
Mon Aug 11 21:37:46 UTC 2014


Meter action can drop or modify packets, so the execution framework is
changed to allow for this.  Also, a meter action can appear alone
(e.g., to measure traffic that is to be dropped), so the existing drop
implementation is not sufficient any more.

The action execution framework is changed in three ways:

1. Action execution can shrink the number of packets in a batch to be
   further processed by the remaining actions.

2. Whenever a packet is 'stolen' (e.g., for output), the corresponding
   packet pointer is changed to NULL. NULLed pointers must be excluded
   from further processing by using the change #1 above.  Sometimes
   this means that the packet pointers are shuffled so that remaining
   action processing never needs to check for the NULL pointers
   explicitly.

3. Packet deletion is centralized to the odp_execute_actions()
   function.  This make memory leaks less likely to appear as new
   kinds of action are added later.

Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
---
 lib/dpif-netdev.c            |   31 ++++++++++-----------
 lib/dpif.c                   |   44 +++++++++++++++++++++++------
 lib/netdev-bsd.c             |    5 ----
 lib/netdev-dpdk.c            |   14 ++++------
 lib/netdev-dummy.c           |    8 +-----
 lib/netdev-linux.c           |    8 +-----
 lib/netdev-provider.h        |    4 +++
 lib/odp-execute.c            |   63 +++++++++++++++++++-----------------------
 lib/odp-execute.h            |   14 ++++++----
 lib/ofp-actions.c            |    1 +
 lib/ofp-actions.h            |    1 +
 ofproto/ofproto-dpif-xlate.c |   11 +++++++-
 ofproto/ofproto-dpif.c       |    5 ++--
 ofproto/ofproto-provider.h   |    3 +-
 ofproto/ofproto.c            |   47 +++++++++++++++++--------------
 15 files changed, 140 insertions(+), 119 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index c662260..1e0f05f 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1924,6 +1924,7 @@ dp_netdev_input(struct dp_netdev *dp, struct dpif_packet **packets, int cnt,
     for (i = 0; i < cnt; i++) {
         if (OVS_UNLIKELY(ofpbuf_size(&packets[i]->ofpbuf) < ETH_HEADER_LEN)) {
             dpif_packet_delete(packets[i]);
+            packets[i] = NULL;
             mfs[i] = NULL;
             continue;
         }
@@ -1952,6 +1953,7 @@ dp_netdev_input(struct dp_netdev *dp, struct dpif_packet **packets, int cnt,
             dp_netdev_queue_userspace_packet(&q, buf, DPIF_UC_MISS,
                                              mfs[i], NULL);
             dpif_packet_delete(packets[i]);
+            packets[i] = NULL;
             continue;
         }
 
@@ -2080,7 +2082,7 @@ dpif_netdev_register_upcall_cb(struct dpif *dpif, exec_upcall_cb *cb)
     dp->upcall_cb = cb;
 }
 
-static void
+static int
 dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
               struct pkt_metadata *md,
               const struct nlattr *a, bool may_steal)
@@ -2097,10 +2099,6 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
         p = dp_netdev_lookup_port(aux->dp, u32_to_odp(nl_attr_get_u32(a)));
         if (OVS_LIKELY(p)) {
             netdev_send(p->netdev, packets, cnt, may_steal);
-        } else if (may_steal) {
-            for (i = 0; i < cnt; i++) {
-                dpif_packet_delete(packets[i]);
-            }
         }
         break;
 
@@ -2123,9 +2121,6 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
             dp_netdev_queue_userspace_packet(&q, packet,
                                              DPIF_UC_ACTION, &key.flow,
                                              userdata);
-            if (may_steal) {
-                dpif_packet_delete(packets[i]);
-            }
         }
 
         if (q.packet_count) {
@@ -2178,13 +2173,16 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
                 struct dpif_packet *recirc_pkt;
                 struct pkt_metadata recirc_md = *md;
 
-                recirc_pkt = (may_steal) ? packets[i]
-                                    : dpif_packet_clone(packets[i]);
-
+                if (may_steal) {
+                    recirc_pkt = packets[i];
+                    packets[i] = NULL; /* Mark as taken. */
+                } else {
+                    recirc_pkt = dpif_packet_clone(packets[i]);
+                }
                 recirc_md.recirc_id = nl_attr_get_u32(a);
 
                 /* Hash is private to each packet */
-                recirc_md.dp_hash = packets[i]->dp_hash;
+                recirc_md.dp_hash = recirc_pkt->dp_hash;
 
                 dp_netdev_input(aux->dp, &recirc_pkt, 1, &recirc_md);
             }
@@ -2193,15 +2191,12 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
             break;
         } else {
             VLOG_WARN("Packet dropped. Max recirculation depth exceeded.");
-            if (may_steal) {
-                for (i = 0; i < cnt; i++) {
-                    dpif_packet_delete(packets[i]);
-                }
-            }
         }
         break;
 
     case OVS_ACTION_ATTR_METER:
+        break; /* Never drop. */
+
     case OVS_ACTION_ATTR_PUSH_VLAN:
     case OVS_ACTION_ATTR_POP_VLAN:
     case OVS_ACTION_ATTR_PUSH_MPLS:
@@ -2212,6 +2207,8 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
     case __OVS_ACTION_ATTR_MAX:
         OVS_NOT_REACHED();
     }
+
+    return cnt;
 }
 
 static void
diff --git a/lib/dpif.c b/lib/dpif.c
index ce941e0..d0476c2 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1068,11 +1068,12 @@ dpif_flow_dump_next(struct dpif_flow_dump_thread *thread,
 struct dpif_execute_helper_aux {
     struct dpif *dpif;
     int error;
+    const struct nlattr *meter_action; /* Non-NULL, if have a meter action. */
 };
 
 /* This is called for actions that need the context of the datapath to be
  * meaningful. */
-static void
+static int
 dpif_execute_helper_cb(void *aux_, struct dpif_packet **packets, int cnt,
                        struct pkt_metadata *md,
                        const struct nlattr *action, bool may_steal OVS_UNUSED)
@@ -1084,6 +1085,13 @@ dpif_execute_helper_cb(void *aux_, struct dpif_packet **packets, int cnt,
     ovs_assert(cnt == 1);
 
     switch ((enum ovs_action_attr)type) {
+    case OVS_ACTION_ATTR_METER:
+        /* Maintain a pointer to the first meter action seen. */
+        if (!aux->meter_action) {
+            aux->meter_action = action;
+        }
+        return cnt;
+
     case OVS_ACTION_ATTR_OUTPUT:
     case OVS_ACTION_ATTR_USERSPACE:
     case OVS_ACTION_ATTR_RECIRC: {
@@ -1091,12 +1099,28 @@ dpif_execute_helper_cb(void *aux_, struct dpif_packet **packets, int cnt,
         struct ofpbuf execute_actions;
         uint64_t stub[256 / 8];
 
-        if (md->tunnel.ip_dst) {
+        if (md->tunnel.ip_dst || aux->meter_action) {
+            ofpbuf_use_stub(&execute_actions, stub, sizeof stub);
+
+            if (aux->meter_action) {
+                const struct nlattr *a = aux->meter_action;
+
+                do {
+                    ofpbuf_put(&execute_actions, a, NLA_ALIGN(a->nla_len));
+                    /* Find next meter action before 'action', if any. */
+                    do {
+                        a = nl_attr_next(a);
+                    } while (a != action &&
+                             nl_attr_type(a) != OVS_ACTION_ATTR_METER);
+                } while (a != action);
+            }
+
             /* The Linux kernel datapath throws away the tunnel information
              * that we supply as metadata.  We have to use a "set" action to
              * supply it. */
-            ofpbuf_use_stub(&execute_actions, stub, sizeof stub);
-            odp_put_tunnel_action(&md->tunnel, &execute_actions);
+            if (md->tunnel.ip_dst) {
+                odp_put_tunnel_action(&md->tunnel, &execute_actions);
+            }
             ofpbuf_put(&execute_actions, action, NLA_ALIGN(action->nla_len));
 
             execute.actions = ofpbuf_data(&execute_actions);
@@ -1113,14 +1137,17 @@ dpif_execute_helper_cb(void *aux_, struct dpif_packet **packets, int cnt,
 
         log_execute_message(aux->dpif, &execute, true, aux->error);
 
-        if (md->tunnel.ip_dst) {
+        if (md->tunnel.ip_dst || aux->meter_action) {
             ofpbuf_uninit(&execute_actions);
+
+            /* Do not re-use the same meters for later output actions. */
+            aux->meter_action = NULL;
         }
-        break;
+
+        return (aux->error == 0) ? cnt : 0;
     }
 
     case OVS_ACTION_ATTR_HASH:
-    case OVS_ACTION_ATTR_METER:
     case OVS_ACTION_ATTR_PUSH_VLAN:
     case OVS_ACTION_ATTR_POP_VLAN:
     case OVS_ACTION_ATTR_PUSH_MPLS:
@@ -1131,6 +1158,7 @@ dpif_execute_helper_cb(void *aux_, struct dpif_packet **packets, int cnt,
     case __OVS_ACTION_ATTR_MAX:
         OVS_NOT_REACHED();
     }
+    return 0;
 }
 
 /* Executes 'execute' by performing most of the actions in userspace and
@@ -1141,7 +1169,7 @@ dpif_execute_helper_cb(void *aux_, struct dpif_packet **packets, int cnt,
 static int
 dpif_execute_with_help(struct dpif *dpif, struct dpif_execute *execute)
 {
-    struct dpif_execute_helper_aux aux = {dpif, 0};
+    struct dpif_execute_helper_aux aux = {dpif, 0, NULL};
     struct dpif_packet packet, *pp;
 
     COVERAGE_INC(dpif_execute_with_help);
diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 16efc3d..35fb6a6 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -734,11 +734,6 @@ netdev_bsd_send(struct netdev *netdev_, struct dpif_packet **pkts, int cnt,
     }
 
     ovs_mutex_unlock(&dev->mutex);
-    if (may_steal) {
-        for (i = 0; i < cnt; i++) {
-            dpif_packet_delete(pkts[i]);
-        }
-    }
 
     return error;
 }
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 6ee9803..5433861 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -703,6 +703,9 @@ dpdk_queue_pkts(struct netdev_dpdk *dev, int qid,
         }
     }
     rte_spinlock_unlock(&txq->tx_lock);
+
+    /* Clear the transferred pointers to mark them as 'taken'. */
+    memset(pkts, 0, cnt * sizeof *pkts);
 }
 
 /* Tx function. Transmit packets indefinitely */
@@ -774,12 +777,6 @@ netdev_dpdk_send(struct netdev *netdev, struct dpif_packet **pkts, int cnt,
 
     if (!may_steal || pkts[0]->ofpbuf.source != OFPBUF_DPDK) {
         dpdk_do_tx_copy(netdev, pkts, cnt);
-
-        if (may_steal) {
-            for (i = 0; i < cnt; i++) {
-                dpif_packet_delete(pkts[i]);
-            }
-        }
     } else {
         int qid;
         int next_tx_idx = 0;
@@ -793,19 +790,18 @@ netdev_dpdk_send(struct netdev *netdev, struct dpif_packet **pkts, int cnt,
                 if (next_tx_idx != i) {
                     dpdk_queue_pkts(dev, qid,
                                     (struct rte_mbuf **)&pkts[next_tx_idx],
-                                    i-next_tx_idx);
+                                    i - next_tx_idx);
                 }
 
                 VLOG_WARN_RL(&rl, "Too big size %d max_packet_len %d",
                              (int)size , dev->max_packet_len);
 
-                dpif_packet_delete(pkts[i]);
                 dropped++;
                 next_tx_idx = i + 1;
             }
         }
         if (next_tx_idx != cnt) {
-           dpdk_queue_pkts(dev, qid,
+            dpdk_queue_pkts(dev, qid,
                             (struct rte_mbuf **)&pkts[next_tx_idx],
                             cnt-next_tx_idx);
         }
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 8d1c298..acd25bc 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -847,7 +847,7 @@ netdev_dummy_rxq_drain(struct netdev_rxq *rxq_)
 
 static int
 netdev_dummy_send(struct netdev *netdev, struct dpif_packet **pkts, int cnt,
-                  bool may_steal)
+                  bool may_steal OVS_UNUSED)
 {
     struct netdev_dummy *dev = netdev_dummy_cast(netdev);
     int error = 0;
@@ -894,12 +894,6 @@ netdev_dummy_send(struct netdev *netdev, struct dpif_packet **pkts, int cnt,
         ovs_mutex_unlock(&dev->mutex);
     }
 
-    if (may_steal) {
-        for (i = 0; i < cnt; i++) {
-            dpif_packet_delete(pkts[i]);
-        }
-    }
-
     return error;
 }
 
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index e50392a..6a1eeee 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -1058,7 +1058,7 @@ netdev_linux_rxq_drain(struct netdev_rxq *rxq_)
  * expected to do additional queuing of packets. */
 static int
 netdev_linux_send(struct netdev *netdev_, struct dpif_packet **pkts, int cnt,
-                  bool may_steal)
+                  bool may_steal OVS_UNUSED)
 {
     int i;
     int error = 0;
@@ -1138,12 +1138,6 @@ netdev_linux_send(struct netdev *netdev_, struct dpif_packet **pkts, int cnt,
         i++;
     }
 
-    if (may_steal) {
-        for (i = 0; i < cnt; i++) {
-            dpif_packet_delete(pkts[i]);
-        }
-    }
-
     if (error && error != EAGAIN) {
             VLOG_WARN_RL(&rl, "error sending Ethernet packet on %s: %s",
                          netdev_get_name(netdev_), ovs_strerror(error));
diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
index 56244ad..5f36374 100644
--- a/lib/netdev-provider.h
+++ b/lib/netdev-provider.h
@@ -260,6 +260,10 @@ struct netdev_class {
      * been sent anyway.
      *
      * To retain ownership of 'buffers' caller can set may_steal to false.
+     * When 'may_steal' is set to 'true', the network device will set the
+     * pointer of each packet in 'buffers' whose ownership was taken ('stolen')
+     * to NULL.  The caller retains ownership of all packets whose pointer is
+     * non-NULL on return.
      *
      * The network device is expected to maintain a packet transmission queue,
      * so that the caller does not ordinarily have to do additional queuing of
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index fd6644d..179f6f6 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -21,13 +21,13 @@
 #include <string.h>
 
 #include "dpif.h"
+#include "flow.h"
 #include "netlink.h"
 #include "ofpbuf.h"
 #include "odp-netlink.h"
 #include "odp-util.h"
 #include "packet-dpif.h"
 #include "packets.h"
-#include "flow.h"
 #include "unaligned.h"
 #include "util.h"
 
@@ -160,14 +160,14 @@ odp_execute_set_action(struct dpif_packet *packet, const struct nlattr *a,
 
 static void
 odp_execute_actions__(void *dp, struct dpif_packet **packets, int cnt,
-                      bool steal, struct pkt_metadata *,
+                      struct pkt_metadata *,
                       const struct nlattr *actions, size_t actions_len,
-                      odp_execute_cb dp_execute_action, bool more_actions);
+                      odp_execute_cb dp_execute_action, bool may_steal);
 
 static void
-odp_execute_sample(void *dp, struct dpif_packet *packet, bool steal,
+odp_execute_sample(void *dp, struct dpif_packet **packet_p,
                    struct pkt_metadata *md, const struct nlattr *action,
-                   odp_execute_cb dp_execute_action, bool more_actions)
+                   odp_execute_cb dp_execute_action, bool may_steal)
 {
     const struct nlattr *subactions = NULL;
     const struct nlattr *a;
@@ -194,20 +194,19 @@ odp_execute_sample(void *dp, struct dpif_packet *packet, bool steal,
         }
     }
 
-    odp_execute_actions__(dp, &packet, 1, steal, md, nl_attr_get(subactions),
+    odp_execute_actions__(dp, packet_p, 1, md, nl_attr_get(subactions),
                           nl_attr_get_size(subactions), dp_execute_action,
-                          more_actions);
+                          may_steal);
 }
 
 static void
 odp_execute_actions__(void *dp, struct dpif_packet **packets, int cnt,
-                      bool steal, struct pkt_metadata *md,
+                      struct pkt_metadata *md,
                       const struct nlattr *actions, size_t actions_len,
-                      odp_execute_cb dp_execute_action, bool more_actions)
+                      odp_execute_cb dp_execute_action, bool may_steal)
 {
     const struct nlattr *a;
     unsigned int left;
-
     int i;
 
     NL_ATTR_FOR_EACH_UNSAFE (a, left, actions, actions_len) {
@@ -215,21 +214,20 @@ odp_execute_actions__(void *dp, struct dpif_packet **packets, int cnt,
 
         switch ((enum ovs_action_attr) type) {
 
-        case OVS_ACTION_ATTR_METER:
-            /* Not implemented yet. */
-            break;
-
             /* These only make sense in the context of a datapath. */
+        case OVS_ACTION_ATTR_METER:
         case OVS_ACTION_ATTR_OUTPUT:
         case OVS_ACTION_ATTR_USERSPACE:
         case OVS_ACTION_ATTR_RECIRC:
             if (dp_execute_action) {
                 /* Allow 'dp_execute_action' to steal the packet data if we do
-                 * not need it any more. */
-                bool may_steal = steal && (!more_actions
-                                           && left <= NLA_ALIGN(a->nla_len)
-                                           && type != OVS_ACTION_ATTR_RECIRC);
-                dp_execute_action(dp, packets, cnt, md, a, may_steal);
+                 * not need it any more.  As a precaution, packets actually
+                 * stolen have their pointers set to NULL, so that we can
+                 * catch bugs where the stolen packet is referenced afterwards.
+                 * 'dp_execute_action' may also effectively drop packets by
+                 * returning a count smaller than 'cnt' parameter. */
+                cnt = dp_execute_action(dp, packets, cnt, md, a, may_steal
+                                        && left <= NLA_ALIGN(a->nla_len));
             }
             break;
 
@@ -305,17 +303,14 @@ odp_execute_actions__(void *dp, struct dpif_packet **packets, int cnt,
 
         case OVS_ACTION_ATTR_SET:
             for (i = 0; i < cnt; i++) {
-                odp_execute_set_action(packets[i], nl_attr_get(a),
-                                       md);
+                odp_execute_set_action(packets[i], nl_attr_get(a), md);
             }
             break;
 
         case OVS_ACTION_ATTR_SAMPLE:
             for (i = 0; i < cnt; i++) {
-                odp_execute_sample(dp, packets[i], steal, md, a,
-                                   dp_execute_action,
-                                   more_actions ||
-                                   left > NLA_ALIGN(a->nla_len));
+                odp_execute_sample(dp, &packets[i], md, a, dp_execute_action,
+                                   may_steal && left <= NLA_ALIGN(a->nla_len));
             }
             break;
 
@@ -332,15 +327,15 @@ odp_execute_actions(void *dp, struct dpif_packet **packets, int cnt,
                     const struct nlattr *actions, size_t actions_len,
                     odp_execute_cb dp_execute_action)
 {
-    odp_execute_actions__(dp, packets, cnt, steal, md, actions, actions_len,
-                          dp_execute_action, false);
-
-    if (!actions_len && steal) {
-        /* Drop action. */
-        int i;
-
-        for (i = 0; i < cnt; i++) {
-            dpif_packet_delete(packets[i]);
+    odp_execute_actions__(dp, packets, cnt, md, actions, actions_len,
+                          dp_execute_action, steal);
+    if (steal) {
+        for (int i = 0; i < cnt; i++) {
+            /* Packets may already have been taken, e.g. by output actions. */
+            if (packets[i]) {
+                dpif_packet_delete(packets[i]);
+                packets[i] = NULL;
+            }
         }
     }
 }
diff --git a/lib/odp-execute.h b/lib/odp-execute.h
index 23dc219..9f4ca7d 100644
--- a/lib/odp-execute.h
+++ b/lib/odp-execute.h
@@ -27,14 +27,16 @@ struct nlattr;
 struct dpif_packet;
 struct pkt_metadata;
 
-typedef void (*odp_execute_cb)(void *dp, struct dpif_packet **packets, int cnt,
-                               struct pkt_metadata *,
-                               const struct nlattr *action, bool may_steal);
+/* Remaining packets are dropped if the callback returns a value < 'cnt'. */
+typedef int (*odp_execute_cb)(void *dp, struct dpif_packet **packets, int cnt,
+                              struct pkt_metadata *,
+                              const struct nlattr *action, bool may_steal);
 
 /* Actions that need to be executed in the context of a datapath are handed
- * to 'dp_execute_action', if non-NULL.  Currently this is called only for
- * actions OVS_ACTION_ATTR_OUTPUT and OVS_ACTION_ATTR_USERSPACE so
- * 'dp_execute_action' needs to handle only these. */
+ * to 'dp_execute_action', if non-NULL.
+ * The caller relinquishes ownership of the 'packets' if 'steal' is 'true'.
+ * In this case the packet pointers in 'packets' will be set to NULL to
+ * enforce the ownership transfer. */
 void odp_execute_actions(void *dp, struct dpif_packet **packets, int cnt,
                          bool steal, struct pkt_metadata *,
                          const struct nlattr *actions, size_t actions_len,
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index ed30ec2..83c2c1c 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -4835,6 +4835,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
 
         om = ofpact_put_METER(ofpacts);
         om->meter_id = ntohl(oim->meter_id);
+        om->provider_meter_id = UINT32_MAX; /* No provider meter ID. */
     }
     if (insts[OVSINST_OFPIT11_APPLY_ACTIONS]) {
         const struct ofp_action_header *actions;
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 0f1ea3f..9373cfd 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -454,6 +454,7 @@ struct ofpact_metadata {
 struct ofpact_meter {
     struct ofpact ofpact;
     uint32_t meter_id;
+    uint32_t provider_meter_id;
 };
 
 /* OFPACT_WRITE_ACTIONS.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 4aedb59..e958f9e 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3363,6 +3363,15 @@ xlate_sample_action(struct xlate_ctx *ctx,
                         probability, &cookie, sizeof cookie.flow_sample);
 }
 
+static void
+xlate_meter_action(struct xlate_ctx *ctx, const struct ofpact_meter *meter)
+{
+    if (meter->provider_meter_id != UINT32_MAX) {
+        nl_msg_put_u32(&ctx->xout->odp_actions, OVS_ACTION_ATTR_METER,
+                       meter->provider_meter_id);
+    }
+}
+
 static bool
 may_receive(const struct xport *xport, struct xlate_ctx *ctx)
 {
@@ -3756,7 +3765,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_METER:
-            /* Not implemented yet. */
+            xlate_meter_action(ctx, ofpact_get_METER(a));
             break;
 
         case OFPACT_GOTO_TABLE: {
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 856456c..6fee950 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3205,9 +3205,8 @@ rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout,
     ofproto_rule_reduce_timeouts(&rule->up, idle_timeout, hard_timeout);
 }
 
-/* Returns 'rule''s actions.  The caller owns a reference on the returned
- * actions and must eventually release it (with rule_actions_unref()) to avoid
- * a memory leak. */
+/* Returns 'rule''s actions.  The actions are managed via RCU and may be used
+ * until the calling thread quiesces. */
 const struct rule_actions *
 rule_dpif_get_actions(const struct rule_dpif *rule)
 {
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 9ba156d..6b615ff 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1624,8 +1624,7 @@ void ofproto_delete_flow(struct ofproto *,
     OVS_EXCLUDED(ofproto_mutex);
 void ofproto_flush_flows(struct ofproto *);
 
-enum ofperr ofproto_check_ofpacts(struct ofproto *,
-                                  const struct ofpact ofpacts[],
+enum ofperr ofproto_check_ofpacts(struct ofproto *, struct ofpact ofpacts[],
                                   size_t ofpacts_len);
 
 static inline const struct rule_actions *
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index b6d4875..d5dacc0 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2544,8 +2544,8 @@ ofproto_group_unref(struct ofgroup *group)
     }
 }
 
-static uint32_t get_provider_meter_id(const struct ofproto *,
-                                      uint32_t of_meter_id);
+static bool ofproto_fix_meter_action(const struct ofproto *,
+                                     struct ofpact_meter *);
 
 /* Creates and returns a new 'struct rule_actions', whose actions are a copy
  * of from the 'ofpacts_len' bytes of 'ofpacts'. */
@@ -2991,23 +2991,23 @@ reject_slave_controller(struct ofconn *ofconn)
  * for 'ofproto':
  *
  *    - If they use a meter, then 'ofproto' has that meter configured.
+ *      Updates the meter action with ofproto's datapath's provider_meter_id.
  *
  *    - If they use any groups, then 'ofproto' has that group configured.
  *
  * Returns 0 if successful, otherwise an OpenFlow error. */
 enum ofperr
 ofproto_check_ofpacts(struct ofproto *ofproto,
-                      const struct ofpact ofpacts[], size_t ofpacts_len)
+                      struct ofpact ofpacts[], size_t ofpacts_len)
 {
-    const struct ofpact *a;
-    uint32_t mid;
-
-    mid = ofpacts_get_meter(ofpacts, ofpacts_len);
-    if (mid && get_provider_meter_id(ofproto, mid) == UINT32_MAX) {
-        return OFPERR_OFPMMFC_INVALID_METER;
-    }
+    struct ofpact *a;
 
     OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
+        if (a->type == OFPACT_METER &&
+            !ofproto_fix_meter_action(ofproto, ofpact_get_METER(a))) {
+            return OFPERR_OFPMMFC_INVALID_METER;
+        }
+
         if (a->type == OFPACT_GROUP
             && !ofproto_group_exists(ofproto, ofpact_get_GROUP(a)->group_id)) {
             return OFPERR_OFPBAC_BAD_OUT_GROUP;
@@ -5044,20 +5044,27 @@ struct meter {
 };
 
 /*
- * This is used in instruction validation at flow set-up time,
- * as flows may not use non-existing meters.
- * Return value of UINT32_MAX signifies an invalid meter.
+ * This is used in instruction validation at flow set-up time, to map
+ * the OpenFlow meter ID to the corresponding datapath provider meter
+ * ID.  If either does not exist, returns false.  Otherwise updates
+ * the meter action and returns true.
  */
-static uint32_t
-get_provider_meter_id(const struct ofproto *ofproto, uint32_t of_meter_id)
+static bool
+ofproto_fix_meter_action(const struct ofproto *ofproto,
+                         struct ofpact_meter *ma)
 {
-    if (of_meter_id && of_meter_id <= ofproto->meter_features.max_meters) {
-        const struct meter *meter = ofproto->meters[of_meter_id];
-        if (meter) {
-            return meter->provider_meter_id.uint32;
+    if (ma->meter_id && ma->meter_id <= ofproto->meter_features.max_meters) {
+        const struct meter *meter = ofproto->meters[ma->meter_id];
+
+        if (meter && meter->provider_meter_id.uint32 != UINT32_MAX) {
+            /* Update the action with the provider's meter ID, so that we
+             * do not need any synchronization between ofproto_dpif_xlate
+             * and ofproto for meter table access. */
+            ma->provider_meter_id = meter->provider_meter_id.uint32;
+            return true;
         }
     }
-    return UINT32_MAX;
+    return false;
 }
 
 /* Finds the meter invoked by 'rule''s actions and adds 'rule' to the meter's
-- 
1.7.10.4




More information about the dev mailing list