[ovs-dev] [PATCH] [RFC] Add OpenFlow 1.3 protocol support for meters.

Rajahalme, Jarno (NSN - FI/Espoo) jarno.rajahalme at nsn.com
Mon Jun 24 21:52:25 UTC 2013


On Jun 20, 2013, at 23:55 , ext Ben Pfaff wrote:

> From: Neil Zhu <zhuj at centecnetworks.com>
> 
> Signed-off-by: Neil Zhu <zhuj at centecnetworks.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> This is the first of a series of Open vSwitch features contributed
> by Centec Networks.  Other features will include groups, the
> "table mod" request, and some queue related features.
> 
> This feature, like the others, does not include Linux kernel
> datapath support, because Centec produces ASIC-based implementations.
> 
> This patch is not completely ready to go, but I am posting it now
> to get the ball rolling.

Earlier would have been nice, so I wouldn't have had to start from scratch :-)

Please find my comments below. After that I'd like you to read through at least the ofp-util.h and ofproto* parts of the patches I sent last week, please.

  Jarno

...
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index 45d8601..5e08235 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -1087,6 +1087,15 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
>         goto exit;
>     }
> 
> +    if (insts[OVSINST_OFPIT13_METER]) {
> +        const struct ofp13_instruction_meter *oim;
> +        struct ofpact_meter *om;
> +        oim = instruction_get_OFPIT13_METER(
> +            insts[OVSINST_OFPIT13_METER]);
> +        om = ofpact_put_METER(ofpacts);
> +        om->meter_id = ntohl(oim->meter_id);
> +    }
> +
>     if (insts[OVSINST_OFPIT11_APPLY_ACTIONS]) {
>         const union ofp_action *actions;
>         size_t n_actions;
> @@ -1231,6 +1240,9 @@ ofpact_check__(const struct ofpact *a, const struct flow *flow,
>     case OFPACT_WRITE_METADATA:
>     case OFPACT_GOTO_TABLE:
>         return 0;
> +        

Extra white-space above!

> +    case OFPACT_METER:
> +        return 0;
> 

The validity of the meter_id should be checked here. At least values larger than OFPM13_MAX are invalid for a meter instruction, as are values larger than the ones supported by the datapath (max_meter in struct ofp13_meter_features). However, the OpenFlow spec is not completely clear on this one, but as flows are expected to be deleted when a meter they use is deleted, it seems logical that you should not be able to create a flow with an invalid meter instruction (same as with groups). But there seem to be no proper error code for an invalid meter, like there is for an invalid group (OFPERR_OFPBAC_BAD_OUT_GROUP), so go figure...

Also, ofpacts_verify() needs support for METER, as the spec says that:

"In practice, the only constraints are that the Meter instruction is executed before the Apply-Actions instruction, that the Clear-Actions instruction is executed before the Write-Actions instruction, and that Goto-Table is executed last."

>     default:
>         NOT_REACHED();
> @@ -1548,6 +1560,9 @@ ofpact_to_nxast(const struct ofpact *a, struct ofpbuf *out)
>         ofpact_sample_to_nxast(ofpact_get_SAMPLE(a), out);
>         break;
> 
> +    case OFPACT_METER:
> +        NOT_REACHED();
> +

I would keep this and the other similar cases below with the other instructions (e.g., GOTO_TABLE).

>     case OFPACT_OUTPUT:
>     case OFPACT_ENQUEUE:
>     case OFPACT_SET_VLAN_VID:
> @@ -1658,6 +1673,9 @@ ofpact_to_openflow10(const struct ofpact *a, struct ofpbuf *out)
>         /* XXX */
>         break;
> 
> +    case OFPACT_METER:
> +        break;
> +
>     case OFPACT_CONTROLLER:
>     case OFPACT_OUTPUT_REG:
>     case OFPACT_BUNDLE:
> @@ -1829,6 +1847,9 @@ ofpact_to_openflow11(const struct ofpact *a, struct ofpbuf *out)
>     case OFPACT_GOTO_TABLE:
>         NOT_REACHED();
> 
> +    case OFPACT_METER:
> +        NOT_REACHED();
> +
>     case OFPACT_CONTROLLER:
>     case OFPACT_OUTPUT_REG:
>     case OFPACT_BUNDLE:
> @@ -1893,6 +1914,11 @@ ofpacts_put_openflow11_instructions(const struct ofpact ofpacts[],
> 
>         if (a->type == OFPACT_CLEAR_ACTIONS) {
>             instruction_put_OFPIT11_CLEAR_ACTIONS(openflow);
> +        } else if (a->type == OFPACT_METER) {
> +            struct ofp13_instruction_meter *oim;
> +
> +            oim = instruction_put_OFPIT13_METER(openflow);
> +            oim->meter_id = htonl(ofpact_get_METER(a)->meter_id);
>         } else if (a->type == OFPACT_GOTO_TABLE) {
>             struct ofp11_instruction_goto_table *oigt;
> 
> @@ -1976,6 +2002,7 @@ ofpact_outputs_to_port(const struct ofpact *ofpact, ofp_port_t port)
>     case OFPACT_SAMPLE:
>     case OFPACT_CLEAR_ACTIONS:
>     case OFPACT_GOTO_TABLE:
> +    case OFPACT_METER:
>     default:
>         return false;
>     }
> @@ -2302,6 +2329,13 @@ ofpact_format(const struct ofpact *a, struct ds *s)
>                           OVSINST_OFPIT11_GOTO_TABLE),
>                       ofpact_get_GOTO_TABLE(a)->table_id);
>         break;
> +        

White-space error above.

> +    case OFPACT_METER:
> +        ds_put_format(s, "%s:%"PRIu32,
> +                      ofpact_instruction_name_from_type(
> +                                          OVSINST_OFPIT13_METER),
> +                      ofpact_get_METER(a)->meter_id);
> +        break;
>     }
> }
> 
> diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
> index 4fc40b3..c880fba 100644
> --- a/lib/ofp-actions.h
> +++ b/lib/ofp-actions.h
> @@ -100,7 +100,8 @@
>     /* XXX Write-Actions */                                         \
>     DEFINE_OFPACT(WRITE_METADATA,  ofpact_metadata,      ofpact)    \
>     DEFINE_OFPACT(CLEAR_ACTIONS,   ofpact_null,          ofpact)    \
> -    DEFINE_OFPACT(GOTO_TABLE,      ofpact_goto_table,    ofpact)
> +    DEFINE_OFPACT(GOTO_TABLE,      ofpact_goto_table,    ofpact)    \
> +    DEFINE_OFPACT(METER,           ofpact_meter,         ofpact)
> 
> /* enum ofpact_type, with a member OFPACT_<ENUM> for each action. */
> enum OVS_PACKED_ENUM ofpact_type {
> @@ -481,6 +482,11 @@ struct ofpact_goto_table {
>     uint8_t table_id;
> };
> 
> +struct ofpact_meter {
> +    struct ofpact ofpact;
> +    uint32_t meter_id;
> +};
> +
> /* Converting OpenFlow to ofpacts. */
> enum ofperr ofpacts_pull_openflow10(struct ofpbuf *openflow,
>                                     unsigned int actions_len,
> @@ -601,6 +607,10 @@ void ofpact_pad(struct ofpbuf *);
>  * It is enforced on parser from text string.
>  */
> #define OVS_INSTRUCTIONS                                    \
> +    DEFINE_INST(OFPIT13_METER,                              \
> +                ofp13_instruction_meter,          true,     \

The "extensible" flag should be 'false', as the meter instruction is not extensible.

> +                "meter")                                    \
> +                                                            \
>     DEFINE_INST(OFPIT11_APPLY_ACTIONS,                      \
>                 ofp11_instruction_actions,        true,     \
>                 "apply_actions")                            \
> @@ -640,7 +650,8 @@ ofpact_is_instruction(const struct ofpact *a)
>     /* XXX Write-Actions */
>     return a->type == OFPACT_CLEAR_ACTIONS
>         || a->type == OFPACT_WRITE_METADATA
> -        || a->type == OFPACT_GOTO_TABLE;
> +        || a->type == OFPACT_GOTO_TABLE
> +        || a->type == OFPACT_METER;
> }
> 
> const char *ofpact_instruction_name_from_type(enum ovs_instruction_type type);
> diff --git a/lib/ofp-msgs.h b/lib/ofp-msgs.h
> index 66ec448..365d6d7 100644
> --- a/lib/ofp-msgs.h
> +++ b/lib/ofp-msgs.h
> @@ -217,7 +217,7 @@ enum ofpraw {
>     /* NXT 1.0+ (19): struct nx_async_config. */
>     OFPRAW_NXT_SET_ASYNC_CONFIG,
> 
> -    /* OFPT 1.3+ (29): struct ofp13_meter_mod. */
> +    /* OFPT 1.3+ (29): struct ofp13_meter_mod, uint16_t[]. */

A more exact length constraint would be ", uint8_t[8][].", as the actions are padded to 8 bytes. Same below.

>     OFPRAW_OFPT13_METER_MOD,
> 
> /* Standard statistics. */
> @@ -315,7 +315,7 @@ enum ofpraw {
>     /* OFPST 1.3+ (9): struct ofp13_meter_multipart_request. */
>     OFPRAW_OFPST13_METER_REQUEST,
> 
> -    /* OFPST 1.3+ (9): struct ofp13_meter_stats[]. */
> +    /* OFPST 1.3+ (9): uint8_t[]. */
>     OFPRAW_OFPST13_METER_REPLY,
> 
>     /* OFPST 1.3+ (10): struct ofp13_meter_multipart_request. */
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index 1890050..8f0c54e 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -718,6 +718,16 @@ parse_named_instruction(enum ovs_instruction_type type,
>         ogt->table_id = str_to_table_id(table_s);
>         break;
>     }
> +
> +    case OVSINST_OFPIT13_METER: {
> +        struct ofpact_meter *om = ofpact_put_METER(ofpacts);
> +        char *meter_s = strsep(&arg, ",");
> +        if (!meter_s || !meter_s[0]) {
> +            ovs_fatal(0, "instruction meter needs meter id");
> +        }
> +        om->meter_id = str_to_u32(meter_s);
> +        break;
> +    }
>     }
> 
>     /* If write_metadata is specified as an action AND an instruction, ofpacts
> @@ -1241,3 +1251,241 @@ exit:
>     }
>     return error;
> }
> +
> +void
> +parse_ofp_meter_mod_file(const char *file_name, uint16_t command,
> +                        struct ofputil_meter_mod **mms, size_t *n_mms)
> +{
> +    size_t allocated_mms;
> +    FILE *stream;
> +    struct ds s;
> +
> +    stream = !strcmp(file_name, "-") ? stdin : fopen(file_name, "r");
> +    if (stream == NULL) {
> +        ovs_fatal(errno, "%s: open", file_name);
> +    }
> +
> +    allocated_mms = *n_mms;
> +    ds_init(&s);
> +    while (!ds_get_preprocessed_line(&s, stream)) {
> +        if (*n_mms >= allocated_mms) {
> +            *mms = x2nrealloc(*mms, &allocated_mms, sizeof **mms);
> +        }
> +        parse_ofp_meter_mod_str(&(*mms)[*n_mms], ds_cstr(&s), command, false);
> +        *n_mms += 1;
> +    }
> +    ds_destroy(&s);
> +
> +    if (stream != stdin) {
> +        fclose(stream);
> +    }
> +}
> +
> +static int
> +parse_meter_flags_str(const char* str, uint16_t* flags)
> +{
> +    while(*str)
> +    {
> +        if (!strncasecmp(str,"KBPS", 4)) {
> +            *flags |= OFPMF13_KBPS;
> +            str += 4;

Virtually all other parsing is done in case sensitive fashion, so maybe meter-related parsing should also be case sensitive? E.g., flow flags are parsed as case sensitive lowercase labels, so maybe meter flags should also be lower case?

> +        } else if (!strncasecmp(str,"PKTPS", 5)) {
> +            *flags |= OFPMF13_PKTPS;
> +            str += 5;
> +        } else if (!strncasecmp(str,"BURST", 5)) {
> +            *flags |= OFPMF13_BURST;
> +            str += 5;
> +        } else if (!strncasecmp(str,"STATS", 5)) {
> +            *flags |= OFPMF13_STATS;
> +            str += 5;
> +        } else if (*str == '-' || *str == '_') {
> +            str++;
> +        } else {
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static void
> +parse_ofp_meter_str(struct ofputil_meter_mod *mm, int command, const char *str_,
> +              bool verbose)
> +{
> +    enum {
> +        F_METER_ID  = 1 << 0,
> +        F_BANDS     = 1 << 1
> +    } fields;
> +#define INVALID_RATE  0xffffffff
> +#define INVALID_BURST 0xffffffff
> +
> +    char *string = NULL;
> +    char *save_ptr = NULL;
> +    char *name;
> +    size_t band = 0, band_i = 0;
> +    struct ofp13_meter_band_drop bands[OFP_MAX_BAND_PER_METER + 1];
> +
> +    switch (command) {
> +    case OFPMC13_ADD:
> +        fields = F_METER_ID | F_BANDS;
> +        break;
> +
> +    case OFPMC13_MODIFY:
> +        fields = F_METER_ID | F_BANDS;
> +        break;
> +
> +    case OFPMC13_DELETE:
> +        fields = F_METER_ID;
> +        break;
> +
> +    default:
> +        NOT_REACHED();
> +    }
> +
> +    memset(&bands, 0x0, sizeof(bands));
> +    memset(mm, 0x0, sizeof(*mm));
> +    mm->command = command;
> +    mm->meter_id = OFPM13_ALL;
> +
> +    if (OFPMC13_DELETE == mm->command && !strlen(str_)) {
> +        mm->meter_id = OFPM13_ALL;
> +        return;
> +    }
> +    mm->flags = 0;
> +
> +    string = xstrdup(str_);
> +
> +    for (band_i = 1; band_i <= OFP_MAX_BAND_PER_METER; band_i++) {
> +        bands[band_i].type = htons(OFPMBT13_EXPERIMENTER);
> +        bands[band_i].rate = htonl(INVALID_RATE);
> +        bands[band_i].burst_size = htonl(INVALID_BURST);
> +    }
> +
> +    for (name = strtok_r(string, "=, \t\r\n", &save_ptr); name;
> +        name = strtok_r(NULL, "=, \t\r\n", &save_ptr)) {
> +
> +        char *value;
> +
> +        value = strtok_r(NULL, ", \t\r\n", &save_ptr);
> +        if (!value) {
> +            ofp_fatal(str_, verbose, "field %s missing value", name);
> +        }
> +
> +        /*modified by liul for bug 22403, 2013-03-04*/
> +        /*command "delete" is only need "meter_id"*/
> +        if (OFPMC13_DELETE == mm->command) {
> +            if (!strcasecmp(name, "meter_id")) {

The same case sensitivity comment applies here...

> +                if(!strcasecmp(value, "all")) {
> +                    mm->meter_id = OFPM13_ALL;
> +                } else {
> +                    mm->meter_id = str_to_u32(value);
> +                    if (!mm->meter_id) {
> +                        ofp_fatal(str_, verbose, "invalid meter_id %"PRIu32"", mm->meter_id);
> +                    }
> +                }
> +            } else {
> +                ofp_fatal(str_, verbose, "only meter_id is needed");
> +            }
> +            return;
> +        } 
> +

It would be nice if the special case for the DELETE could be covered via the enum fields specified above.

> +        if (!strcasecmp(name, "meter_id")) {
> +            if(!strcasecmp(value, "all")) {
> +                mm->meter_id = OFPM13_ALL;
> +            } else {
> +                mm->meter_id = str_to_u32(value);
> +                if (!mm->meter_id) {
> +                    ofp_fatal(str_, verbose, "invalid meter_id %"PRIu32"", mm->meter_id);
> +                }
> +            }
> +        } else if (!strcasecmp(name, "flags")) {
> +            if (parse_meter_flags_str(value, &mm->flags)) {
> +                ofp_fatal(str_, verbose, "invalid meter flags %s", value);
> +            }

Alternatively, the flags could be parsed as individual keywords, like with flow parsing.

> +        } else if (!strcasecmp(name, "band_type")){
> +            if (!mm->flags) {
> +                ofp_fatal(str_, verbose, "must specify flags before band_type");
> +            }
> +            if (band >= OFP_MAX_BAND_PER_METER) {
> +                ofp_fatal(str_, verbose, "Only %"PRIu32" bands per meter are supported.", OFP_MAX_BAND_PER_METER);
> +            }
> +            if (!strcasecmp(value, "drop")) {
> +                bands[++band].type = htons(OFPMBT13_DROP);
> +            } else {
> +                ofp_fatal(str_, verbose, "invalid band type %s", value);
> +            }
> +        } else if (!strcasecmp(name, "rate")) {
> +            if (band < 1) {
> +                ofp_fatal(str_, verbose, "must specify a band type before rate");
> +            }
> +            bands[band].rate = htonl(str_to_u32(value));
> +        } else if (!strcasecmp(name, "burst")) {
> +            if (band < 1) {
> +                ofp_fatal(str_, verbose, "must specify a band type before burst");
> +            }
> +            if (!(mm->flags & OFPMF13_BURST)) {
> +                ofp_fatal(str_, verbose, "BURST is not specified in flags");
> +            }
> +            bands[band].burst_size = htonl(str_to_u32(value));
> +        } else {
> +            ofp_fatal(str_, verbose, "unknown keyword %s", name);
> +        }
> +    }
> +    if ( fields & F_METER_ID) {
> +        if (!mm->meter_id) {
> +            ofp_fatal(str_, verbose, "must specify a meter_id");
> +        }
> +    }
> +    if ( fields & F_BANDS) {
> +        if (!mm->flags) {
> +            ofp_fatal(str_, verbose, "must specify meter flags");
> +        }
> +        if (!band) {
> +            ofp_fatal(str_, verbose, "must specify a band type");
> +        }
> +        for (band_i = 1; band_i <= band; band_i++) {
> +            if (htons(OFPMBT13_EXPERIMENTER) == bands[band_i].type) {
> +                ofp_fatal(str_, verbose, "must specify a band type");
> +            }
> +            if (htonl(INVALID_RATE) == bands[band_i].rate) {
> +                ofp_fatal(str_, verbose, "must specify a band rate");
> +            }
> +            if ((mm->flags & OFPMF13_BURST) &&
> +                htonl(INVALID_BURST) == bands[band_i].burst_size) {
> +                ofp_fatal(str_, verbose, "must specify a band burst");
> +            }
> +
> +            if (bands[band_i].type == htons(OFPMBT13_DROP)) {
> +                bands[band_i].len = htons(sizeof(struct ofp13_meter_band_drop));
> +            }
> +        }
> +        if (band) {
> +            mm->n_bands = band;
> +            mm->bands = malloc(sizeof(struct ofp13_meter_band_drop) * band);
> +            memcpy(mm->bands, &bands[1], sizeof(struct ofp13_meter_band_drop) * band);
> +        }
> +    } else {
> +        if (band || mm->flags) {
> +            ofp_fatal(str_, verbose, "only meter_id is needed");
> +        }
> +    }
> +
> +    free(string);
> +}
> +
> +void
> +parse_ofp_meter_mod_str(struct ofputil_meter_mod *mm, const char *string,
> +        uint16_t command, bool verbose)
> +{
> +    parse_ofp_meter_str(mm, command, string, verbose);
> +}
> +
> +void
> +parse_ofp_meter_request_str(struct ofputil_meter_request *mr, char *string)
> +{
> +    struct ofputil_meter_mod mm;
> +
> +    memset(&mm, 0, sizeof(mm));
> +    parse_ofp_meter_str(&mm, OFPMC13_DELETE, string, false);
> +    mr->meter_id = mm.meter_id;
> +}
> diff --git a/lib/ofp-parse.h b/lib/ofp-parse.h
> index d2d3c3c..4620eae 100644
> --- a/lib/ofp-parse.h
> +++ b/lib/ofp-parse.h
> @@ -28,6 +28,9 @@ struct ofpbuf;
> struct ofputil_flow_mod;
> struct ofputil_flow_monitor_request;
> struct ofputil_flow_stats_request;
> +/* Added by yanxa, for bug 21920, 2012-1-5 */
> +struct ofputil_meter_mod;
> +struct ofputil_meter_request;
> 
> void parse_ofp_str(struct ofputil_flow_mod *, int command, const char *str_,
>                    bool verbose);
> @@ -48,4 +51,9 @@ char *parse_ofp_exact_flow(struct flow *, const char *);
> void parse_flow_monitor_request(struct ofputil_flow_monitor_request *,
>                                 const char *);
> 
> +void parse_ofp_meter_mod_file(const char *file_name, uint16_t command,
> +                        struct ofputil_meter_mod **mms, size_t *n_mms);
> +void parse_ofp_meter_mod_str(struct ofputil_meter_mod *mm, const char *string,
> +                            uint16_t command, bool verbose);
> +void parse_ofp_meter_request_str(struct ofputil_meter_request *mr, char *string);
> #endif /* ofp-parse.h */
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index 42fd9a6..7e35ef2 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -1896,6 +1896,201 @@ ofp_print_not_implemented(struct ds *string)
>     ds_put_cstr(string, "NOT IMPLEMENTED YET!\n");
> }
> 
> +/* Meter */
> +static void
> +ofp_print_meter_flags(struct ds *s, uint16_t flags)
> +{
> +    if (flags & OFPMF13_KBPS) {
> +        ds_put_cstr(s, "KBPS");
> +    }
> +    if (flags & OFPMF13_PKTPS) {
> +        ds_put_cstr(s, "-PKTPS");
> +    }
> +    if (flags & OFPMF13_BURST) {
> +        ds_put_cstr(s, "-BURST");
> +    }
> +    if (flags & OFPMF13_STATS) {
> +        ds_put_cstr(s, "-STATS");
> +    }
> +}
> +
> +static void
> +ofp_print_meter_mod(struct ds *s, const struct ofp_header *oh)
> +{
> +    size_t band_i;
> +    struct ofputil_meter_mod mm;
> +    int error;
> +
> +    memset(&mm, 0, sizeof(mm));
> +    error = ofputil_decode_meter_mod(&mm, oh);

It would be customary for the *decode* to take care of initializing the 'mm' as needed, so that the memset would not be needed here.

> +    if (error) {
> +        ofp_print_error(s, error);
> +        return;
> +    }
> +
> +    ds_put_char(s, '\n');
> +
> +    ds_put_char(s, ' ');
> +    switch (mm.command) {
> +    case OFPMC13_ADD:
> +        ds_put_cstr(s, "ADD");
> +        break;
> +
> +    case OFPMC13_MODIFY:
> +        ds_put_cstr(s, "MOD");
> +        break;
> +
> +    case OFPMC13_DELETE:
> +        ds_put_cstr(s, "DEL");
> +        break;
> +
> +    default:
> +        ds_put_format(s, "cmd:%"PRIu16"", mm.command);
> +    }
> +    ds_put_char(s, ' ');
> +
> +    ds_put_format(s, "meter_id=%"PRIu32"", mm.meter_id);

The empty string after the 'PRIu32' is unnecessary.

> +    ds_put_format(s, ",flags=");
> +    ofp_print_meter_flags(s, mm.flags);
> +
> +    for (band_i = 0; band_i < mm.n_bands; band_i++) {
> +        ds_put_format(s, ",band_type=%s,", ntohs(mm.bands[band_i].type) == OFPMBT13_DROP ? "DROP" : "KNOWN");

Would numeric output be better than "KNOWN" in case of unsupported band type?

> +        ds_put_format(s, "rate=%"PRIu32"", ntohl(mm.bands[band_i].rate));
> +
> +        if (mm.flags & OFPMF13_BURST) {
> +            ds_put_format(s, ",burst=%"PRIu32"", ntohl(mm.bands[band_i].burst_size));
> +        }
> +    }
> +}
> +
> +static void
> +ofp_print_meter_request(struct ds *s, const struct ofp_header *oh)
> +{
> +    struct ofputil_meter_request omr;
> +    enum ofperr error;
> +
> +    error = ofputil_decode_meter_request(&omr, oh);
> +    if (error) {
> +        ofp_print_error(s, error);
> +        return;
> +    }
> +
> +    ds_put_char(s, ' ');
> +    ds_put_cstr(s, "METER_STATS");
> +    ds_put_char(s, ' ');

It should be unnecessary to print the stats request type here.

> +    if (OFPM13_ALL == omr.meter_id) {
> +        ds_put_format(s, "meter_id=all");
> +    } else {
> +        ds_put_format(s, "meter_id=%"PRIu32"", omr.meter_id);
> +    }
> +}
> +
> +static void
> +ofp_print_meter_reply(struct ds *s, const struct ofp_header *oh)
> +{
> +    struct ofpbuf b;
> +    size_t band_i;
> +
> +    ofpbuf_use_const(&b, oh, ntohs(oh->length));
> +    for (;;) {
> +        struct ofputil_meter_stats_reply omsr;
> +        enum ofperr error;
> +
> +        error = ofputil_decode_meter_reply(&omsr, &b);
> +        if (error) {
> +            if (error != EOF) {
> +                ofp_print_error(s, error);
> +            }
> +            break;
> +        }
> +        ds_put_char(s, '\n');
> +
> +        ds_put_char(s, ' ');
> +        ds_put_format(s, "meter_id=%"PRIu32",", omsr.meter_id);
> +
> +        ds_put_cstr(s, "duration=");
> +        ofp_print_duration(s, omsr.duration_sec, omsr.duration_nsec);
> +        ds_put_format(s, ",flow_count=%"PRIu32",", omsr.flow_count);
> +        ds_put_format(s, "packet_in_count=%"PRIu64",", omsr.packet_in_count);
> +        ds_put_format(s, "byte_in_count=%"PRIu64",", omsr.byte_in_count);
> +
> +        for (band_i = 0; band_i < omsr.n_bands; band_i++) {
> +            ds_put_format(s, "packet_band_count=%"PRIu64",", ntohll(omsr.bands_stats[band_i].packet_band_count));
> +            ds_put_format(s, "byte_band_count=%"PRIu64"", ntohll(omsr.bands_stats[band_i].byte_band_count));
> +        }
> +    }
> +}
> +
> +static void
> +ofp_print_meter_config_request(struct ds *s, const struct ofp_header *oh)
> +{
> +    struct ofputil_meter_request omr;
> +    enum ofperr error;
> +
> +    error = ofputil_decode_meter_request(&omr, oh);
> +    if (error) {
> +        ofp_print_error(s, error);
> +        return;
> +    }
> +
> +    ds_put_char(s, ' ');
> +    ds_put_cstr(s, "METER_CONFIG");
> +    ds_put_char(s, ' ');

It should be unnecessary to print the stats request type here.

> +    if (OFPM13_ALL == omr.meter_id) {
> +        ds_put_format(s, "meter_id=all");
> +    } else {
> +        ds_put_format(s, "meter_id=%"PRIu32"", omr.meter_id);
> +    }
> +}
> +
> +static void
> +ofp_print_meter_config_reply(struct ds *s, const struct ofp_header *oh)
> +{
> +    struct ofpbuf b;
> +    size_t band_i;
> +
> +    ofpbuf_use_const(&b, oh, ntohs(oh->length));
> +    for (;;) {
> +        struct ofputil_meter_config_reply omcr;
> +        enum ofperr error;
> +
> +        error = ofputil_decode_meter_config_reply(&omcr, &b);
> +        if (error) {
> +            if (error != EOF) {
> +                ofp_print_error(s, error);
> +            }
> +            break;
> +        }
> +
> +        ds_put_char(s, '\n');
> +        ds_put_char(s, ' ');
> +        ds_put_format(s, "meter_id=%"PRIu32",flags=", omcr.meter_id);
> +        ofp_print_meter_flags(s, omcr.flags);
> +        ds_put_char(s, ',');
> +
> +        for (band_i = 0; band_i < omcr.n_bands; band_i++) {
> +            ds_put_format(s, "band_type=%s,", ntohs(omcr.bands[band_i].type) == OFPMBT13_DROP ? "DROP" : "KNOWN");
> +            ds_put_format(s, "rate=%"PRIu32",", ntohl(omcr.bands[band_i].rate));
> +            ds_put_format(s, "burst=%"PRIu32"", ntohl(omcr.bands[band_i].burst_size));
> +        }

Some duplicated code here. Also, should the 'burst' be printed here if not specified by the flags?

> +    }
> +}
> +
> +static void
> +ofp_print_meter_feature_reply(struct ds *s, const struct ofp13_meter_features *nmf)
> +{
> +    ds_put_char(s, '\n');
> +
> +    ds_put_format(s, "max_meter=%"PRIu32",", ntohl(nmf->max_meter));
> +    ds_put_format(s, " band_types=%s\n", ntohl(nmf->band_types) == OFPMBT13_DROP ? "DROP" : "KNOWN");
> +    ds_put_cstr(s,   "capabilities=");
> +    ofp_print_meter_flags(s, ntohl(nmf->capabilities));
> +
> +    ds_put_char(s, '\n');
> +    ds_put_format(s, "max_bands=%"PRIu8",", nmf->max_bands);
> +    ds_put_format(s, " max_color=%"PRIu32"\n", nmf->max_color);
> +}
> +
> static void
> ofp_to_string__(const struct ofp_header *oh, enum ofpraw raw,
>                 struct ds *string, int verbosity)
> @@ -1910,24 +2105,47 @@ ofp_to_string__(const struct ofp_header *oh, enum ofpraw raw,
>     case OFPTYPE_QUEUE_GET_CONFIG_REPLY:
>     case OFPTYPE_GET_ASYNC_REQUEST:
>     case OFPTYPE_GET_ASYNC_REPLY:
> -    case OFPTYPE_METER_MOD:
>     case OFPTYPE_GROUP_REQUEST:
>     case OFPTYPE_GROUP_REPLY:
>     case OFPTYPE_GROUP_DESC_REQUEST:
>     case OFPTYPE_GROUP_DESC_REPLY:
>     case OFPTYPE_GROUP_FEATURES_REQUEST:
>     case OFPTYPE_GROUP_FEATURES_REPLY:
> -    case OFPTYPE_METER_REQUEST:
> -    case OFPTYPE_METER_REPLY:
> -    case OFPTYPE_METER_CONFIG_REQUEST:
> -    case OFPTYPE_METER_CONFIG_REPLY:
> -    case OFPTYPE_METER_FEATURES_REQUEST:
> -    case OFPTYPE_METER_FEATURES_REPLY:
>     case OFPTYPE_TABLE_FEATURES_REQUEST:
>     case OFPTYPE_TABLE_FEATURES_REPLY:
>         ofp_print_not_implemented(string);
>         break;
> 
> +    /* Meter implementation */
> +    case OFPTYPE_METER_MOD:
> +        ofp_print_meter_mod(string, oh);
> +        break;
> +
> +    case OFPTYPE_METER_CONFIG_REQUEST:

Technically, this is also a "stats" (or, rather, a "multi-part") request, so should have this here:

        ofp_print_stats_request(string, oh);

> 
> +        ofp_print_meter_config_request(string, oh);
> +        break;
> +

> +    case OFPTYPE_METER_FEATURES_REQUEST:
> +        ofp_print_stats_request(string, oh);
> +        break;
> +
> +    case OFPTYPE_METER_REPLY:
> +        ofp_print_stats_reply(string, oh);
> +        ofp_print_meter_reply(string, oh);
> +        break;
> +
> +    case OFPTYPE_METER_REQUEST:
> +        ofp_print_stats_request(string, oh);
> +        ofp_print_meter_request(string, oh);
> +        break;
> +
> +    case OFPTYPE_METER_CONFIG_REPLY:

Also, this is a multi-part reply, so should have this here:

        ofp_print_stats_reply(string, oh);

> +        ofp_print_meter_config_reply(string, oh);
> +        break;
> +
> +    case OFPTYPE_METER_FEATURES_REPLY:

This too (even if it will never have more than 1 part :-)

> +        ofp_print_meter_feature_reply(string, ofpmsg_body(oh));
> +        break;
>     case OFPTYPE_HELLO:
>         ofp_print_hello(string, oh);
>         break;
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 90f4f35..04d5ae2 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -5059,3 +5059,406 @@ ofputil_append_queue_stat(struct list *replies,
>         NOT_REACHED();
>     }
> }
> +
> +/* Meter */
> +/* Find out usable protocols for meter */
> +enum ofputil_protocol
> +ofputil_meter_usable_protocols(void)
> +{
> +    enum ofputil_protocol usable_protocols;
> +
> +    usable_protocols = OFPUTIL_P_ANY;
> +    usable_protocols &= OFPUTIL_P_OF13_OXM;
> +
> +    return usable_protocols;
> +}
> +
> +/* Pull bands */
> +static enum ofperr
> +ofputil_pull_bands(struct ofpbuf *b, unsigned int bands_len,
> +                     struct ofp13_meter_band_drop **bandsp, size_t *n_bandsp)
> +{
> +    if (bands_len % OFP_METER_BAND_ALIGN != 0) {
> +        VLOG_WARN_RL(&bad_ofmsg_rl, "OpenFlow message bands length %u "
> +                     "is not a multiple of %d", bands_len, OFP_METER_BAND_ALIGN);
> +        goto error;
> +    }
> +
> +    *bandsp = ofpbuf_try_pull(b, bands_len);

It would be good to validate the bands and convert to host byte order while doing so.

> +    if (*bandsp == NULL) {
> +        VLOG_WARN_RL(&bad_ofmsg_rl, "OpenFlow message bands length %u "
> +                     "exceeds remaining message length (%zu)",
> +                     bands_len, b->size);
> +        goto error;
> +    }
> +
> +    *n_bandsp = bands_len / OFP_METER_BAND_ALIGN;
> +    return 0;
> +
> +error:
> +    *bandsp = NULL;
> +    *n_bandsp = 0;
> +
> +    return OFPERR_OFPBRC_BAD_LEN;
> +}
> +
> +/* Converts 'mm' into an OFPT_METER_MOD message returns the message. */
> +struct ofpbuf *
> +ofputil_encode_meter_mod(const struct ofputil_meter_mod *mm,
> +                    enum ofp_version ofp_version)
> +{
> +    struct ofpbuf *msg;
> +    size_t band;
> +
> +    switch (ofp_version) {
> +        case OFP13_VERSION: {
> +            struct ofp13_meter_mod *omm;
> +
> +            msg = ofpraw_alloc(OFPRAW_OFPT13_METER_MOD, ofp_version, 0);
> +            omm = ofpbuf_put_zeros(msg, sizeof *omm);
> +            omm->command  = htons(mm->command);
> +            omm->flags    = htons(mm->flags);
> +            omm->meter_id = htonl(mm->meter_id);
> +
> +            for(band = 0; band < mm->n_bands; band ++)
> +            {
> +                ofpbuf_put(msg, (const void *)&(mm->bands[band]), sizeof(struct ofp13_meter_band_drop));
> +            }
> +            break;
> +        }
> +        case OFP10_VERSION:
> +        case OFP11_VERSION:
> +        case OFP12_VERSION:
> +            return NULL;
> +        default:
> +            NOT_REACHED();
> +    }
> +
> +    ofpmsg_update_length(msg);
> +
> +    return msg;
> +}
> +
> +/* Decode meter mod message */
> +enum ofperr
> +ofputil_decode_meter_mod(struct ofputil_meter_mod *mm,
> +                        const struct ofp_header *oh)
> +{
> +    struct ofpbuf b;
> +    enum ofpraw raw;
> +
> +    ofpbuf_use_const(&b, oh, ntohs(oh->length));
> +    raw = ofpraw_pull_assert(&b);
> +    if (raw == OFPRAW_OFPT13_METER_MOD) {

There is no other kind of meter_mod, so the ofpbuf_use_const() and
ofpraw_pull_assert() are unnecessary here.

> +        const struct ofp13_meter_mod *omm;
> +        enum ofperr error;
> +
> +        omm = ofpbuf_pull(&b, sizeof *omm);
> +
> +        if (ntohs(omm->command) != OFPMC13_DELETE) {
> +            if (!b.size) {
> +                VLOG_WARN_RL(&bad_ofmsg_rl, "Invalid meter id: %"PRIu32", need specify a band",
> +                    ntohl(omm->meter_id));
> +                return OFPERR_OFPMMFC_INVALID_METER;
> +            }

> +
> +            error  = ofputil_pull_bands(&b, b.size, &mm->bands, &mm->n_bands);
> +            if (error) {
> +                return error;
> +            }
> +
> +            if (mm->n_bands > OFP_MAX_BAND_PER_METER) {
> +                VLOG_WARN_RL(&bad_ofmsg_rl, "Invalid meter, id : %"PRIu32". Only %"PRIu32" bands per meter are supported.",
> +                    ntohl(omm->meter_id),
> +                    OFP_MAX_BAND_PER_METER);
> +                return OFPERR_OFPMMFC_OUT_OF_BANDS;
> +            }
> +        }

else initialize mm->bands to NULL and mm->n_bands to 0?

> +        if (b.size) {
> +            VLOG_WARN_RL(&bad_ofmsg_rl, "METER_MOD request claims invalid "
> +                         "length %zu", b.size);
> +            return OFPERR_OFPMMFC_INVALID_METER;
> +        }
> +
> +        mm->meter_id = ntohl(omm->meter_id);
> +        mm->flags    = ntohs(omm->flags);

flags should be ignored for a DELETE.

> +        mm->command  = ntohs(omm->command);
> +
> +    } else {
> +        NOT_REACHED();
> +    }
> +
> +    return 0;
> +}
> +

