[ovs-dev,ACL,Meters,5/7] ovn: Support configuring meters through SB Meter table.
diff mbox series

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

Commit Message

Justin Pettit July 30, 2018, 6:46 a.m. UTC
Add the ability to configure meters through the newly introduced Meter
table in the Southbound database.  Previously, meters were configured by
providing strings to describe the meter in the extended meter table.
This patch changes the behavior so that the extended meter table's
strings are references to names in the Meter table.  The old behavior is
still supported if the extended meter table entry begins with "__string: "

Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
 ovn/controller/ofctrl.c         | 124 +++++++++++++++++++++++---------
 ovn/controller/ofctrl.h         |   3 +-
 ovn/controller/ovn-controller.c |   1 +
 ovn/lib/actions.c               |  11 +--
 4 files changed, 100 insertions(+), 39 deletions(-)

Comments

Ben Pfaff July 30, 2018, 6:44 p.m. UTC | #1
On Sun, Jul 29, 2018 at 11:46:36PM -0700, Justin Pettit wrote:
> Add the ability to configure meters through the newly introduced Meter
> table in the Southbound database.  Previously, meters were configured by
> providing strings to describe the meter in the extended meter table.
> This patch changes the behavior so that the extended meter table's
> strings are references to names in the Meter table.  The old behavior is
> still supported if the extended meter table entry begins with "__string: "
> 
> Signed-off-by: Justin Pettit <jpettit@ovn.org>

I have the following suggestions.  Neither is important.

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 01981a79480b..d810d413f207 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -907,7 +907,7 @@ ofctrl_can_put(void)
  *
  * Replaces the group table and meter table on the switch, if possible,
  * by the contents of '->desired'.  Regardless of whether the table is
