ofproto: Meter translation.

Translate OpenFlow METER instructions to datapath meter actions.

Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
Signed-off-by: Andy Zhou <azhou@ovn.org>

include/openvswitch/ofp-actions.h

@@ -534,6 +534,7 @@ struct ofpact_metadata {
 struct ofpact_meter {
     struct ofpact ofpact;
     uint32_t meter_id;
+    uint32_t provider_meter_id;
 };
 
 /* OFPACT_WRITE_ACTIONS, OFPACT_CLONE.

lib/dpif.c

@@ -1104,6 +1104,7 @@ struct dpif_execute_helper_aux {
     struct dpif *dpif;
     const struct flow *flow;
     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
@@ -1119,6 +1120,13 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
     ovs_assert(packets_->count == 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;
+        }
+	break;
+
     case OVS_ACTION_ATTR_CT:
     case OVS_ACTION_ATTR_OUTPUT:
     case OVS_ACTION_ATTR_TUNNEL_PUSH:
@@ -1129,15 +1137,36 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
         struct ofpbuf execute_actions;
         uint64_t stub[256 / 8];
         struct pkt_metadata *md = &packet->md;
-        bool dst_set;
 
-        dst_set = flow_tnl_dst_is_set(&md->tunnel);
-        if (dst_set) {
+        if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_action) {
+            ofpbuf_use_stub(&execute_actions, stub, sizeof stub);
+
+            if (aux->meter_action) {
+                const struct nlattr *a = aux->meter_action;
+
+                /* XXX: This code collects meter actions since the last action
+                 * execution via the datapath to be executed right before the
+                 * current action that needs to be executed by the datapath.
+                 * This is only an approximation, but better than nothing.
+                 * Fundamentally, we should have a mechanism by which the
+                 * datapath could return the result of the meter action so that
+                 * we could execute them at the right order. */
+                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 = execute_actions.data;
@@ -1170,14 +1199,16 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
 
         dp_packet_delete(clone);
 
-        if (dst_set) {
+        if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_action) {
             ofpbuf_uninit(&execute_actions);
+
+            /* Do not re-use the same meters for later output actions. */
+            aux->meter_action = NULL;
         }
         break;
     }
 
     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:
@@ -1203,7 +1234,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
 static int
 dpif_execute_with_help(struct dpif *dpif, struct dpif_execute *execute)
 {
-    struct dpif_execute_helper_aux aux = {dpif, execute->flow, 0};
+    struct dpif_execute_helper_aux aux = {dpif, execute->flow, 0, NULL};
     struct dp_packet_batch pb;
 
     COVERAGE_INC(dpif_execute_with_help);

lib/ofp-actions.c

@@ -6938,6 +6938,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;

ofproto/ofproto-dpif-xlate.c

@@ -4747,6 +4747,15 @@ xlate_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc)
     ctx->was_mpls = old_was_mpls;
 }
 
+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->odp_actions, OVS_ACTION_ATTR_METER,
+                       meter->provider_meter_id);
+    }
+}
+
 static bool
 may_receive(const struct xport *xport, struct xlate_ctx *ctx)
 {
@@ -5556,7 +5565,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: {

ofproto/ofproto.c

@@ -3004,8 +3004,8 @@ remove_groups_rcu(struct ofgroup **groups)
     free(groups);
 }
 
-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'. */
@@ -3398,6 +3398,7 @@ 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.
  *
@@ -3409,16 +3410,16 @@ ofproto_check_ofpacts(struct ofproto *ofproto,
                       const struct ofpact ofpacts[], size_t ofpacts_len)
     OVS_REQUIRES(ofproto_mutex)
 {
-    uint32_t mid;
+    const struct ofpact *a;
 
-    mid = ofpacts_get_meter(ofpacts, ofpacts_len);
-    if (mid && get_provider_meter_id(ofproto, mid) == UINT32_MAX) {
-        return OFPERR_OFPMMFC_INVALID_METER;
-    }
+    OFPACT_FOR_EACH_FLATTENED (a, ofpacts, ofpacts_len) {
+        if (a->type == OFPACT_METER &&
+            !ofproto_fix_meter_action(ofproto, ofpact_get_METER(a))) {
+            return OFPERR_OFPMMFC_INVALID_METER;
+        }
 
-    const struct ofpact_group *a;
-    OFPACT_FOR_EACH_TYPE_FLATTENED (a, GROUP, ofpacts, ofpacts_len) {
-        if (!ofproto_group_exists(ofproto, a->group_id)) {
+        if (a->type == OFPACT_GROUP
+            && !ofproto_group_exists(ofproto, ofpact_get_GROUP(a)->group_id)) {
             return OFPERR_OFPBAC_BAD_OUT_GROUP;
         }
     }
@@ -6149,20 +6150,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