...

> diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> index f8705a2..bab1d02 100644
> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> @@ -729,4 +729,111 @@ int ofputil_decode_queue_stats(struct ofputil_queue_stats *qs, struct ofpbuf *ms
> void ofputil_append_queue_stat(struct list *replies,
>                                const struct ofputil_queue_stats *oqs);
> 
> +/* Meter modify message */
> +#define OFP_METER_BAND_ALIGN 16      /* Alignment of ofp_actions. */

OpenFlow text says: "NB: The length of an action *must* always be a multiple of eight." so the comment is incorrect.

Also, the minimum band length is 16, but supposedly the band config structure for a future band type could be, e.g., 24 bytes long.

> +#define OFP_MAX_BAND_PER_METER  1

I guess this is enough for the 'drop' type :-)

> +
> +struct ofputil_meter_stats {
> +    uint64_t packet_in_count;   /* Number of packets in input. */
> +    uint64_t byte_in_count;     /* Number of bytes in input. */
> +
> +    struct ofp13_meter_band_stats bands_stats[OFP_MAX_BAND_PER_METER];
> +    size_t n_bands;
> +};

size_t seems like an overkill for n_bands. Also, you use uint16_t below, which is inconsistent.

> +
> +struct ofputil_meter_mod {
> +    uint16_t command;         /* One of OFPMC_*. */
> +    uint16_t flags;           /* One of OFPMF_*. */
> +    uint32_t meter_id;        /* Meter instance. */
> +    struct ofp13_meter_band_drop *bands; /*TODO Check band type */
> +    size_t n_bands;
> +};

