diff mbox series

[ovs-dev,ACL,Meters,6/7] ofproto: Add support for specifying a meter in controller actions.

Message ID 20180730064638.121021-6-jpettit@ovn.org
State Accepted
Headers show
Series [ovs-dev,ACL,Meters,1/7] ovn: Use C strings instead of ds for extended tables. | expand

Commit Message

Justin Pettit July 30, 2018, 6:46 a.m. UTC
Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
 include/openvswitch/ofp-actions.h |  8 ++++++
 lib/ofp-actions.c                 | 27 +++++++++++++++++--
 ofproto/ofproto-dpif-xlate.c      | 24 +++++++++++++----
 ofproto/ofproto-dpif.c            |  1 +
 ofproto/ofproto.c                 | 44 +++++++++++++++++++++++++++++++
 utilities/ovs-ofctl.8.in          |  4 +++
 6 files changed, 101 insertions(+), 7 deletions(-)

Comments

Ben Pfaff July 30, 2018, 7:49 p.m. UTC | #1
On Sun, Jul 29, 2018 at 11:46:37PM -0700, Justin Pettit wrote:
> Signed-off-by: Justin Pettit <jpettit@ovn.org>

In ofproto_check_ofpacts(), the other checks, if they fail, return an
error and prevent the flow from being added.  The new one doesn't; is
the difference intentional?  Similarly, is there anything that prevents
a meter from being deleted while still in use and, if that happens, does
anything particularly bad happen?

In ovs-ofctl.8.in, the wording seems a little vague because "associate"
is such a weak word.  Maybe change
    Associate packets sent to the controller with meter \fIid\fR.
to
    Use meter \fIid\fR to rate-limit the OpenFlow packet-in messages
    that this action sends to the controller.
or something similarly descriptive

Do only "drop" actions make sense for controller metering?

Please add some tests for the new action to the test "ovn -- action
parsing" in ovn.at.

Please document the new action in ovn-sb.xml.

Acked-by: Ben Pfaff <blp@ovn.org>

Thanks,

Ben.
Ben Pfaff July 30, 2018, 7:51 p.m. UTC | #2
On Mon, Jul 30, 2018 at 12:49:51PM -0700, Ben Pfaff wrote:
> On Sun, Jul 29, 2018 at 11:46:37PM -0700, Justin Pettit wrote:
> > Signed-off-by: Justin Pettit <jpettit@ovn.org>
> 
> In ofproto_check_ofpacts(), the other checks, if they fail, return an
> error and prevent the flow from being added.  The new one doesn't; is
> the difference intentional?  Similarly, is there anything that prevents
> a meter from being deleted while still in use and, if that happens, does
> anything particularly bad happen?
> 
> In ovs-ofctl.8.in, the wording seems a little vague because "associate"
> is such a weak word.  Maybe change
>     Associate packets sent to the controller with meter \fIid\fR.
> to
>     Use meter \fIid\fR to rate-limit the OpenFlow packet-in messages
>     that this action sends to the controller.
> or something similarly descriptive
> 
> Do only "drop" actions make sense for controller metering?
> 
> Please add some tests for the new action to the test "ovn -- action
> parsing" in ovn.at.
> 
> Please document the new action in ovn-sb.xml.
> 
> Acked-by: Ben Pfaff <blp@ovn.org>

Oops, some of the above was for patch 6 and some for patch 7.  I think
you can figure it out though.

I'll look at both again to see if I missed anything.
Justin Pettit July 31, 2018, 12:55 a.m. UTC | #3
> On Jul 30, 2018, at 12:49 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Sun, Jul 29, 2018 at 11:46:37PM -0700, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> 
> In ofproto_check_ofpacts(), the other checks, if they fail, return an
> error and prevent the flow from being added.  The new one doesn't; is
> the difference intentional?

Yes.  My thought is that those controller actions are likely important, so if the meter doesn't exist, it's still safer to set up the flow and log a warning.

> Similarly, is there anything that prevents
> a meter from being deleted while still in use and, if that happens, does
> anything particularly bad happen?

Nothing prevents it.  However, if it happens, then the metering just stops being enforced, and the other actions are unaffected.

> In ovs-ofctl.8.in, the wording seems a little vague because "associate"
> is such a weak word.  Maybe change
>    Associate packets sent to the controller with meter \fIid\fR.
> to
>    Use meter \fIid\fR to rate-limit the OpenFlow packet-in messages
>    that this action sends to the controller.
> or something similarly descriptive

Yes, that is better.

> Do only "drop" actions make sense for controller metering?

