Re: [ovs-dev] [action upcall meters 3/5] ofproto: Support action upcall meters

[Andy Zhou]

On Thu, Apr 27, 2017 at 3:33 PM, Jarno Rajahalme  wrote:
> With small nits below:
>
> Acked-by: Jarno Rajahalme 
>
>> On Apr 14, 2017, at 12:46 PM, Andy Zhou  wrote:
>>
>> Allow action upcall meters, i.e. slowpath and controller meters,
>> to be added and displayed.
>>
>> Keep track of datapath meter ID of those action upcall meters in
>> ofproto to aid action translation. Later patches will make use of them.
>>
>> Signed-off-by: Andy Zhou 
>> ---
>> lib/ofp-print.c| 33 ++---
>> ofproto/ofproto-provider.h |  4 
>> ofproto/ofproto.c  | 52 
>> ++
>> 3 files changed, 82 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
>> index a8cdfcbf20b1..140af05950b7 100644
>> --- a/lib/ofp-print.c
>> +++ b/lib/ofp-print.c
>> @@ -1333,11 +1333,36 @@ ofp_print_meter_band(struct ds *s, uint16_t flags,
>> }
>>
>> static void
>> +ofp_print_meter_id(struct ds *s, uint32_t meter_id, char seperator)
>> +{
>> +if (meter_id <= OFPM13_MAX) {
>> +ds_put_format(s, "meter%c%"PRIu32, seperator, meter_id);
>> +} else {
>> +const char *name;
>> +switch (meter_id) {
>> +case OFPM13_SLOWPATH:
>> +name = "slowpath";
>> +break;
>> +case OFPM13_CONTROLLER:
>> +name = "controller";
>> +break;
>> +case OFPM13_ALL:
>> +name = "ALL”;
>
> We require lower case “all” when parsing, so better print that way also.
Make sense. Will change.
>
>> +break;
>> +default:
>> +name = "unknown";
>> +}
>> +ds_put_format(s, "meter%c%s", seperator, name);
>> +}
>> +}
>> +
>> +static void
>> ofp_print_meter_stats(struct ds *s, const struct ofputil_meter_stats *ms)
>> {
>> uint16_t i;
>>
>> -ds_put_format(s, "meter:%"PRIu32" ", ms->meter_id);
>> +ofp_print_meter_id(s, ms->meter_id, ':');
>> +ds_put_char(s, ' ');
>> ds_put_format(s, "flow_count:%"PRIu32" ", ms->flow_count);
>> ds_put_format(s, "packet_in_count:%"PRIu64" ", ms->packet_in_count);
>> ds_put_format(s, "byte_in_count:%"PRIu64" ", ms->byte_in_count);
>> @@ -1358,7 +1383,8 @@ ofp_print_meter_config(struct ds *s, const struct 
>> ofputil_meter_config *mc)
>> {
>> uint16_t i;
>>
>> -ds_put_format(s, "meter=%"PRIu32" ", mc->meter_id);
>> +ofp_print_meter_id(s, mc->meter_id, '=');
>> +ds_put_char(s, ' ');
>>
>> ofp_print_meter_flags(s, mc->flags);
>>
>> @@ -1412,8 +1438,9 @@ ofp_print_meter_stats_request(struct ds *s, const 
>> struct ofp_header *oh)
>> uint32_t meter_id;
>>
>> ofputil_decode_meter_request(oh, _id);
>> +ds_put_char(s, ' ');
>>
>> -ds_put_format(s, " meter=%"PRIu32, meter_id);
>> +ofp_print_meter_id(s, meter_id, '=');
>> }
>>
>> static const char *
>> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
>> index 000326d7f79d..688a9e5d32eb 100644
>> --- a/ofproto/ofproto-provider.h
>> +++ b/ofproto/ofproto-provider.h
>> @@ -112,6 +112,10 @@ struct ofproto {
>> /* Meter table.  */
>> struct ofputil_meter_features meter_features;
>> struct hmap meters; /* uint32_t indexed 'struct meter *'.  */
>> +uint32_t slowpath_meter_id; /* Datapath slowpath meter.  UINT32_MAX
>> +   if not defined.  */
>> +uint32_t controller_meter_id;   /* Datapath controller meter. UINT32_MAX
>> +   if not defined.  */
>>
>> /* OpenFlow connections. */
>> struct connmgr *connmgr;
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 8c4c7e67f213..abbb849a384b 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -568,6 +568,8 @@ ofproto_create(const char *datapath_name, const char 
>> *datapath_type,
>> memset(>meter_features, 0, sizeof ofproto->meter_features);
>> }
>> hmap_init(>meters);
>> +ofproto->slowpath_meter_id = UINT32_MAX;
>> +ofproto->controller_meter_id = UINT32_MAX;
>>
>> /* Set the initial tables version. */
>> ofproto_bump_tables_version(ofproto);
>> @@ -6232,9 +6234,33 @@ ofproto_get_meter(const struct ofproto *ofproto, 
>> uint32_t meter_id)
>> return NULL;
>> }
>>
>> +static uint32_t *
>> +ofproto_upcall_meter_ptr(struct ofproto *ofproto, uint32_t meter_id)
>> +{
>> +switch(meter_id) {
>> +case OFPM13_SLOWPATH:
>> +return >slowpath_meter_id;
>> +break;
>> +case OFPM13_CONTROLLER:
>> +return >controller_meter_id;
>> +break;
>> +case OFPM13_ALL:
>> +OVS_NOT_REACHED();
>> +default:
>> +return NULL;
>> +}
>> +}
>> +
>> static void
>> ofproto_add_meter(struct ofproto *ofproto, struct meter *meter)
>> {
>> +uint32_t *upcall_meter_ptr = ofproto_upcall_meter_ptr(ofproto, 
>> meter->id);
>> +
>> +/* Cache datapath meter IDs of special 

Re: [ovs-dev] [action upcall meters 3/5] ofproto: Support action upcall meters

[Jarno Rajahalme]

With small nits below:

Acked-by: Jarno Rajahalme 

> On Apr 14, 2017, at 12:46 PM, Andy Zhou  wrote:
> 
> Allow action upcall meters, i.e. slowpath and controller meters,
> to be added and displayed.
> 
> Keep track of datapath meter ID of those action upcall meters in
> ofproto to aid action translation. Later patches will make use of them.
> 
> Signed-off-by: Andy Zhou 
> ---
> lib/ofp-print.c| 33 ++---
> ofproto/ofproto-provider.h |  4 
> ofproto/ofproto.c  | 52 ++
> 3 files changed, 82 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index a8cdfcbf20b1..140af05950b7 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -1333,11 +1333,36 @@ ofp_print_meter_band(struct ds *s, uint16_t flags,
> }
> 
> static void
> +ofp_print_meter_id(struct ds *s, uint32_t meter_id, char seperator)
> +{
> +if (meter_id <= OFPM13_MAX) {
> +ds_put_format(s, "meter%c%"PRIu32, seperator, meter_id);
> +} else {
> +const char *name;
> +switch (meter_id) {
> +case OFPM13_SLOWPATH:
> +name = "slowpath";
> +break;
> +case OFPM13_CONTROLLER:
> +name = "controller";
> +break;
> +case OFPM13_ALL:
> +name = "ALL”;

We require lower case “all” when parsing, so better print that way also.

> +break;
> +default:
> +name = "unknown";
> +}
> +ds_put_format(s, "meter%c%s", seperator, name);
> +}
> +}
> +
> +static void
> ofp_print_meter_stats(struct ds *s, const struct ofputil_meter_stats *ms)
> {
> uint16_t i;
> 
> -ds_put_format(s, "meter:%"PRIu32" ", ms->meter_id);
> +ofp_print_meter_id(s, ms->meter_id, ':');
> +ds_put_char(s, ' ');
> ds_put_format(s, "flow_count:%"PRIu32" ", ms->flow_count);
> ds_put_format(s, "packet_in_count:%"PRIu64" ", ms->packet_in_count);
> ds_put_format(s, "byte_in_count:%"PRIu64" ", ms->byte_in_count);
> @@ -1358,7 +1383,8 @@ ofp_print_meter_config(struct ds *s, const struct 
> ofputil_meter_config *mc)
> {
> uint16_t i;
> 
> -ds_put_format(s, "meter=%"PRIu32" ", mc->meter_id);
> +ofp_print_meter_id(s, mc->meter_id, '=');
> +ds_put_char(s, ' ');
> 
> ofp_print_meter_flags(s, mc->flags);
> 
> @@ -1412,8 +1438,9 @@ ofp_print_meter_stats_request(struct ds *s, const 
> struct ofp_header *oh)
> uint32_t meter_id;
> 
> ofputil_decode_meter_request(oh, _id);
> +ds_put_char(s, ' ');
> 
> -ds_put_format(s, " meter=%"PRIu32, meter_id);
> +ofp_print_meter_id(s, meter_id, '=');
> }
> 
> static const char *
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 000326d7f79d..688a9e5d32eb 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -112,6 +112,10 @@ struct ofproto {
> /* Meter table.  */
> struct ofputil_meter_features meter_features;
> struct hmap meters; /* uint32_t indexed 'struct meter *'.  */
> +uint32_t slowpath_meter_id; /* Datapath slowpath meter.  UINT32_MAX
> +   if not defined.  */
> +uint32_t controller_meter_id;   /* Datapath controller meter. UINT32_MAX
> +   if not defined.  */
> 
> /* OpenFlow connections. */
> struct connmgr *connmgr;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 8c4c7e67f213..abbb849a384b 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -568,6 +568,8 @@ ofproto_create(const char *datapath_name, const char 
> *datapath_type,
> memset(>meter_features, 0, sizeof ofproto->meter_features);
> }
> hmap_init(>meters);
> +ofproto->slowpath_meter_id = UINT32_MAX;
> +ofproto->controller_meter_id = UINT32_MAX;
> 
> /* Set the initial tables version. */
> ofproto_bump_tables_version(ofproto);
> @@ -6232,9 +6234,33 @@ ofproto_get_meter(const struct ofproto *ofproto, 
> uint32_t meter_id)
> return NULL;
> }
> 
> +static uint32_t *
> +ofproto_upcall_meter_ptr(struct ofproto *ofproto, uint32_t meter_id)
> +{
> +switch(meter_id) {
> +case OFPM13_SLOWPATH:
> +return >slowpath_meter_id;
> +break;
> +case OFPM13_CONTROLLER:
> +return >controller_meter_id;
> +break;
> +case OFPM13_ALL:
> +OVS_NOT_REACHED();
> +default:
> +return NULL;
> +}
> +}
> +
> static void
> ofproto_add_meter(struct ofproto *ofproto, struct meter *meter)
> {
> +uint32_t *upcall_meter_ptr = ofproto_upcall_meter_ptr(ofproto, 
> meter->id);
> +
> +/* Cache datapath meter IDs of special meters. */
> +if (upcall_meter_ptr) {
> +*upcall_meter_ptr = meter->provider_meter_id.uint32;
> +}
> +
> hmap_insert(>meters, >node, hash_int(meter->id, 0));
> }
> 
> @@ -6248,7 +6274,7 @@ static bool
> 

[ovs-dev] [action upcall meters 3/5] ofproto: Support action upcall meters

[Andy Zhou]

Allow action upcall meters, i.e. slowpath and controller meters,
to be added and displayed.

Keep track of datapath meter ID of those action upcall meters in
ofproto to aid action translation. Later patches will make use of them.

Signed-off-by: Andy Zhou 
---
 lib/ofp-print.c| 33 ++---
 ofproto/ofproto-provider.h |  4 
 ofproto/ofproto.c  | 52 ++
 3 files changed, 82 insertions(+), 7 deletions(-)

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index a8cdfcbf20b1..140af05950b7 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -1333,11 +1333,36 @@ ofp_print_meter_band(struct ds *s, uint16_t flags,
 }
 
 static void
+ofp_print_meter_id(struct ds *s, uint32_t meter_id, char seperator)
+{
+if (meter_id <= OFPM13_MAX) {
+ds_put_format(s, "meter%c%"PRIu32, seperator, meter_id);
+} else {
+const char *name;
+switch (meter_id) {
+case OFPM13_SLOWPATH:
+name = "slowpath";
+break;
+case OFPM13_CONTROLLER:
+name = "controller";
+break;
+case OFPM13_ALL:
+name = "ALL";
+break;
+default:
+name = "unknown";
+}
+ds_put_format(s, "meter%c%s", seperator, name);
+}
+}
+
+static void
 ofp_print_meter_stats(struct ds *s, const struct ofputil_meter_stats *ms)
 {
 uint16_t i;
 
-ds_put_format(s, "meter:%"PRIu32" ", ms->meter_id);
+ofp_print_meter_id(s, ms->meter_id, ':');
+ds_put_char(s, ' ');
 ds_put_format(s, "flow_count:%"PRIu32" ", ms->flow_count);
 ds_put_format(s, "packet_in_count:%"PRIu64" ", ms->packet_in_count);
 ds_put_format(s, "byte_in_count:%"PRIu64" ", ms->byte_in_count);
@@ -1358,7 +1383,8 @@ ofp_print_meter_config(struct ds *s, const struct 
ofputil_meter_config *mc)
 {
 uint16_t i;
 
-ds_put_format(s, "meter=%"PRIu32" ", mc->meter_id);
+ofp_print_meter_id(s, mc->meter_id, '=');
+ds_put_char(s, ' ');
 
 ofp_print_meter_flags(s, mc->flags);
 
@@ -1412,8 +1438,9 @@ ofp_print_meter_stats_request(struct ds *s, const struct 
ofp_header *oh)
 uint32_t meter_id;
 
 ofputil_decode_meter_request(oh, _id);
+ds_put_char(s, ' ');
 
-ds_put_format(s, " meter=%"PRIu32, meter_id);
+ofp_print_meter_id(s, meter_id, '=');
 }
 
 static const char *
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 000326d7f79d..688a9e5d32eb 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -112,6 +112,10 @@ struct ofproto {
 /* Meter table.  */
 struct ofputil_meter_features meter_features;
 struct hmap meters; /* uint32_t indexed 'struct meter *'.  */
+uint32_t slowpath_meter_id; /* Datapath slowpath meter.  UINT32_MAX
+   if not defined.  */
+uint32_t controller_meter_id;   /* Datapath controller meter. UINT32_MAX
+   if not defined.  */
 
 /* OpenFlow connections. */
 struct connmgr *connmgr;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 8c4c7e67f213..abbb849a384b 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -568,6 +568,8 @@ ofproto_create(const char *datapath_name, const char 
*datapath_type,
 memset(>meter_features, 0, sizeof ofproto->meter_features);
 }
 hmap_init(>meters);
+ofproto->slowpath_meter_id = UINT32_MAX;
+ofproto->controller_meter_id = UINT32_MAX;
 
 /* Set the initial tables version. */
 ofproto_bump_tables_version(ofproto);
@@ -6232,9 +6234,33 @@ ofproto_get_meter(const struct ofproto *ofproto, 
uint32_t meter_id)
 return NULL;
 }
 
+static uint32_t *
+ofproto_upcall_meter_ptr(struct ofproto *ofproto, uint32_t meter_id)
+{
+switch(meter_id) {
+case OFPM13_SLOWPATH:
+return >slowpath_meter_id;
+break;
+case OFPM13_CONTROLLER:
+return >controller_meter_id;
+break;
+case OFPM13_ALL:
+OVS_NOT_REACHED();
+default:
+return NULL;
+}
+}
+
 static void
 ofproto_add_meter(struct ofproto *ofproto, struct meter *meter)
 {
+uint32_t *upcall_meter_ptr = ofproto_upcall_meter_ptr(ofproto, meter->id);
+
+/* Cache datapath meter IDs of special meters. */
+if (upcall_meter_ptr) {
+*upcall_meter_ptr = meter->provider_meter_id.uint32;
+}
+
 hmap_insert(>meters, >node, hash_int(meter->id, 0));
 }
 
@@ -6248,7 +6274,7 @@ static bool
 ofproto_fix_meter_action(const struct ofproto *ofproto,
  struct ofpact_meter *ma)
 {
-if (ma->meter_id && ma->meter_id <= ofproto->meter_features.max_meters) {
+if (ma->meter_id) {
 const struct meter *meter = ofproto_get_meter(ofproto, ma->meter_id);
 
 if (meter && meter->provider_meter_id.uint32 != UINT32_MAX) {
@@ -6306,6 +6332,12 @@ static void
 meter_destroy(struct ofproto *ofproto, struct meter *meter)