IMO it would be better to also define an "struct ofputil_meter_band" that could be a "union" of all supported meter band types, and also have the host endianness in the values. Now it is a bit uneven, when some fields are converted to host byte order, while others are not, which results in some unnecessary conversions, it seems. Also, figuring out the 'n_bands' in the future could also require parsing the bands as they are pulled.

Now, when referring to the OpenFlow message data, the bands pointer should be const, since the OpenFlow message data may not be modified as the request may have to be postponed.

> +
> +struct ofputil_meter_request {
> +    uint32_t meter_id;
> +    uint8_t pad[4];
> +};

The pad should not be necessary for an internal representation.

> +
> +struct ofputil_meter_band_stats_reply {
> +    uint64_t packet_band_count;   /* Number of packets in band. */
> +    uint64_t byte_band_count;     /* Number of bytes in band. */
> +};
> +
> +struct ofputil_meter_stats_reply {
> +    uint32_t meter_id;          /* Meter instance. */
> +    uint16_t len;               /* Length in bytes of this stats. */
> +    uint8_t  pad[6];            /* Padding */
> +    uint32_t flow_count;        /* Number of flows bound to meter. */
> +    uint64_t packet_in_count;   /* Number of packets in input. */
> +    uint64_t byte_in_count;     /* Number of bytes in input. */
> +    uint32_t duration_sec;      /* Time meter has been alive in seconds. */
> +    uint32_t duration_nsec;     /* Time meter has been alive in nanoseconds beyond duration_sec. */
> +
> +    struct ofp13_meter_band_stats* bands_stats;
> +    uint16_t n_bands;
> +    uint16_t resv;
> +};

