[ovs-dev] [PATCH 1/3] ovn: ovn-controller changes for qos settings

Ben Pfaff blp at ovn.org
Tue Feb 9 04:03:26 UTC 2016


That sounds great.  I like simple.

On Tue, Feb 09, 2016 at 08:19:57AM +0530, Babu Shanmugam wrote:
> Ben, I tested your solution and it works well. It is in fact simpler than my
> approach. I will squash the other two commits along with this for v2.
> 
> 
> Thank you,
> Babu
> 
> On Tuesday 09 February 2016 05:44 AM, Ben Pfaff wrote:
> >On Mon, Feb 08, 2016 at 03:20:11PM +0530, Babu Shanmugam wrote:
> >>>>The qos settings are managed using the 'options' fields in the
> >>>>"Port_Binding" table.
> >>>>
> >>>>Signed-off-by: Babu Shanmugam <bschanmu at redhat.com>
> >>>This seems more complicated than necessary.  In the
> >>>SBREC_PORT_BINDING_FOR_EACH loop in binding_run(), I think that we only
> >>>need to be able to find the ovsrec_interface associated with the
> >>>binding_rec.  Then we can check and possibly update the
> >>>ovsrec_interface's ingress_policing_rate and ingress_policing_burst.
> >>>Can you explain why there's this more complicated data structure?
> >>If not for this iface_qos hash map, we need to run a loop OVSREC
> >>Interface_table entries for every iteration of SBREC Port_Binding entry to
> >>check against its policing_values and update it if needed.
> >>If we have an api to fetch the Interface_table entry with the interface id,
> >>this hash map wouldn't be needed. I was not able find such an api, but is
> >>there any such api?
> >>
> >>Below is a summary on the use of this hash map;
> >>
> >>  * At the start of binding_run(), policing values of all the interfaces
> >>    are hashed inside this map with the interface_id as the key.
> >>  * On each iteration of SBREC Port_Binding, this hash map is looked up
> >>    and policing values of the SBREC port_binding and OVSREC
> >>    Interface_table are compared and one of the following actions are taken
> >>      o If both the values are equal, then the hash map entry
> >>        corresponding to this interface id is removed
> >>      o If there is a difference, the difference is noted in the hash
> >>        map entry
> >>  * After looping SBREC_Port_Binding, the OVSREC Inteface_table is
> >>    looped for all the entries, and if the interface_id is present in
> >>    the hash map, its policing values are updated.
> >Here's what I had in mind.  It's untested.
> >
> >--8<--------------------------cut here-------------------------->8--
> >
> >From: Ben Pfaff <blp at ovn.org>
> >Date: Mon, 8 Feb 2016 16:14:20 -0800
> >Subject: [PATCH] ovn-controller: Copy southbound database policing settings
> >  into OVS config.
> >
> >Signed-off-by: Ben Pfaff <blp at ovn.org>
> >---
> >  ovn/controller/binding.c        | 46 ++++++++++++++++++++++++++++-------------
> >  ovn/controller/ovn-controller.c |  4 ++++
> >  2 files changed, 36 insertions(+), 14 deletions(-)
> >
> >diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> >index ce9cccf..f295473 100644
> >--- a/ovn/controller/binding.c
> >+++ b/ovn/controller/binding.c
> >@@ -1,4 +1,4 @@
> >-/* Copyright (c) 2015 Nicira, Inc.
> >+/* Copyright (c) 2015, 2016 Nicira, Inc.
> >   *
> >   * Licensed under the Apache License, Version 2.0 (the "License");
> >   * you may not use this file except in compliance with the License.
> >@@ -47,7 +47,7 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> >  }
> >  static void
> >-get_local_iface_ids(const struct ovsrec_bridge *br_int, struct sset *lports)
> >+get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lports)
> >  {
> >      int i;
> >@@ -68,7 +68,7 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int, struct sset *lports)
> >              if (!iface_id) {
> >                  continue;
> >              }
> >-            sset_add(lports, iface_id);
> >+            shash_add(lports, iface_id, iface_rec);
> >          }
> >      }
> >  }
> >@@ -132,6 +132,17 @@ add_local_datapath(struct hmap *local_datapaths,
> >      }
> >  }
> >+static void
> >+update_qos(const struct ovsrec_interface *iface_rec,
> >+           const struct sbrec_port_binding *pb)
> >+{
> >+    int rate = smap_get_int(&pb->options, "policing_rate", 0);
> >+    int burst = smap_get_int(&pb->options, "policing_burst", 0);
> >+
> >+    ovsrec_interface_set_ingress_policing_rate(iface_rec, MAX(0, rate));
> >+    ovsrec_interface_set_ingress_policing_burst(iface_rec, MAX(0, burst));
> >+}
> >+
> >  void
> >  binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
> >              const char *chassis_id, struct simap *ct_zones,
> >@@ -139,8 +150,6 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
> >  {
> >      const struct sbrec_chassis *chassis_rec;
> >      const struct sbrec_port_binding *binding_rec;
> >-    struct sset lports, all_lports;
> >-    const char *name;
> >      if (!ctx->ovnsb_idl_txn) {
> >          return;
> >@@ -151,15 +160,19 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
> >          return;
> >      }
> >-    sset_init(&lports);
> >-    sset_init(&all_lports);
> >+    struct shash lports = SHASH_INITIALIZER(&lports);
> >      if (br_int) {
> >          get_local_iface_ids(br_int, &lports);
> >      } else {
> >          /* We have no integration bridge, therefore no local logical ports.
> >           * We'll remove our chassis from all port binding records below. */
> >      }
> >-    sset_clone(&all_lports, &lports);
> >+
> >+    struct sset all_lports = SSET_INITIALIZER(&all_lports);
> >+    struct shash_node *node;
> >+    SHASH_FOR_EACH (node, &lports) {
> >+        sset_add(&all_lports, node->name);
> >+    }
> >      ovsdb_idl_txn_add_comment(
> >          ctx->ovnsb_idl_txn,"ovn-controller: updating port bindings for '%s'",
> >@@ -169,14 +182,19 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
> >       * chassis and update the binding accordingly.  This includes both
> >       * directly connected logical ports and children of those ports. */
> >      SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
> >-        if (sset_find_and_delete(&lports, binding_rec->logical_port) ||
> >-                (binding_rec->parent_port && binding_rec->parent_port[0] &&
> >-                 sset_contains(&all_lports, binding_rec->parent_port))) {
> >+        const struct ovsrec_interface *iface_rec
> >+            = shash_find_and_delete(&lports, binding_rec->logical_port);
> >+        if (iface_rec
> >+            || (binding_rec->parent_port && binding_rec->parent_port[0] &&
> >+                sset_contains(&all_lports, binding_rec->parent_port))) {
> >              if (binding_rec->parent_port && binding_rec->parent_port[0]) {
> >                  /* Add child logical port to the set of all local ports. */
> >                  sset_add(&all_lports, binding_rec->logical_port);
> >              }
> >              add_local_datapath(local_datapaths, binding_rec);
> >+            if (iface_rec) {
> >+                update_qos(iface_rec, binding_rec);
> >+            }
> >              if (binding_rec->chassis == chassis_rec) {
> >                  continue;
> >              }
> >@@ -199,13 +217,13 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
> >          }
> >      }
> >-    SSET_FOR_EACH (name, &lports) {
> >-        VLOG_DBG("No port binding record for lport %s", name);
> >+    SHASH_FOR_EACH (node, &lports) {
> >+        VLOG_DBG("No port binding record for lport %s", node->name);
> >      }
> >      update_ct_zones(&all_lports, ct_zones, ct_zone_bitmap);
> >-    sset_destroy(&lports);
> >+    shash_destroy(&lports);
> >      sset_destroy(&all_lports);
> >  }
> >diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
> >index 3638342..ac824ad 100644
> >--- a/ovn/controller/ovn-controller.c
> >+++ b/ovn/controller/ovn-controller.c
> >@@ -240,6 +240,10 @@ main(int argc, char *argv[])
> >      ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_interface_col_name);
> >      ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_interface_col_type);
> >      ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_interface_col_options);
> >+    ovsdb_idl_add_column(ovs_idl_loop.idl,
> >+                         &ovsrec_interface_col_ingress_policing_rate);
> >+    ovsdb_idl_add_column(ovs_idl_loop.idl,
> >+                         &ovsrec_interface_col_ingress_policing_burst);
> >      ovsdb_idl_add_table(ovs_idl_loop.idl, &ovsrec_table_port);
> >      ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_port_col_name);
> >      ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_port_col_interfaces);
> 



More information about the dev mailing list