Re: [ovs-dev] [PATCH 2/3] ofproto: Store meters into imap

[Andy Zhou]

On Fri, Mar 31, 2017 at 7:53 PM, Ben Pfaff  wrote:
> On Fri, Mar 31, 2017 at 01:42:02PM -0700, Andy Zhou wrote:
>> Currently, meters are stored in a fixed pointer array. It is not
>> very efficient since the controller, at least in theory, can
>> pick any meter id (up to the limits to uint32_t), not necessarily
>> within the lower end of a region, or in close range to each other.
>> In particular, OFPM_SLOWPATH and OFPM_CONTROLLER meters are specified
>> at the high region.
>>
>> Switching to use imap, so that ofproto layer does not restrict
>> the number of meters that controller can add, nor does it care
>> about the value of meter_id. Only datapth actually limits the
>> number of meters ofproto layer can support.
>>
>> Signed-off-by: Andy Zhou 
>
> Instead of using a new integer-to-pointer map data structure, did you
> consider adding an hmap_node to struct meter?  That's the first approach
> that comes to mind, so I'm curious to know whether it's a poor approach
> for some reason.
I just thought 'imap' will be more general. Then I coded the way you suggested,
and it end up very similar code wise.  I will drop 'imap' in future
versions. Thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/3] ofproto: Store meters into imap

[Ben Pfaff]

On Fri, Mar 31, 2017 at 01:42:02PM -0700, Andy Zhou wrote:
> Currently, meters are stored in a fixed pointer array. It is not
> very efficient since the controller, at least in theory, can
> pick any meter id (up to the limits to uint32_t), not necessarily
> within the lower end of a region, or in close range to each other.
> In particular, OFPM_SLOWPATH and OFPM_CONTROLLER meters are specified
> at the high region.
> 
> Switching to use imap, so that ofproto layer does not restrict
> the number of meters that controller can add, nor does it care
> about the value of meter_id. Only datapth actually limits the
> number of meters ofproto layer can support.
> 
> Signed-off-by: Andy Zhou 

Instead of using a new integer-to-pointer map data structure, did you
consider adding an hmap_node to struct meter?  That's the first approach
that comes to mind, so I'm curious to know whether it's a poor approach
for some reason.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 2/3] ofproto: Store meters into imap

[Andy Zhou]

Currently, meters are stored in a fixed pointer array. It is not
very efficient since the controller, at least in theory, can
pick any meter id (up to the limits to uint32_t), not necessarily
within the lower end of a region, or in close range to each other.
In particular, OFPM_SLOWPATH and OFPM_CONTROLLER meters are specified
at the high region.

Switching to use imap, so that ofproto layer does not restrict
the number of meters that controller can add, nor does it care
about the value of meter_id. Only datapth actually limits the
number of meters ofproto layer can support.

Signed-off-by: Andy Zhou 
---
 ofproto/ofproto-provider.h |   8 ++-
 ofproto/ofproto.c  | 125 +++--
 2 files changed, 79 insertions(+), 54 deletions(-)

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index b7b12cdfd5f4..ccd574284f5d 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -38,6 +38,7 @@
 #include "guarded-list.h"
 #include "heap.h"
 #include "hindex.h"
+#include "imap.h"
 #include "object-collection.h"
 #include "ofproto/ofproto.h"
 #include "openvswitch/list.h"
@@ -109,12 +110,9 @@ struct ofproto {
 /* List of expirable flows, in all flow tables. */
 struct ovs_list expirable OVS_GUARDED_BY(ofproto_mutex);
 
-/* Meter table.
- * OpenFlow meters start at 1.  To avoid confusion we leave the first
- * pointer in the array un-used, and index directly with the OpenFlow
- * meter_id. */
+/* Meter table.  */
 struct ofputil_meter_features meter_features;
-struct meter **meters; /* 'meter_features.max_meter' + 1 pointers. */
+struct imap meters; /* uint32_t indexed 'struct meter *'.  */
 
 /* OpenFlow connections. */
 struct connmgr *connmgr;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 84ea95b0c2a2..e6d605549b07 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -281,7 +281,8 @@ static uint64_t pick_fallback_dpid(void);
 static void ofproto_destroy__(struct ofproto *);
 static void update_mtu(struct ofproto *, struct ofport *);
 static void update_mtu_ofproto(struct ofproto *);
-static void meter_delete(struct ofproto *, uint32_t first, uint32_t last);
+static void meter_delete(struct ofproto *, uint32_t);
+static void meter_delete_all(struct ofproto *);
 static void meter_insert_rule(struct rule *);
 
 /* unixctl. */
@@ -566,8 +567,7 @@ ofproto_create(const char *datapath_name, const char 
*datapath_type,
 } else {
 memset(>meter_features, 0, sizeof ofproto->meter_features);
 }
-ofproto->meters = xzalloc((ofproto->meter_features.max_meters + 1)
-  * sizeof(struct meter *));
+imap_init(>meters);
 
 /* Set the initial tables version. */
 ofproto_bump_tables_version(ofproto);
@@ -1635,11 +1635,9 @@ ofproto_destroy(struct ofproto *p, bool del)
 return;
 }
 
-if (p->meters) {
-meter_delete(p, 1, p->meter_features.max_meters);
-p->meter_features.max_meters = 0;
-free(p->meters);
-p->meters = NULL;
+if (!imap_is_empty(>meters)) {
+meter_delete_all(p);
+imap_destroy(>meters);
 }
 
 ofproto_flush__(p);
@@ -6215,6 +6213,19 @@ struct meter {
 struct ofputil_meter_band *bands;
 };
 
+static struct meter *
+ofproto_get_meter(const struct ofproto *ofproto, uint32_t index)
+{
+return imap_get(>meters, index);
+}
+
+static void
+ofproto_add_meter(struct ofproto *ofproto, uint32_t index,
+  struct meter *meter)
+{
+imap_add(>meters, index, meter);
+}
+
 /*
  * This is used in instruction validation at flow set-up time, to map
  * the OpenFlow meter ID to the corresponding datapath provider meter
@@ -6226,7 +6237,7 @@ ofproto_fix_meter_action(const struct ofproto *ofproto,
  struct ofpact_meter *ma)
 {
 if (ma->meter_id && ma->meter_id <= ofproto->meter_features.max_meters) {
-const struct meter *meter = ofproto->meters[ma->meter_id];
+const struct meter *meter = ofproto_get_meter(ofproto, ma->meter_id);
 
 if (meter && meter->provider_meter_id.uint32 != UINT32_MAX) {
 /* Update the action with the provider's meter ID, so that we
@@ -6246,7 +6257,7 @@ meter_insert_rule(struct rule *rule)
 {
 const struct rule_actions *a = rule_get_actions(rule);
 uint32_t meter_id = ofpacts_get_meter(a->ofpacts, a->ofpacts_len);
-struct meter *meter = rule->ofproto->meters[meter_id];
+struct meter *meter = ofproto_get_meter(rule->ofproto, meter_id);
 
 ovs_list_insert(>rules, >meter_list_node);
 }
@@ -6279,31 +6290,50 @@ meter_create(const struct ofputil_meter_config *config,
 }
 
 static void
-meter_delete(struct ofproto *ofproto, uint32_t first, uint32_t last)
+meter_destroy(struct ofproto *ofproto, struct meter *meter)
 OVS_REQUIRES(ofproto_mutex)
 {
-for (uint32_t mid = first; mid <= last; ++mid) {
-struct meter