The "len", "pad", and "resv" fields should not be necessary for an internal representation.

> +
> +struct ofputil_meter_band_config_reply {
> +    uint16_t type;
> +    uint16_t len;
> +    uint32_t rate;
> +    uint32_t burst_size;
> +    uint32_t resved;
> +};

The "len" and "resved" fields should not be necessary for an internal representation.

> +
> +struct ofputil_meter_config_reply {
> +    uint16_t length;
> +    uint16_t flags;
> +    uint32_t meter_id;
> +
> +    struct ofp13_meter_band_drop* bands;
> +    uint16_t n_bands;
> +    uint16_t resv;
> +};
> +

The "length" and "resv" fields should not be necessary for an internal representation.

> +struct ofputil_meter_features_reply {
> +    uint32_t max_meter;         /* Maximum number of meters. */
> +    uint32_t band_types;        /* Bitmaps of OFPMBT_* values supported. */
> +    uint32_t capabilities;      /* Bitmaps of "nx_meter_flags". */
> +    uint8_t max_bands;          /* Maximum bands per meters */
> +    uint8_t max_color;          /* Maximum color value */
> +};
> +
> +/* Meter */
> +enum ofputil_protocol
> +ofputil_meter_usable_protocols(void);

By convention these should be on the same line.

> +struct ofpbuf *
> +ofputil_encode_meter_mod(const struct ofputil_meter_mod *mm, enum ofp_version ofp_version);
> +struct ofpbuf *
> +ofputil_encode_meter_stats_request(const struct ofputil_meter_request *msr, enum ofp_version ofp_version);
> +struct ofpbuf *
> +ofputil_encode_meter_config_request(const struct ofputil_meter_request *msr, enum ofp_version ofp_version);
> +struct ofpbuf *
> +ofputil_encode_meter_features_request(enum ofp_version ofp_version);
> +enum ofperr
> +ofputil_decode_meter_mod(struct ofputil_meter_mod *mm,
> +                                const struct ofp_header *oh);
> +enum ofperr
> +ofputil_decode_meter_request(struct ofputil_meter_request *msr,
> +                                 const struct ofp_header *oh);
> +enum ofperr
> +ofputil_decode_meter_reply(struct ofputil_meter_stats_reply *omsr,
> +                                struct ofpbuf *msg);
> +enum ofperr
> +ofputil_decode_meter_config_reply(struct ofputil_meter_config_reply *omcr,
> +                                struct ofpbuf *msg);
> +void
> +ofputil_append_meter_stats_reply(const struct ofputil_meter_stats_reply *msr,
> +                                struct list *replies);
> +void
> +ofputil_append_meter_band_stats_reply(const struct ofputil_meter_band_stats_reply *mbsr,
> +                                struct list *replies);
> +void
> +ofputil_append_meter_config_reply(const struct ofputil_meter_config_reply *mcr,
> +                                struct list *replies);
> +void
> +ofputil_append_meter_band_config_reply(const struct ofputil_meter_band_config_reply *mbc,
> +                                struct list *replies);
> +
> #endif /* ofp-util.h */
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a52a8cf..dd27e9f 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1772,6 +1772,10 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>             break;
>         }
> 
> +        case OFPACT_METER:
> +            NOT_REACHED();
> +            break;
> +
>         case OFPACT_SAMPLE:
>             xlate_sample_action(ctx, ofpact_get_SAMPLE(a));
>             break;
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 6fa7894..09060c8 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -6772,4 +6772,13 @@ const struct ofproto_class ofproto_dpif_class = {
>     forward_bpdu_changed,
>     set_mac_table_config,
>     set_realdev,
> +
> +    NULL,                       /* meter_alloc */
> +    NULL,                       /* meter_construct */
> +    NULL,                       /* meter_destruct */
> +    NULL,                       /* meter_dealloc */

The alloc/dealloc interface seems unused.

> +    NULL,                       /* meter_get_stats */
> +    NULL,                       /* meter_update */
> +    NULL,                       /* meter_get_refcnt */

As the OF rule table is managed at the ofproto level, it seems to me that the mapping from meters to rules should be as well.

> +    NULL,                       /* meter_get_features */
> };
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 565cb01..da9e0c5 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -24,6 +24,7 @@
> #include "classifier.h"
> #include "heap.h"
> #include "hindex.h"
> +#include "ihash.h"
> #include "list.h"
> #include "ofp-errors.h"
> #include "ofp-util.h"
> @@ -100,6 +101,8 @@ struct ofproto {
>     unsigned long int *vlan_bitmap; /* 4096-bit bitmap of in-use VLANs. */
>     bool vlans_changed;             /* True if new VLANs are in use. */
>     int min_mtu;                    /* Current MTU of non-internal ports. */
> +
> +    struct ihash meters;          /* hash struct ofmeter indexed by meter-id */
> };
> 
> void ofproto_init_tables(struct ofproto *, int n_tables);
> @@ -257,6 +260,20 @@ bool ofoperation_has_out_port(const struct ofoperation *, ofp_port_t out_port);
> 
> bool ofproto_rule_is_hidden(const struct rule *);
> 
> +struct ofmeter {
> +    struct ofproto *ofproto;     /* The ofproto that contains this meter. */
> +

'ofproto' seems to be here only for the sake of passing one less argument in the ofproto_class interface. I'd rather not use the space here.

> +    long long int created;       /* Creation time. */
> +    long long int modified;      /* Time of last modification. */

OpenFlow interface does not require the 'modified' field.

> +
> +    uint32_t meter_id;
> +
> +    struct ofp13_meter_band_drop bands[OFP_MAX_BAND_PER_METER];

For now (OFP_MAX_BAND_PER_METER == 1) this is OK, but when datapaths start supporting multiple meter band types and meter band types for which multiple meter bands make sense, max_bands may be 10 or more. That's why I think 'bands' should be dynamically allocated.

> +    uint16_t n_bands;
> +
> +    uint16_t flags;              /* Bitmap of NXMF_* */
> +};
> +
> /* ofproto class structure, to be defined by each ofproto implementation.
>  *
>  *
> @@ -1321,6 +1338,16 @@ struct ofproto_class {
>      * it. */
>     int (*set_realdev)(struct ofport *ofport,
>                        ofp_port_t realdev_ofp_port, int vid);
> +
> +    struct ofmeter *(*meter_alloc)(void);
> +    int (*meter_construct)(struct ofmeter *meter);
> +    int (*meter_destruct)(struct ofmeter *meter);
> +    void (*meter_dealloc)(struct ofmeter *meter);

