On Thu, Apr 27, 2017 at 3:33 PM, Jarno Rajahalme <ja...@ovn.org> wrote:
> With small nits below:
>
> Acked-by: Jarno Rajahalme <ja...@ovn.org>
>
>> On Apr 14, 2017, at 12:46 PM, Andy Zhou <az...@ovn.org> 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 <az...@ovn.org>
>> ---
>> 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, &meter_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(&ofproto->meter_features, 0, sizeof ofproto->meter_features);
>> }
>> hmap_init(&ofproto->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 &ofproto->slowpath_meter_id;
>> + break;
>> + case OFPM13_CONTROLLER:
>> + return &ofproto->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(&ofproto->meters, &meter->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) {
>
> This change would better be placed with patch 1/5?
Yes, It would also make sense there, I will move it.
>
>> 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)
>> OVS_REQUIRES(ofproto_mutex)
>> {
>> + uint32_t *upcall_meter_ptr;
>> + upcall_meter_ptr = ofproto_upcall_meter_ptr(ofproto, meter->id);
>> + if (upcall_meter_ptr) {
>> + *upcall_meter_ptr = UINT32_MAX;
>> + }
>> +
>> if (!ovs_list_is_empty(&meter->rules)) {
>> struct rule_collection rules;
>> struct rule *rule;
>> @@ -6438,12 +6470,24 @@ handle_meter_mod(struct ofconn *ofconn, const struct
>> ofp_header *oh)
>>
>> if (mm.command != OFPMC13_DELETE) {
>> /* Fails also when meters are not implemented by the provider. */
>> - if (meter_id == 0 || meter_id > OFPM13_MAX) {
>> + if (ofproto->meter_features.max_meters == 0) {
>> error = OFPERR_OFPMMFC_INVALID_METER;
>> goto exit_free_bands;
>> - } else if (meter_id > ofproto->meter_features.max_meters) {
>> - error = OFPERR_OFPMMFC_OUT_OF_METERS;
>> + }
>> +
>> + if (meter_id == 0) {
>> + error = OFPERR_OFPMMFC_INVALID_METER;
>> goto exit_free_bands;
>> + } else if (meter_id > OFPM13_MAX) {
>> + switch(meter_id) {
>> + case OFPM13_SLOWPATH:
>> + case OFPM13_CONTROLLER:
>> + break;
>> + case OFPM13_ALL:
>> + default:
>> + error = OFPERR_OFPMMFC_INVALID_METER;
>> + goto exit_free_bands;
>> + }
>> }
>> if (mm.meter.n_bands > ofproto->meter_features.max_bands) {
>> error = OFPERR_OFPMMFC_OUT_OF_BANDS;
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