Probably.  I added some documentation to that effect to the description of the "meter" argument to the log() action in ovn-sb.xml.

> Please add some tests for the new action to the test "ovn -- action
> parsing" in ovn.at.
> 
> Please document the new action in ovn-sb.xml.

Which new action?  I've extended the log() action documentation to add "meter", which was missing before.  There was no existing log action parsing test.  I'll add that as a separate patch, since I think the existing log() parsing code could use a little cleanup in that area, too.

Thanks!

--Justin
diff mbox series

Patch

diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index b3dd0959d53e..3b0350496294 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -287,6 +287,8 @@  struct ofpact_output {
     uint16_t max_len;           /* Max send len, for port OFPP_CONTROLLER. */
 };
 
+#define NX_CTLR_NO_METER 0
+
 /* OFPACT_CONTROLLER.
  *
  * Used for NXAST_CONTROLLER. */
@@ -305,6 +307,12 @@  struct ofpact_controller {
         /* Arbitrary data to include in the packet-in message (currently,
          * only in NXT_PACKET_IN2). */
         uint16_t userdata_len;
+
+        /* Meter to which this controller action should be associated.
+         * If requested, this will override a "controller" virtual meter.
+         * A value of NX_CTLR_NO_METER means no meter is requested. */
+        uint32_t meter_id;
+        uint32_t provider_meter_id;
     );
     uint8_t userdata[0];
 };
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index fa94cbbd9436..5aacff62a34a 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -754,6 +754,7 @@  enum nx_action_controller2_prop_type {
     NXAC2PT_REASON,             /* uint8_t reason (OFPR_*), default 0. */
     NXAC2PT_USERDATA,           /* Data to copy into NXPINT_USERDATA. */
     NXAC2PT_PAUSE,              /* Flag to pause pipeline to resume later. */
+    NXAC2PT_METER_ID,           /* ovs_b32 meter (default NX_CTLR_NO_METER). */
 };
 
 /* The action structure for NXAST_CONTROLLER2 is "struct ext_action_header",
@@ -771,6 +772,7 @@  decode_NXAST_RAW_CONTROLLER(const struct nx_action_controller *nac,
     oc->max_len = ntohs(nac->max_len);
     oc->controller_id = ntohs(nac->controller_id);
     oc->reason = nac->reason;
+    oc->meter_id = NX_CTLR_NO_METER;
     ofpact_finish_CONTROLLER(out, &oc);
 
     return 0;
@@ -790,6 +792,7 @@  decode_NXAST_RAW_CONTROLLER2(const struct ext_action_header *eah,
     oc->ofpact.raw = NXAST_RAW_CONTROLLER2;
     oc->max_len = UINT16_MAX;
     oc->reason = OFPR_ACTION;
+    oc->meter_id = NX_CTLR_NO_METER;
 
     struct ofpbuf properties;
     ofpbuf_use_const(&properties, eah, ntohs(eah->len));
@@ -831,6 +834,10 @@  decode_NXAST_RAW_CONTROLLER2(const struct ext_action_header *eah,
             oc->pause = true;
             break;
 
+        case NXAC2PT_METER_ID:
+            error = ofpprop_parse_u32(&payload, &oc->meter_id);
+            break;
+
         default:
             error = OFPPROP_UNKNOWN(false, "NXAST_RAW_CONTROLLER2", type);
             break;
@@ -852,6 +859,7 @@  encode_CONTROLLER(const struct ofpact_controller *controller,
 {
     if (controller->userdata_len
         || controller->pause
+        || controller->meter_id != NX_CTLR_NO_METER
         || controller->ofpact.raw == NXAST_RAW_CONTROLLER2) {
         size_t start_ofs = out->size;
         put_NXAST_CONTROLLER2(out);
@@ -872,6 +880,9 @@  encode_CONTROLLER(const struct ofpact_controller *controller,
         if (controller->pause) {
             ofpprop_put_flag(out, NXAC2PT_PAUSE);
         }
+        if (controller->meter_id != NX_CTLR_NO_METER) {
+            ofpprop_put_u32(out, NXAC2PT_METER_ID, controller->meter_id);
+        }
         pad_ofpat(out, start_ofs);
     } else {
         struct nx_action_controller *nac;
@@ -889,6 +900,7 @@  parse_CONTROLLER(char *arg, const struct ofpact_parse_params *pp)
     enum ofp_packet_in_reason reason = OFPR_ACTION;
     uint16_t controller_id = 0;
     uint16_t max_len = UINT16_MAX;
+    uint32_t meter_id = NX_CTLR_NO_METER;
     const char *userdata = NULL;
     bool pause = false;
 
@@ -921,6 +933,11 @@  parse_CONTROLLER(char *arg, const struct ofpact_parse_params *pp)
                 userdata = value;
             } else if (!strcmp(name, "pause")) {
                 pause = true;
+            } else if (!strcmp(name, "meter_id")) {
+                char *error = str_to_u32(value, &meter_id);
+                if (error) {
+                    return error;
+                }
             } else {
                 return xasprintf("unknown key \"%s\" parsing controller "
                                  "action", name);
@@ -928,7 +945,8 @@  parse_CONTROLLER(char *arg, const struct ofpact_parse_params *pp)
         }
     }
 
-    if (reason == OFPR_ACTION && controller_id == 0 && !userdata && !pause) {
+    if (reason == OFPR_ACTION && controller_id == 0 && !userdata && !pause
+        && meter_id == NX_CTLR_NO_METER) {
         struct ofpact_output *output;
 
         output = ofpact_put_OUTPUT(pp->ofpacts);
@@ -942,6 +960,7 @@  parse_CONTROLLER(char *arg, const struct ofpact_parse_params *pp)
         controller->reason = reason;
         controller->controller_id = controller_id;
         controller->pause = pause;
+        controller->meter_id = meter_id;
 
         if (userdata) {
             size_t start_ofs = pp->ofpacts->size;
@@ -976,7 +995,7 @@  format_CONTROLLER(const struct ofpact_controller *a,
                   const struct ofpact_format_params *fp)
 {
     if (a->reason == OFPR_ACTION && !a->controller_id && !a->userdata_len
-        && !a->pause) {
+        && !a->pause && a->meter_id == NX_CTLR_NO_METER) {
         ds_put_format(fp->s, "%sCONTROLLER:%s%"PRIu16,
                       colors.special, colors.end, a->max_len);
     } else {
@@ -1006,6 +1025,10 @@  format_CONTROLLER(const struct ofpact_controller *a,
         if (a->pause) {
             ds_put_format(fp->s, "%spause%s,", colors.value, colors.end);
         }
+        if (a->meter_id != NX_CTLR_NO_METER) {
+            ds_put_format(fp->s, "%smeter_id=%s%"PRIu32",",
+                          colors.param, colors.end, a->meter_id);
+        }
         ds_chomp(fp->s, ',');
         ds_put_format(fp->s, "%s)%s", colors.paren, colors.end);
     }
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index dc63afa35a6b..01f1fafeb356 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4619,6 +4619,7 @@  static void
 xlate_controller_action(struct xlate_ctx *ctx, int len,
                         enum ofp_packet_in_reason reason,
                         uint16_t controller_id,
+                        uint32_t provider_meter_id,
                         const uint8_t *userdata, size_t userdata_len)
 {
     xlate_commit_actions(ctx);
@@ -4655,9 +4656,21 @@  xlate_controller_action(struct xlate_ctx *ctx, int len,
     }
     recirc_refs_add(&ctx->xout->recircs, recirc_id);
 
+    /* If the controller action didn't request a meter (indicated by a
+     * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was
+     * configured through the "controller" virtual meter.
+     *
+     * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is
+     * configured. */
+    uint32_t meter_id;
+    if (provider_meter_id == UINT32_MAX) {
+        meter_id = ctx->xbridge->ofproto->up.controller_meter_id;
+    } else {
+        meter_id = provider_meter_id;
+    }
+
     size_t offset;
     size_t ac_offset;