IMO the above interfaces should not be needed as such. My rationale is that the datapath needs to maintain it's own meter table to be able to act on meters on per-packet basis. That table can contain whatever data the datapath needs for the implementation (such as the means for the current rate measurement), so the datapaths should not have a need to piggyback on the ofproto-level meter table.

Also, as a datapath instance (a backer) can be used to support multiple ofproto instances, on the datapath level the OpenFlow meter_ids can be overlapping. It seems to me that it is the responsibility of a ofproto_class instance to maintain the mapping from OpenFlow meters to datapath meters (like for port numbers currently), and I could not figure out anything else for the ofproto_class instance to do w.r.t. meter table. That is why I proposed including a 32-bit "cookie" field into the ofproto-level meter instance for the ofproto_class instance to fill in and use to facilitate the mapping between an OpenFlow meter and the corresponding datapath meter implementation.

> +    int (*meter_get_stats)(struct ofmeter *meter,
> +                        struct ofputil_meter_stats* meter_stats);
> +    int (*meter_update)(struct ofmeter *meter);
> +    int (*meter_get_refcnt)(struct ofmeter *meter, uint32_t* refcnt);

It seems to me that the reference count should be the responsibility of the ofproto rather than the ofproto_class instance.

> +    void (*meter_get_features)(struct ofproto *ofproto, uint32_t* max_meter, uint32_t* min_meter, uint8_t* max_bands);
> };