- * updated, this deletes all the groups ore metersjfrom the '->desired'
+ * updated, this deletes all the groups ore meters from the '->desired'
  * and frees them. (The hmap itself isn't destroyed.)
  *
  * Sends conntrack flush messages to each zone in 'pending_ct_zones' that
@@ -1080,10 +1080,10 @@ ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
     struct ovn_extend_table_info *m_installed, *next_meter;
     EXTEND_TABLE_FOR_EACH_INSTALLED (m_installed, next_meter, meters) {
         /* Delete the meter. */
-        struct ofputil_meter_mod mm;
-        memset(&mm, 0, sizeof mm);
-        mm.command = OFPMC13_DELETE;
-        mm.meter.meter_id = m_installed->table_id;
+        struct ofputil_meter_mod mm = {
+            .command = OFPMC13_DELETE,
+            .meter.meter_id = m_installed->table_id,
+        };
         add_meter_mod(&mm, &msgs);
 
         ovn_extend_table_remove(meters, m_installed);

Acked-by: Ben Pfaff <blp@ovn.org>
Justin Pettit July 30, 2018, 10:05 p.m. UTC | #2
> On Jul 30, 2018, at 11:44 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Sun, Jul 29, 2018 at 11:46:36PM -0700, Justin Pettit wrote:
>> Add the ability to configure meters through the newly introduced Meter
>> table in the Southbound database.  Previously, meters were configured by
>> providing strings to describe the meter in the extended meter table.
>> This patch changes the behavior so that the extended meter table's
>> strings are references to names in the Meter table.  The old behavior is
>> still supported if the extended meter table entry begins with "__string: "
>> 
>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> 
> I have the following suggestions.  Neither is important.
> 
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index 01981a79480b..d810d413f207 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -907,7 +907,7 @@ ofctrl_can_put(void)
>  *
>  * Replaces the group table and meter table on the switch, if possible,
>  * by the contents of '->desired'.  Regardless of whether the table is
> - * updated, this deletes all the groups ore metersjfrom the '->desired'
> + * updated, this deletes all the groups ore meters from the '->desired'
>  * and frees them. (The hmap itself isn't destroyed.)
>  *
>  * Sends conntrack flush messages to each zone in 'pending_ct_zones' that
> @@ -1080,10 +1080,10 @@ ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
>     struct ovn_extend_table_info *m_installed, *next_meter;
>     EXTEND_TABLE_FOR_EACH_INSTALLED (m_installed, next_meter, meters) {
>         /* Delete the meter. */
> -        struct ofputil_meter_mod mm;
> -        memset(&mm, 0, sizeof mm);
> -        mm.command = OFPMC13_DELETE;
> -        mm.meter.meter_id = m_installed->table_id;
> +        struct ofputil_meter_mod mm = {
> +            .command = OFPMC13_DELETE,
> +            .meter.meter_id = m_installed->table_id,
> +        };
>         add_meter_mod(&mm, &msgs);
> 
>         ovn_extend_table_remove(meters, m_installed);

That is better.  Thanks.

--Justin

Patch
diff mbox series

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index c7f41612456d..01981a79480b 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -812,6 +812,81 @@  add_ct_flush_zone(uint16_t zone_id, struct ovs_list *msgs)
     ovs_list_push_back(msgs, &msg->list_node);
 }
 
+static void
+add_meter_string(struct ovn_extend_table_info *m_desired,
+                 struct ovs_list *msgs)
+{
+    /* Create and install new meter. */
+    struct ofputil_meter_mod mm;
+    enum ofputil_protocol usable_protocols;
+    char *meter_string = xasprintf("meter=%"PRIu32",%s",
+                                   m_desired->table_id,
+                                   &m_desired->name[9]);
+    char *error = parse_ofp_meter_mod_str(&mm, meter_string, OFPMC13_ADD,
+                                          &usable_protocols);
+    if (!error) {
+        add_meter_mod(&mm, msgs);
+    } else {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+        VLOG_ERR_RL(&rl, "new meter %s %s", error, meter_string);
+        free(error);
+    }
+    free(meter_string);
+}
+
+static void
+add_meter(struct ovn_extend_table_info *m_desired,
+          const struct sbrec_meter_table *meter_table,
+          struct ovs_list *msgs)
+{
+    const struct sbrec_meter *sb_meter;
+    SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) {
+        if (!strcmp(m_desired->name, sb_meter->name)) {
+            break;
+        }
+    }
+
+    if (!sb_meter) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+        VLOG_ERR_RL(&rl, "could not find meter named \"%s\"", m_desired->name);
+        return;
+    }
+
+    struct ofputil_meter_mod mm;
+    mm.command = OFPMC13_ADD;
+    mm.meter.meter_id = m_desired->table_id;
+    mm.meter.flags = OFPMF13_STATS;
+
+    if (!strcmp(sb_meter->unit, "pktps")) {
+        mm.meter.flags |= OFPMF13_PKTPS;
+    } else {
+        mm.meter.flags |= OFPMF13_KBPS;
+    }
+
+    mm.meter.n_bands = sb_meter->n_bands;
+    mm.meter.bands = xcalloc(mm.meter.n_bands, sizeof *mm.meter.bands);
+
+    for (size_t i = 0; i < sb_meter->n_bands; i++) {
+        struct sbrec_meter_band *sb_band = sb_meter->bands[i];
+        struct ofputil_meter_band *mm_band = &mm.meter.bands[i];
+
+        if (!strcmp(sb_band->action, "drop")) {
+            mm_band->type = OFPMBT13_DROP;
+        }
+
+        mm_band->prec_level = 0;
+        mm_band->rate = sb_band->rate;
+        mm_band->burst_size = sb_band->burst_size;
+
+        if (mm_band->burst_size) {
+            mm.meter.flags |= OFPMF13_BURST;
+        }
+    }
+
+    add_meter_mod(&mm, msgs);
+    free(mm.meter.bands);
+}
+
 /* The flow table can be updated if the connection to the switch is up and
  * in the correct state and not backlogged with existing flow_mods.  (Our
  * criteria for being backlogged appear very conservative, but the socket
@@ -830,10 +905,10 @@  ofctrl_can_put(void)
 /* Replaces the flow table on the switch, if possible, by the flows added
  * with ofctrl_add_flow().
  *
- * Replaces the group table and meter table on the switch, if possible, by the
- *  contents of 'groups->desired'.  Regardless of whether the group table
- * is updated, this deletes all the groups from the 'groups->desired' and frees
- * them. (The hmap itself isn't destroyed.)
+ * Replaces the group table and meter table on the switch, if possible,
+ * by the contents of '->desired'.  Regardless of whether the table is
+ * updated, this deletes all the groups ore metersjfrom the '->desired'
+ * and frees them. (The hmap itself isn't destroyed.)
  *
  * Sends conntrack flush messages to each zone in 'pending_ct_zones' that
  * is in the CT_ZONE_OF_QUEUED state and then moves the zone into the
@@ -842,7 +917,7 @@  ofctrl_can_put(void)
  * This should be called after ofctrl_run() within the main loop. */
 void
 ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
-           int64_t nb_cfg)
+           const struct sbrec_meter_table *meter_table, int64_t nb_cfg)
 {
     if (!ofctrl_can_put()) {
         ovn_flow_table_clear(flow_table);
@@ -892,22 +967,13 @@  ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
      * add them to the switch. */
     struct ovn_extend_table_info *m_desired;
     EXTEND_TABLE_FOR_EACH_UNINSTALLED (m_desired, meters) {
-        /* Create and install new meter. */
-        struct ofputil_meter_mod mm;
-        enum ofputil_protocol usable_protocols;
-        char *meter_string = xasprintf("meter=%"PRIu32",%s",
-                                       m_desired->table_id,
-                                       m_desired->name);
-        char *error = parse_ofp_meter_mod_str(&mm, meter_string, OFPMC13_ADD,
-                                              &usable_protocols);
-        if (!error) {
-            add_meter_mod(&mm, &msgs);
+        if (!strncmp(m_desired->name, "__string: ", 10)) {
+            /* The "set-meter" action creates a meter entry name that
+             * describes the meter itself. */
+            add_meter_string(m_desired, &msgs);
         } else {
-            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-            VLOG_ERR_RL(&rl, "new meter %s %s", error, meter_string);
-            free(error);
+            add_meter(m_desired, meter_table, &msgs);
         }
-        free(meter_string);
     }
 
     /* Iterate through all of the installed flows.  If any of them are no
@@ -1015,21 +1081,11 @@  ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
     EXTEND_TABLE_FOR_EACH_INSTALLED (m_installed, next_meter, meters) {
         /* Delete the meter. */
         struct ofputil_meter_mod mm;
-        enum ofputil_protocol usable_protocols;
-        char *meter_string = xasprintf("meter=%"PRIu32"",
-                                       m_installed->table_id);
-        char *error = parse_ofp_meter_mod_str(&mm, meter_string,
-                                              OFPMC13_DELETE,
-                                              &usable_protocols);
-        if (!error) {
-            add_meter_mod(&mm, &msgs);
-        } else {
-            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-            VLOG_ERR_RL(&rl,  "Error deleting meter %"PRIu32": %s",
-                        m_installed->table_id, error);
-            free(error);
-        }
-        free(meter_string);
+        memset(&mm, 0, sizeof mm);
+        mm.command = OFPMC13_DELETE;
+        mm.meter.meter_id = m_installed->table_id;
+        add_meter_mod(&mm, &msgs);
+
         ovn_extend_table_remove(meters, m_installed);
     }
 
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index 886b9bd21e9a..e3fc95b05cd3 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -27,6 +27,7 @@  struct hmap;
 struct match;
 struct ofpbuf;
 struct ovsrec_bridge;
+struct sbrec_meter_table;
 struct shash;
 
 /* Interface for OVN main loop. */
@@ -36,7 +37,7 @@  enum mf_field_id ofctrl_run(const struct ovsrec_bridge *br_int,
                             struct shash *pending_ct_zones);
 bool ofctrl_can_put(void);
 void ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
-                int64_t nb_cfg);
+                const struct sbrec_meter_table *meter_table, int64_t nb_cfg);
 void ofctrl_wait(void);
 void ofctrl_destroy(void);
 int64_t ofctrl_get_cur_cfg(void);
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 008f81d70eeb..470e02ae862e 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -768,6 +768,7 @@  main(int argc, char *argv[])
                                    time_msec());
 
                     ofctrl_put(&flow_table, &pending_ct_zones,
+                               sbrec_meter_table_get(ovnsb_idl_loop.idl),
                                get_nb_cfg(sbrec_sb_global_table_get(
                                               ovnsb_idl_loop.idl)));
 
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 430bb677bafb..26cdb4fdd482 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -2217,13 +2217,16 @@  encode_SET_METER(const struct ovnact_set_meter *cl,
     uint32_t table_id;
     struct ofpact_meter *om;
 
+    /* Use the special "__string:" prefix to indicate that the name
+     * describes the meter itself. */
     char *name;
     if (cl->burst) {
-        name = xasprintf("kbps burst stats bands=type=drop rate=%"PRId64" "
-                         "burst_size=%"PRId64"", cl->rate, cl->burst);
+        name = xasprintf("__string: kbps burst stats bands=type=drop "
+                         "rate=%"PRId64" burst_size=%"PRId64"", cl->rate,
+                         cl->burst);
     } else {
-        name = xasprintf("kbps stats bands=type=drop rate=%"PRId64"",
-                         cl->rate);
+        name = xasprintf("__string: kbps stats bands=type=drop "
+                         "rate=%"PRId64"", cl->rate);
     }
 
     table_id = ovn_extend_table_assign_id(ep->meter_table, name);