-    uint32_t meter_id = ctx->xbridge->ofproto->up.controller_meter_id;
     if (meter_id != UINT32_MAX) {
         /* If controller meter is configured, generate clone(meter, userspace)
          * action. */
@@ -4852,7 +4865,7 @@  compose_dec_ttl(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids)
 
         for (i = 0; i < ids->n_controllers; i++) {
             xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL,
-                                    ids->cnt_ids[i], NULL, 0);
+                                    ids->cnt_ids[i], UINT32_MAX, NULL, 0);
         }
 
         /* Stop processing for current table. */
@@ -4893,7 +4906,7 @@  compose_dec_nsh_ttl_action(struct xlate_ctx *ctx)
             return false;
         } else {
             xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL,
-                                    0, NULL, 0);
+                                    0, UINT32_MAX, NULL, 0);
         }
     }
 
@@ -4926,7 +4939,7 @@  compose_dec_mpls_ttl_action(struct xlate_ctx *ctx)
             return false;
         } else {
             xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL, 0,
-                                    NULL, 0);
+                                    UINT32_MAX, NULL, 0);
         }
     }
 
@@ -4985,7 +4998,7 @@  xlate_output_action(struct xlate_ctx *ctx, ofp_port_t port,
                                  : group_bucket_action ? OFPR_GROUP
                                  : ctx->in_action_set ? OFPR_ACTION_SET
                                  : OFPR_ACTION),