This interface lacks some of the features data that I think should be passed (more below). As the max_meter is used to fill in the corresponding field in the ofp13_meter_features I get it to be "Maximum number of meters," as specified in the comment in the OpenFlow spec. Hence, 'max_meter' is not the highest meter_id value in use, but the highest meter_id value the controller should ever try to configure. Meters start at 1, so I do not understand what 'min_meter' is for?

For reference: "The meter_id field uniquely identifies a meter within a switch. Meter are defined starting with meter_id=1 up to the maximum number of meters that the switch can support."

> 
> extern const struct ofproto_class ofproto_dpif_class;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index afd8e17..b67d7e2 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -438,6 +438,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
>     ofproto->vlan_bitmap = NULL;
>     ofproto->vlans_changed = false;
>     ofproto->min_mtu = INT_MAX;
> +    ihash_init(&ofproto->meters);
> 
>     error = ofproto->ofproto_class->construct(ofproto);
>     if (error) {
> @@ -1089,6 +1090,8 @@ ofproto_destroy__(struct ofproto *ofproto)
> 
>     free(ofproto->vlan_bitmap);
> 
> +    ihash_destroy_free_data(&ofproto->meters);
> +
>     ofproto->ofproto_class->dealloc(ofproto);
> }
> 
> @@ -4106,6 +4109,525 @@ handle_flow_monitor_cancel(struct ofconn *ofconn, const struct ofp_header *oh)
>     return 0;
> }
> 
> +static bool
> +rule_has_meter_id(const struct rule *p_rule, uint32_t meter_id)
> +{
> +    const struct ofpact *a;
> +    const struct ofpact *ofpacts = p_rule->ofpacts;
> +    int ofpacts_len = p_rule->ofpacts_len;
> +
> +    OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
> +        struct ofpact_meter * meter;
> +
> +        if (OFPACT_METER == a->type) {
> +            meter = ofpact_get_METER(a);
> +            if (meter->meter_id == meter_id) {
> +                return true;
> +            }
> +        }
> +    }
> +
> +    return false;
> +}
> +
> +static enum ofperr
> +collect_rules_loose_meter(struct ofproto *ofproto, uint8_t table_id,
> +                    const struct match *match, uint32_t meter_id, struct list *rules)
> +{
> +    struct oftable *table;
> +    struct cls_rule cr;
> +    enum ofperr error = 0;
> +    ovs_be64 cookie = 0;
> +    ovs_be64 cookie_mask = 0;
> +
> +    error = check_table_id(ofproto, table_id);
> +    if (error) {
> +        return error;
> +    }
> +
> +    list_init(rules);
> +    cls_rule_init(&cr, match, 0);
> +
> +    OFPROTO_FOR_EACH_TABLE (table, ofproto) {
> +        struct cls_cursor cursor;
> +        struct rule *rule;
> +
> +        cls_cursor_init(&cursor, &table->cls, NULL);
> +        CLS_CURSOR_FOR_EACH (rule, cr, &cursor) {
> +            if (rule->pending) {
> +                error = OFPROTO_POSTPONE;
> +                goto exit;
> +            }
> +            if (!ofproto_rule_is_hidden(rule)
> +                    && rule_has_meter_id(rule, meter_id)
> +                    && !((rule->flow_cookie ^ cookie) & cookie_mask)) {
> +                list_push_back(rules, &rule->ofproto_node);
> +            }
> +        }
> +    }
> +
> +exit:
> +    cls_rule_destroy(&cr);
> +    return error;
> +}

The above is *very* slow, as each meter delete traverses every action of every rule in every table (as it is used below). That is why I proposed adding a list node to struct rule for tracking which rules "belong" to which meter, if any.