-                                0, NULL, 0);
+                                0, UINT32_MAX, NULL, 0);
         break;
     case OFPP_NONE:
         break;
@@ -6392,6 +6405,7 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
                 xlate_controller_action(ctx, controller->max_len,
                                         controller->reason,
                                         controller->controller_id,
+                                        controller->provider_meter_id,
                                         controller->userdata,
                                         controller->userdata_len);
             }
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ad1e8af43f6e..afe333e35166 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1520,6 +1520,7 @@  add_internal_flows(struct ofproto_dpif *ofproto)
     controller->max_len = UINT16_MAX;
     controller->controller_id = 0;
     controller->reason = OFPR_IMPLICIT_MISS;
+    controller->meter_id = NX_CTLR_NO_METER;
     ofpact_finish_CONTROLLER(&ofpacts, &controller);
 
     error = add_internal_miss_flow(ofproto, id++, &ofpacts,
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index f946e27b77a9..e71431129af9 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3017,6 +3017,9 @@  remove_groups_rcu(struct ofgroup **groups)
 static bool ofproto_fix_meter_action(const struct ofproto *,
                                      struct ofpact_meter *);
 
+static bool ofproto_fix_controller_action(const struct ofproto *,
+                                          struct ofpact_controller *);
+
 /* Creates and returns a new 'struct rule_actions', whose actions are a copy
  * of from the 'ofpacts_len' bytes of 'ofpacts'. */
 const struct rule_actions *
@@ -3429,6 +3432,17 @@  ofproto_check_ofpacts(struct ofproto *ofproto,
             return OFPERR_OFPMMFC_INVALID_METER;
         }
 
+        if (a->type == OFPACT_CONTROLLER) {
+            struct ofpact_controller *ca = ofpact_get_CONTROLLER(a);
+
+            if (!ofproto_fix_controller_action(ofproto, ca)) {
+                static struct vlog_rate_limit rl2 = VLOG_RATE_LIMIT_INIT(1, 5);
+                VLOG_INFO_RL(&rl2, "%s: controller action specified an "
+                             "unknown meter id: %d",
+                             ofproto->name, ca->meter_id);
+            }
+        }
+
         if (a->type == OFPACT_GROUP
             && !ofproto_group_exists(ofproto, ofpact_get_GROUP(a)->group_id)) {
             return OFPERR_OFPBAC_BAD_OUT_GROUP;
@@ -6292,6 +6306,36 @@  ofproto_fix_meter_action(const struct ofproto *ofproto,
     return false;
 }
 
+/* This is used in instruction validation at flow set-up time, to map
+ * the OpenFlow meter ID in a controller action to the corresponding
+ * datapath provider meter ID.  If either does not exist, sets the
+ * provider meter id to a value to prevent the provider from using it
+ * and returns false.  Otherwise, updates the meter action and returns
+ * true. */
+static bool
+ofproto_fix_controller_action(const struct ofproto *ofproto,
+                              struct ofpact_controller *ca)
+{
+    if (ca->meter_id == NX_CTLR_NO_METER) {
+        ca->provider_meter_id = UINT32_MAX;
+        return true;
+    }
+
+    const struct meter *meter = ofproto_get_meter(ofproto, ca->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. */
+        ca->provider_meter_id = meter->provider_meter_id.uint32;
+        return true;
+    }
+
+    /* Prevent the meter from being set by the ofproto provider. */
+    ca->provider_meter_id = UINT32_MAX;
+    return false;
+}
+
 /* Finds the meter invoked by 'rule''s actions and adds 'rule' to the meter's
  * list of rules. */
 static void
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 5f26c4cca34a..f7dbd250ef37 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -859,6 +859,10 @@  switch in an \fBNXT_RESUME\fR message, which will restart the packet's
 traversal from the point where it was interrupted.  This permits an
 OpenFlow controller to interpose on a packet midway through processing
 in Open vSwitch.
+.IP "\fBmeter_id=\fIid\fR"
+Associate packets sent to the controller with meter \fIid\fR.  By
+default, packets sent to the controller are associated with the
+"controller" virtual meter, if one was configured.
 .
 .RE
 .IP