> +
> +static void
> +ofproto_rule_expire_due_to_meter_del(struct rule *rule, uint8_t reason)
> +{
> +    struct ofproto *ofproto = rule->ofproto;
> +    struct ofopgroup *group;
> +
> +    ovs_assert(reason == OFPRR_DELETE);
> +
> +    ofproto_rule_send_removed(rule, reason);
> +
> +    group = ofopgroup_create_unattached(ofproto);
> +    ofoperation_create(group, rule, OFOPERATION_DELETE, reason);
> +    oftable_remove_rule(rule);
> +    rule->ofproto->ofproto_class->rule_destruct(rule);
> +    ofopgroup_submit(group);
> +}
> +
> +static int
> +meter_del_remove_rules(struct ofproto *ofproto, uint32_t meter_id)
> +{
> +    struct list rules;
> +    struct match match;
> +    struct rule *rule, *next;
> +    enum ofperr error;
> +
> +    list_init(&rules);
> +    match_init_catchall(&match);
> +    error = collect_rules_loose_meter(ofproto, 0, &match, meter_id, &rules);
> +    if (error || list_is_empty(&rules))
> +    {
> +        return error;
> +    }
> +
> +    LIST_FOR_EACH_SAFE (rule, next, ofproto_node, &rules) {
> +        ofproto_rule_expire_due_to_meter_del(rule, OFPRR_DELETE);
> +    }
> +
> +    return 0;
> +}
> +
> +static enum ofperr
> +add_meter(struct ofproto *ofproto, struct ofconn *ofconn,
> +         const struct ofputil_meter_mod *mm, const struct ofp_header *request)
> +{
> +    int error;
> +    struct ofmeter* new_meter = NULL;
> +    struct ihash_node *ihash_node = NULL;
> +
> +    ofconn = ofconn; /* TODO unused parameter */
> +    request = request; /* TODO unused parameter */
> +
> +    if (mm->meter_id < 1 || mm->meter_id > OFPM13_MAX) {
> +        VLOG_ERR("Fail to add meter, invalid meter_id %"PRIu32"\n", mm->meter_id);
> +        return OFPERR_OFPMMFC_INVALID_METER;
> +    }
> +
> +    ihash_node = ihash_find(&ofproto->meters, mm->meter_id);
> +
> +    if (ihash_node) {
> +        VLOG_ERR("Fail to add meter, meter_id %"PRIu32" is existed\n", mm->meter_id);
> +        return OFPERR_OFPMMFC_METER_EXISTS;
> +    }
> +
> +    new_meter = malloc(sizeof(struct ofmeter));
> +    memset(new_meter, 0, sizeof(struct ofmeter));
> +    new_meter->meter_id = mm->meter_id;
> +    new_meter->flags    = mm->flags;
> +    new_meter->n_bands  = mm->n_bands;
> +    memcpy(new_meter->bands, mm->bands, sizeof(struct ofp13_meter_band_drop) * mm->n_bands);
> +
> +    new_meter->ofproto = ofproto;
> +    new_meter->created = new_meter->modified = time_msec();
> +
> +    error = ofproto->ofproto_class->meter_construct(new_meter);
> +
> +    if (error) {
> +        free(new_meter);
> +        return error;
> +    }
> +
> +    ihash_add(&ofproto->meters, new_meter->meter_id, new_meter);
> +
> +    return 0;
> +}
> +
> +
> +static enum ofperr
> +modify_meter(struct ofproto *ofproto, struct ofconn *ofconn,
> +         const struct ofputil_meter_mod *mm, const struct ofp_header *request)
> +{
> +    int error;
> +    struct ofmeter* old_meter = NULL;
> +    struct ofmeter* new_meter = NULL;
> +    struct ihash_node *ihash_node = NULL;
> +
> +    ofconn = ofconn; /* TODO unused parameter */
> +    request = request; /* TODO unused parameter */
> +
> +    if (mm->meter_id < 1 || mm->meter_id > OFPM13_MAX) {
> +        VLOG_ERR("Fail to modify meter, invalid meter_id %"PRIu32"\n", mm->meter_id);
> +        return OFPERR_OFPMMFC_INVALID_METER;
> +    }
> +
> +    ihash_node = ihash_find(&ofproto->meters, mm->meter_id);
> +
> +    if (!ihash_node) {
> +        VLOG_ERR("Fail to modify meter, meter_id %"PRIu32" is existed\n", mm->meter_id);
> +        return OFPERR_OFPMMFC_UNKNOWN_METER;
> +    }
> +
> +    old_meter = (struct ofmeter*) ihash_node->data;
> +
> +    new_meter = malloc(sizeof(struct ofmeter));
> +    memset(new_meter, 0, sizeof(struct ofmeter));
> +    new_meter->meter_id = mm->meter_id;
> +    new_meter->flags    = mm->flags;
> +    new_meter->n_bands  = mm->n_bands;
> +    memcpy(new_meter->bands, mm->bands, sizeof(struct ofp13_meter_band_drop) * mm->n_bands);
> +
> +    new_meter->ofproto  = ofproto;
> +    new_meter->created  = old_meter->created;
> +    new_meter->modified = time_msec();
> +
> +    error = ofproto->ofproto_class->meter_update(new_meter);
> +    if (error) {
> +        free(new_meter);
> +        return error;
> +    }
> +
> +    old_meter->modified = new_meter->modified;
> +    old_meter->flags    = new_meter->flags;
> +    old_meter->n_bands  = new_meter->n_bands;
> +    memcpy(old_meter->bands, new_meter->bands, sizeof(new_meter->bands));
> +    free(new_meter);
> +
> +    return 0;
> +}
> +
> +static enum ofperr
> +delete_meter(struct ofproto *ofproto, struct ofconn *ofconn,
> +                   struct ofputil_meter_mod *mm,
> +                   const struct ofp_header *request)
> +{
> +    struct ofmeter* meter = NULL;
> +    int error;
> +    struct ihash_node *ihash_node = NULL;
> +
> +    if (mm->meter_id == OFPM13_ALL) {
> +        struct ihash_node *node, *next;
> +
> +        IHASH_FOR_EACH_SAFE (node, next, &ofproto->meters) {
> +            meter = (struct ofmeter*) node->data;
> +            mm->meter_id = meter->meter_id;
> +            delete_meter(ofproto, ofconn, mm, request);
> +        }
> +
> +        return 0;
> +    }
> +
> +    if (mm->meter_id < 1 || mm->meter_id > OFPM13_MAX) {
> +        VLOG_ERR("Fail to modify meter, invalid meter_id %"PRIu32"\n", mm->meter_id);
> +        return OFPERR_OFPMMFC_INVALID_METER;
> +    }
> +
> +    ihash_node = ihash_find(&ofproto->meters, mm->meter_id);
> +
> +    if (!ihash_node) {
> +        return 0;
> +    }
> +
> +    meter = (struct ofmeter*) ihash_node->data;
> +
> +    meter_del_remove_rules(ofproto, meter->meter_id);

The return value should not be ignored, as the operation may have to be postponed.

> +
> +    error = ofproto->ofproto_class->meter_destruct(meter);
> +    if (error) {
> +        return error;
> +    }
> +
> +    ihash_delete(&ofproto->meters, ihash_node);
> +    free(meter);
> +
> +    return 0;
> +}
> +
> +static enum ofperr
> +handle_meter_mod__(struct ofproto *ofproto, struct ofconn *ofconn,
> +                  const struct ofputil_meter_mod *mm,
> +                  const struct ofp_header *oh)
> +{
> +    switch (mm->command) {
> +    case OFPMC13_ADD:
> +        return add_meter(ofproto, ofconn, mm, oh);
> +
> +    case OFPMC13_MODIFY:
> +        return modify_meter(ofproto, ofconn, mm, oh);
> +
> +    case OFPMC13_DELETE:
> +    {
> +        struct ofputil_meter_mod omm;
> +        memset(&omm, 0, sizeof(omm));
> +        omm.meter_id = mm->meter_id;
> +        return delete_meter(ofproto, ofconn, &omm, oh);
> +    }
> +    default:
> +        return OFPERR_OFPMMFC_BAD_COMMAND;
> +    }
> +}
> +
> +static enum ofperr
> +handle_meter_mod(struct ofconn *ofconn, const struct ofp_header *oh)
> +{
> +    struct ofputil_meter_mod mm;
> +    enum ofperr error;
> +
> +    error = reject_slave_controller(ofconn);
> +    if (error) {
> +        return error;
> +    }
> +
> +    memset(&mm, 0, sizeof(mm));
> +    error = ofputil_decode_meter_mod(&mm, oh);
> +    if (error) {
> +        return error;
> +    }
> +
> +    return handle_meter_mod__(ofconn_get_ofproto(ofconn), ofconn, &mm, oh);
> +}
> +
> +static enum ofperr
> +handle_meter_request__(struct ofproto *ofproto, const struct ofp_header *request,
> +                          struct ofmeter* meter, struct list* replies)
> +{
> +    struct ofputil_meter_stats meter_stats;
> +    struct ofputil_meter_stats_reply ms;
> +    size_t band_i = 0;
> +    uint32_t refcnt = 0;
> +
> +    request = request; /* TODO unused parameter */
> +
> +    memset(&meter_stats, 0, sizeof(meter_stats));
> +    ofproto->ofproto_class->meter_get_stats(meter, &meter_stats);
> +    ofproto->ofproto_class->meter_get_refcnt(meter, &refcnt);
> +
> +    memset(&ms, 0, sizeof(ms));
> +    ms.meter_id   = meter->meter_id;
> +    ms.len        = sizeof(struct ofp13_meter_stats) + sizeof(struct ofp13_meter_band_stats) * meter->n_bands;
> +    ms.flow_count = refcnt;
> +    ms.packet_in_count = meter_stats.packet_in_count;
> +    ms.byte_in_count   = meter_stats.byte_in_count;
> +    ms.n_bands         = meter->n_bands;
> +    calc_duration(meter->created, time_msec(),
> +                  &ms.duration_sec, &ms.duration_nsec);
> +    ofputil_append_meter_stats_reply(&ms, replies);
> +
> +    for (band_i = 0; band_i < meter->n_bands; band_i++){
> +        struct ofputil_meter_band_stats_reply mbs;
> +
> +        mbs.packet_band_count = ntohll(meter_stats.bands_stats[band_i].packet_band_count);
> +        mbs.byte_band_count   = ntohll(meter_stats.bands_stats[band_i].byte_band_count);
> +

These conversions are inverted in ofputil_append_meter_band_stats_reply() below...

> +        ofputil_append_meter_band_stats_reply(&mbs, replies);
> +    }
> +
> +    return 0;
> +}
> +
> +static enum ofperr
> +handle_meter_request(struct ofconn *ofconn, const struct ofp_header *request)
> +{
> +    struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
> +    struct ofputil_meter_request msr;
> +    struct list replies;
> +    struct ihash_node *ihash_node = NULL;
> +    struct ofmeter* meter = NULL;
> +    enum ofp_version version;
> +    enum ofperr error;
> +    enum ofpraw raw;
> +    uint32_t max_meter;
> +    uint32_t min_meter;
> +    uint8_t max_bands;
> +
> +    version = ofputil_protocol_to_ofp_version(ofconn_get_protocol(ofconn));
> +    switch (version) {
> +    case OFP13_VERSION:
> +        raw = OFPRAW_OFPST13_METER_REPLY;
> +        break;
> +    case OFP10_VERSION:
> +    case OFP11_VERSION:
> +    case OFP12_VERSION:
> +    default:
> +        NOT_REACHED();
> +    }
> +
> +    error = ofputil_decode_meter_request(&msr, request);
> +    if (error) {
> +        return error;
> +    }
> +    ofproto->ofproto_class->meter_get_features(ofproto, &max_meter, &min_meter, &max_bands);
> +
> +    ofpmp_init(&replies, request);
> +    if (0 < msr.meter_id && msr.meter_id <= OFPM13_MAX) {
> +        ihash_node = ihash_find(&ofproto->meters, msr.meter_id);
> +
> +        if (ihash_node) {
> +            meter = (struct ofmeter*) ihash_node->data;
> +            handle_meter_request__(ofproto, request, meter, &replies);
> +        }
> +
> +    } else if (msr.meter_id == OFPM13_ALL){
> +        uint32_t meter_id;
> +
> +        for (meter_id = 1; meter_id <= max_meter; meter_id++) {
> +            ihash_node = ihash_find(&ofproto->meters, meter_id);
> +
> +            if (!ihash_node) {
> +                continue;
> +            }

Taken the assumption here that the datapath implementation limits the max_meter to some sensible number, I feel that we could maintain a simple index table of meters instead of an integer hash table.

> +            meter = (struct ofmeter*) ihash_node->data;
> +            handle_meter_request__(ofproto, request, meter, &replies);
> +        }
> +    }
> +
> +    ofconn_send_replies(ofconn, &replies);
> +
> +    return 0;
> +}
> +
> +static enum ofperr
> +handle_meter_config_request__(struct ofproto *ofproto, const struct ofp_header *request,
> +                          struct ofmeter* meter, struct list* replies)
> +{
> +    struct ofputil_meter_config_reply mcr;
> +    size_t band_i = 0;
> +
> +    ofproto = ofproto; /* TODO unused parameter */
> +    request = request; /* TODO unused parameter */
> +
> +    mcr.flags    = meter->flags;
> +    mcr.meter_id = meter->meter_id;
> +    mcr.length   = sizeof(struct ofp13_meter_config) + meter->n_bands * sizeof(struct ofp13_meter_band_drop);
> +    mcr.n_bands  = meter->n_bands;
> +    mcr.bands    = meter->bands;
> +
> +    ofputil_append_meter_config_reply(&mcr, replies);
> +
> +    for (band_i = 0; band_i < meter->n_bands; band_i++){
> +        struct ofputil_meter_band_config_reply mbc;
> +        mbc.type       = ntohs(meter->bands[band_i].type);
> +        mbc.len        = sizeof(struct ofp13_meter_band_drop);
> +        mbc.rate       = ntohl(meter->bands[band_i].rate);
> +        mbc.burst_size = ntohl(meter->bands[band_i].burst_size);
> +
> +        ofputil_append_meter_band_config_reply(&mbc, replies);
> +    }
> +
> +    return 0;
> +}
> +
> +static enum ofperr
> +handle_meter_config_request(struct ofconn *ofconn, const struct ofp_header *request)
> +{
> +    struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
> +    struct ofputil_meter_request msr;
> +    enum ofp_version version;
> +    struct list replies;
> +    struct ihash_node *ihash_node = NULL;
> +    struct ofmeter* meter = NULL;
> +    enum ofperr error;
> +    enum ofpraw raw;
> +    uint32_t max_meter;
> +    uint32_t min_meter;
> +    uint8_t max_bands;
> +
> +    version = ofputil_protocol_to_ofp_version(ofconn_get_protocol(ofconn));
> +    switch (version) {
> +    case OFP13_VERSION:
> +        raw = OFPRAW_OFPST13_METER_CONFIG_REPLY;
> +        break;
> +    case OFP10_VERSION:
> +    case OFP11_VERSION:
> +    case OFP12_VERSION:
> +    default:
> +        NOT_REACHED();
> +    }
> +
> +    error = ofputil_decode_meter_request(&msr, request);
> +    if (error) {
> +        return error;
> +    }
> +    ofproto->ofproto_class->meter_get_features(ofproto, &max_meter, &min_meter, &max_bands);
> +
> +    ofpmp_init(&replies, request);
> +    if (0 < msr.meter_id && msr.meter_id < OFPM13_MAX) {
> +        ihash_node = ihash_find(&ofproto->meters, msr.meter_id);
> +
> +        if (ihash_node) {
> +            meter = (struct ofmeter*) ihash_node->data;
> +
> +            handle_meter_config_request__(ofproto, request, meter, &replies);
> +        }
> +    } else if (msr.meter_id == OFPM13_ALL) {
> +        uint32_t meter_id;
> +        for (meter_id = min_meter; meter_id <= max_meter; meter_id++) {
> +            ihash_node = ihash_find(&ofproto->meters, meter_id);
> +
> +            if (!ihash_node) {
> +                continue;
> +            }
> +
> +            meter = (struct ofmeter*) ihash_node->data;
> +            handle_meter_config_request__(ofproto, request, meter, &replies);
> +        }
> +    }
> +
> +    ofconn_send_replies(ofconn, &replies);
> +
> +    return 0;
> +}
> +
> +static enum ofperr
> +handle_meter_features_request(struct ofconn *ofconn, const struct ofp_header *oh)
> +{
> +    struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
> +    struct ofpbuf *b;
> +    struct ofp13_meter_features *omf;
> +    enum ofp_version version;
> +    uint32_t max_meter;
> +    uint32_t min_meter;
> +    uint8_t max_bands;
> +    enum ofpraw raw;
> +
> +    version = ofputil_protocol_to_ofp_version(ofconn_get_protocol(ofconn));
> +    switch (version) {
> +    case OFP13_VERSION:
> +        raw = OFPRAW_OFPST13_METER_FEATURES_REPLY;
> +        break;
> +    case OFP10_VERSION:
> +    case OFP11_VERSION:
> +    case OFP12_VERSION:
> +    default:
> +        NOT_REACHED();
> +    }
> +
> +    ofproto->ofproto_class->meter_get_features(ofproto, &max_meter, &min_meter, &max_bands);
> +    b = ofpraw_alloc_xid(raw, version, oh->xid, 0);
> +    omf = ofpbuf_put_zeros(b, sizeof *omf);
> +    omf->max_meter    = htonl(max_meter);
> +    omf->band_types   = htonl(OFPMBT13_DROP);

This should be a set of bits, but I'm not completely sure if this should be htonl(1 << OFPMBT13_DROP) or htonl(1 << (OFPMBT13_DROP - 1)), as band types start at 1.

> +    omf->capabilities = htonl(OFPMF13_KBPS | OFPMF13_BURST | OFPMF13_STATS);
> +    omf->max_bands    = max_bands;
> +    omf->max_color    = 0;
> +

Optimally, it should be the underlying datapath that determines what band types and capabilities/flags are supported. This would be important when userspace supports more features than the current kernel datapath, for example.

For some truth in advertising, max_bands (OF: "Maximum bands per meters") should be limited to what is supported at the ofproto level (currently 1).

> +    ofconn_send_reply(ofconn, b);
> +    return 0;
> +}
> +
> static enum ofperr
> handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg)
> {
> @@ -4199,16 +4721,25 @@ handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg)
>     case OFPTYPE_FLOW_MONITOR_STATS_REQUEST:
>         return handle_flow_monitor_request(ofconn, oh);
> 
> +        /* Meters. */
> +    case OFPTYPE_METER_MOD:
> +        return handle_meter_mod(ofconn, oh);
> +
> +    case OFPTYPE_METER_REQUEST:
> +        return handle_meter_request(ofconn, oh);
> +
> +    case OFPTYPE_METER_CONFIG_REQUEST:
> +        return handle_meter_config_request(ofconn, oh);
> +
> +    case OFPTYPE_METER_FEATURES_REQUEST:
> +        return handle_meter_features_request(ofconn, oh);
> +
>         /* FIXME: Change the following once they are implemented: */
>     case OFPTYPE_QUEUE_GET_CONFIG_REQUEST:
>     case OFPTYPE_GET_ASYNC_REQUEST:
> -    case OFPTYPE_METER_MOD:
>     case OFPTYPE_GROUP_REQUEST:
>     case OFPTYPE_GROUP_DESC_REQUEST:
>     case OFPTYPE_GROUP_FEATURES_REQUEST:
> -    case OFPTYPE_METER_REQUEST:
> -    case OFPTYPE_METER_CONFIG_REQUEST:
> -    case OFPTYPE_METER_FEATURES_REQUEST:
>     case OFPTYPE_TABLE_FEATURES_REQUEST:
>         return OFPERR_OFPBRC_BAD_TYPE;
...

EOF




More information about the dev mailing list