[ovs-dev] [PATCH v2 1/2] ofp-actions: Support OF1.5 meter action.
Numan Siddique
nusiddiq at redhat.com
Mon Jun 17 17:02:41 UTC 2019
On Mon, Jun 10, 2019 at 4:55 AM Ben Pfaff <blp at ovn.org> wrote:
> OpenFlow 1.5 changed "meter" from an instruction to an action. This commit
> supports it properly.
>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
>
Acked-by: Numan Siddique <nusiddiq at redhat.com>
Does this comment here needs any update -
https://github.com/openvswitch/ovs/blob/master/include/openvswitch/ofp-actions.h#L136
?
Thanks
Numan
> ---
> Documentation/topics/openflow.rst | 6 --
> NEWS | 1 +
> include/openvswitch/ofp-actions.h | 4 +-
> lib/ofp-actions.c | 117 +++++++++++++++++++++---------
> lib/ovs-actions.xml | 11 ++-
> tests/ofp-actions.at | 3 +
> 6 files changed, 100 insertions(+), 42 deletions(-)
>
> diff --git a/Documentation/topics/openflow.rst
> b/Documentation/topics/openflow.rst
> index b6a5c6b54920..b4e49be916be 100644
> --- a/Documentation/topics/openflow.rst
> +++ b/Documentation/topics/openflow.rst
> @@ -242,12 +242,6 @@ features are listed in the previous section.
>
> (optional for OF1.5+)
>
> -* Meter action
> -
> - (EXT-379)
> -
> - (required for OF1.5+ if metering is supported)
> -
> * Port properties for pipeline fields
>
> Prototype for OVS was done during specification.
> diff --git a/NEWS b/NEWS
> index 19cebf89a785..6f91a4d6950e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -8,6 +8,7 @@ Post-v2.11.0
> - OpenFlow:
> * Removed support for OpenFlow 1.6 (draft), which ONF abandoned.
> * New action "check_pkt_larger".
> + * Support for OpenFlow 1.5 "meter" action.
> - Userspace datapath:
> * ICMPv6 ND enhancements: support for match and set ND options type
> and reserved fields.
> diff --git a/include/openvswitch/ofp-actions.h
> b/include/openvswitch/ofp-actions.h
> index 436c4aadf548..792b2679d3a8 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2012, 2013, 2014, 2015, 2016, 2017 Nicira, Inc.
> + * Copyright (c) 2012, 2013, 2014, 2015, 2016, 2017, 2019 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -1360,7 +1360,7 @@ enum {
> const char *ovs_instruction_name_from_type(enum ovs_instruction_type
> type);
> int ovs_instruction_type_from_name(const char *name);
> enum ovs_instruction_type ovs_instruction_type_from_ofpact_type(
> - enum ofpact_type);
> + enum ofpact_type, enum ofp_version);
> enum ofperr ovs_instruction_type_from_inst_type(
> enum ovs_instruction_type *instruction_type, const uint16_t
> inst_type);
>
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index cabd5a05e1f4..ddef3b0c8780 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -259,6 +259,9 @@ enum ofp_raw_action_type {
> /* NX1.0-1.4(6): struct nx_action_reg_move, ... VLMFF */
> NXAST_RAW_REG_MOVE,
>
> + /* OF1.5+(29): uint32_t. */
> + OFPAT_RAW15_METER,
> +
> /* ## ------------------------- ## */
> /* ## Nicira extension actions. ## */
> /* ## ------------------------- ## */
> @@ -407,7 +410,7 @@ static void ofpacts_update_instruction_actions(struct
> ofpbuf *openflow,
> static void pad_ofpat(struct ofpbuf *openflow, size_t start_ofs);
>
> static enum ofperr ofpacts_verify(const struct ofpact[], size_t
> ofpacts_len,
> - uint32_t allowed_ovsinsts,
> + enum ofp_version, uint32_t
> allowed_ovsinsts,
> enum ofpact_type outer_action,
> char **errorp);
>
> @@ -5957,7 +5960,7 @@ parse_CLONE(char *arg, const struct
> ofpact_parse_params *pp)
> char *error;
>
> ofpbuf_pull(pp->ofpacts, sizeof *clone);
> - error = ofpacts_parse_copy(arg, pp, false, 0);
> + error = ofpacts_parse_copy(arg, pp, false, OFPACT_CLONE);
> /* header points to the action list */
> pp->ofpacts->header = ofpbuf_push_uninit(pp->ofpacts, sizeof *clone);
> clone = pp->ofpacts->header;
> @@ -7216,14 +7219,32 @@ check_OUTPUT_TRUNC(const struct
> ofpact_output_trunc *a,
> return ofpact_check_output_port(a->port, cp->max_ports);
> }
>
> -/* Meter instruction. */
> +/* Meter.
> + *
> + * In OpenFlow 1.3 and 1.4, "meter" is an instruction.
> + * In OpenFlow 1.5 and later, "meter" is an action.
> + *
> + * OpenFlow 1.5 */
> +
> +static enum ofperr
> +decode_OFPAT_RAW15_METER(uint32_t meter_id,
> + enum ofp_version ofp_version OVS_UNUSED,
> + struct ofpbuf *out)
> +{
> + struct ofpact_meter *om = ofpact_put_METER(out);
> + om->meter_id = meter_id;
> + om->provider_meter_id = UINT32_MAX; /* No provider meter ID. */
> + return 0;
> +}
>
> static void
> encode_METER(const struct ofpact_meter *meter,
> enum ofp_version ofp_version, struct ofpbuf *out)
> {
> - if (ofp_version >= OFP13_VERSION) {
> + if (ofp_version == OFP13_VERSION || ofp_version == OFP14_VERSION) {
> instruction_put_OFPIT13_METER(out)->meter_id =
> htonl(meter->meter_id);
> + } else if (ofp_version >= OFP15_VERSION) {
> + put_OFPAT15_METER(out, meter->meter_id);
> }
> }
>
> @@ -7683,8 +7704,8 @@ ofpacts_pull_openflow_actions__(struct ofpbuf
> *openflow,
> error = ofpacts_decode(actions, actions_len, version, vl_mff_map,
> ofpacts_tlv_bitmap, ofpacts);
> if (!error) {
> - error = ofpacts_verify(ofpacts->data, ofpacts->size,
> allowed_ovsinsts,
> - outer_action, NULL);
> + error = ofpacts_verify(ofpacts->data, ofpacts->size, version,
> + allowed_ovsinsts, outer_action, NULL);
> }
> if (error) {
> ofpbuf_clear(ofpacts);
> @@ -7724,10 +7745,10 @@ ofpacts_pull_openflow_actions(struct ofpbuf
> *openflow,
> uint64_t *ofpacts_tlv_bitmap,
> struct ofpbuf *ofpacts)
> {
> - return ofpacts_pull_openflow_actions__(openflow, actions_len, version,
> - 1u <<
> OVSINST_OFPIT11_APPLY_ACTIONS,
> - ofpacts, 0, vl_mff_map,
> - ofpacts_tlv_bitmap);
> + return ofpacts_pull_openflow_actions__(
> + openflow, actions_len, version,
> + (1u << OVSINST_OFPIT11_APPLY_ACTIONS) | (1u <<
> OVSINST_OFPIT13_METER),
> + ofpacts, 0, vl_mff_map, ofpacts_tlv_bitmap);
> }
>
> /* OpenFlow 1.1 action sets. */
> @@ -7981,11 +8002,14 @@ ovs_instruction_type_from_name(const char *name)
> }
>
> enum ovs_instruction_type
> -ovs_instruction_type_from_ofpact_type(enum ofpact_type type)
> +ovs_instruction_type_from_ofpact_type(enum ofpact_type type,
> + enum ofp_version version)
> {
> switch (type) {
> case OFPACT_METER:
> - return OVSINST_OFPIT13_METER;
> + return (version >= OFP15_VERSION
> + ? OVSINST_OFPIT11_APPLY_ACTIONS
> + : OVSINST_OFPIT13_METER);
> case OFPACT_CLEAR_ACTIONS:
> return OVSINST_OFPIT11_CLEAR_ACTIONS;
> case OFPACT_WRITE_ACTIONS:
> @@ -8080,7 +8104,7 @@ struct ovsinst_map {
> static const struct ovsinst_map *
> get_ovsinst_map(enum ofp_version version)
> {
> - /* OpenFlow 1.1 and 1.2 instructions. */
> + /* OpenFlow 1.1, 1.2, and 1.5 instructions. */
> static const struct ovsinst_map of11[] = {
> { OVSINST_OFPIT11_GOTO_TABLE, 1 },
> { OVSINST_OFPIT11_WRITE_METADATA, 2 },
> @@ -8090,7 +8114,7 @@ get_ovsinst_map(enum ofp_version version)
> { 0, -1 },
> };
>
> - /* OpenFlow 1.3+ instructions. */
> + /* OpenFlow 1.3 and 1.4 instructions. */
> static const struct ovsinst_map of13[] = {
> { OVSINST_OFPIT11_GOTO_TABLE, 1 },
> { OVSINST_OFPIT11_WRITE_METADATA, 2 },
> @@ -8101,7 +8125,7 @@ get_ovsinst_map(enum ofp_version version)
> { 0, -1 },
> };
>
> - return version < OFP13_VERSION ? of11 : of13;
> + return version == OFP13_VERSION || version == OFP14_VERSION ? of13 :
> of11;
> }
>
> /* Converts 'ovsinst_bitmap', a bitmap whose bits correspond to OVSINST_*
> @@ -8193,7 +8217,7 @@ OVS_INSTRUCTIONS
>
> static enum ofperr
> decode_openflow11_instructions(const struct ofp11_instruction insts[],
> - size_t n_insts,
> + size_t n_insts, enum ofp_version version,
> const struct ofp11_instruction *out[])
> {
> const struct ofp11_instruction *inst;
> @@ -8209,6 +8233,11 @@ decode_openflow11_instructions(const struct
> ofp11_instruction insts[],
> return error;
> }
>
> + if (type == OVSINST_OFPIT13_METER && version >= OFP15_VERSION) {
> + /* "meter" is an action, not an instruction, in OpenFlow 1.5.
> */
> + return OFPERR_OFPBIC_UNKNOWN_INST;
> + }
> +
> if (out[type]) {
> return OFPERR_OFPBIC_DUP_INST;
> }
> @@ -8271,7 +8300,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
> *openflow,
> }
>
> error = decode_openflow11_instructions(
> - instructions, instructions_len / OFP11_INSTRUCTION_ALIGN,
> + instructions, instructions_len / OFP11_INSTRUCTION_ALIGN, version,
> insts);
> if (error) {
> goto exit;
> @@ -8344,7 +8373,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
> *openflow,
> ogt->table_id = oigt->table_id;
> }
>
> - error = ofpacts_verify(ofpacts->data, ofpacts->size,
> + error = ofpacts_verify(ofpacts->data, ofpacts->size, version,
> (1u << N_OVS_INSTRUCTIONS) - 1, 0, NULL);
> exit:
> if (error) {
> @@ -8559,7 +8588,8 @@ ofpacts_verify_nested(const struct ofpact *a, enum
> ofpact_type outer_action,
>
> if (outer_action) {
> ovs_assert(outer_action == OFPACT_WRITE_ACTIONS
> - || outer_action == OFPACT_CT);
> + || outer_action == OFPACT_CT
> + || outer_action == OFPACT_CLONE);
>
> if (outer_action == OFPACT_CT) {
> if (!field) {
> @@ -8571,6 +8601,10 @@ ofpacts_verify_nested(const struct ofpact *a, enum
> ofpact_type outer_action,
> return OFPERR_OFPBAC_BAD_ARGUMENT;
> }
> }
> +
> + if (a->type == OFPACT_METER) {
> + return unsupported_nesting(a->type, outer_action, errorp);
> + }
> }
>
> return 0;
> @@ -8580,6 +8614,10 @@ ofpacts_verify_nested(const struct ofpact *a, enum
> ofpact_type outer_action,
> * appropriate order as defined by the OpenFlow spec and as required by
> Open
> * vSwitch.
> *
> + * The 'version' is relevant only for error reporting: Open vSwitch
> enforces
> + * the same rules for every version of OpenFlow, but different versions
> require
> + * different error codes.
> + *
> * 'allowed_ovsinsts' is a bitmap of OVSINST_* values, in which 1-bits
> indicate
> * instructions that are allowed within 'ofpacts[]'.
> *
> @@ -8587,8 +8625,8 @@ ofpacts_verify_nested(const struct ofpact *a, enum
> ofpact_type outer_action,
> * within another action of type 'outer_action'. */
> static enum ofperr
> ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len,
> - uint32_t allowed_ovsinsts, enum ofpact_type outer_action,
> - char **errorp)
> + enum ofp_version version, uint32_t allowed_ovsinsts,
> + enum ofpact_type outer_action, char **errorp)
> {
> const struct ofpact *a;
> enum ovs_instruction_type inst;
> @@ -8616,7 +8654,7 @@ ofpacts_verify(const struct ofpact ofpacts[], size_t
> ofpacts_len,
> return error;
> }
>
> - next = ovs_instruction_type_from_ofpact_type(a->type);
> + next = ovs_instruction_type_from_ofpact_type(a->type, version);
> if (a > ofpacts
> && (inst == OVSINST_OFPIT11_APPLY_ACTIONS
> ? next < inst
> @@ -8638,8 +8676,13 @@ ofpacts_verify(const struct ofpact ofpacts[],
> size_t ofpacts_len,
> if (!((1u << next) & allowed_ovsinsts)) {
> const char *name = ovs_instruction_name_from_type(next);
>
> - verify_error(errorp, "%s instruction not allowed here", name);
> - return OFPERR_OFPBIC_UNSUP_INST;
> + if (next == OVSINST_OFPIT13_METER && version >=
> OFP15_VERSION) {
> + verify_error(errorp, "%s action not allowed here", name);
> + return OFPERR_OFPBAC_BAD_TYPE;
> + } else {
> + verify_error(errorp, "%s instruction not allowed here",
> name);
> + return OFPERR_OFPBIC_UNSUP_INST;
> + }
> }
>
> inst = next;
> @@ -8684,9 +8727,9 @@ ofpacts_put_openflow_actions(const struct ofpact
> ofpacts[], size_t ofpacts_len,
> }
>
> static enum ovs_instruction_type
> -ofpact_is_apply_actions(const struct ofpact *a)
> +ofpact_is_apply_actions(const struct ofpact *a, enum ofp_version version)
> {
> - return (ovs_instruction_type_from_ofpact_type(a->type)
> + return (ovs_instruction_type_from_ofpact_type(a->type, version)
> == OVSINST_OFPIT11_APPLY_ACTIONS);
> }
>
> @@ -8707,14 +8750,14 @@ ofpacts_put_openflow_instructions(const struct
> ofpact ofpacts[],
>
> a = ofpacts;
> while (a < end) {
> - if (ofpact_is_apply_actions(a)) {
> + if (ofpact_is_apply_actions(a, ofp_version)) {
> size_t ofs = openflow->size;
>
> instruction_put_OFPIT11_APPLY_ACTIONS(openflow);
> do {
> encode_ofpact(a, ofp_version, openflow);
> a = ofpact_next(a);
> - } while (a < end && ofpact_is_apply_actions(a));
> + } while (a < end && ofpact_is_apply_actions(a, ofp_version));
> ofpacts_update_instruction_actions(openflow, ofs);
> } else {
> encode_ofpact(a, ofp_version, openflow);
> @@ -9023,12 +9066,13 @@ ofpacts_get_meter(const struct ofpact ofpacts[],
> size_t ofpacts_len)
> const struct ofpact *a;
>
> OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
> - enum ovs_instruction_type inst;
> -
> - inst = ovs_instruction_type_from_ofpact_type(a->type);
> if (a->type == OFPACT_METER) {
> return ofpact_get_METER(a)->meter_id;
> - } else if (inst > OVSINST_OFPIT13_METER) {
> + }
> +
> + enum ovs_instruction_type inst
> + = ovs_instruction_type_from_ofpact_type(a->type, 0);
> + if (inst > OVSINST_OFPIT13_METER) {
> break;
> }
> }
> @@ -9174,6 +9218,13 @@ ofpacts_parse__(char *str, const struct
> ofpact_parse_params *pp,
> ofp_port_t port;
> if (ofpact_type_from_name(key, &type)) {
> error = ofpact_parse(type, value, pp);
> +
> + if (type == OFPACT_METER && !allow_instructions) {
> + /* Meter is an action in OF1.5 and it's being used in a
> + * context where instructions aren't allowed. Therefore,
> + * this must be OF1.5+. */
> + *pp->usable_protocols &= OFPUTIL_P_OF15_UP;
> + }
> } else if (!strcasecmp(key, "mod_vlan_vid")) {
> error = parse_set_vlan_vid(value, true, pp);
> } else if (!strcasecmp(key, "mod_vlan_pcp")) {
> @@ -9208,7 +9259,7 @@ ofpacts_parse__(char *str, const struct
> ofpact_parse_params *pp,
> }
>
> char *error = NULL;
> - ofpacts_verify(pp->ofpacts->data, pp->ofpacts->size,
> + ofpacts_verify(pp->ofpacts->data, pp->ofpacts->size, OFP11_VERSION,
> (allow_instructions
> ? (1u << N_OVS_INSTRUCTIONS) - 1
> : ((1u << OVSINST_OFPIT11_APPLY_ACTIONS)
> diff --git a/lib/ovs-actions.xml b/lib/ovs-actions.xml
> index ab6c0030aa50..e52cd849e37b 100644
> --- a/lib/ovs-actions.xml
> +++ b/lib/ovs-actions.xml
> @@ -2832,11 +2832,20 @@ while <var>link</var> > <var>max_link</var>
> 1.5 changes <code>meter</code> from an instruction to an action.
> </p>
>
> + <p>
> + OpenFlow 1.5 allows implementations to restrict
> <code>meter</code> to
> + be the first action in an action list and to exclude
> + <code>meter</code> from action sets, for better compatibility
> with
> + OpenFlow 1.3 and 1.4. Open vSwitch restricts the
> <code>meter</code>
> + action both ways.
> + </p>
> +
> <p>
> Open vSwitch 2.0 introduced OpenFlow protocol support for
> meters, but
> it did not include a datapath implementation. Open vSwitch 2.7
> added
> meter support to the userspace datapath. Open vSwitch 2.10
> added
> - meter support to the kernel datapath.
> + meter support to the kernel datapath. Open vSwitch 2.12 added
> + support for meter as an action in OpenFlow 1.5.
> </p>
> </conformance>
> </action>
> diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
> index f944369f4086..4893280a998f 100644
> --- a/tests/ofp-actions.at
> +++ b/tests/ofp-actions.at
> @@ -792,6 +792,9 @@ AT_DATA([test-data], [dnl
> # actions=set_field:00:00:00:00:12:34/00:00:00:00:ff:ff->eth_src
> 0019 0018 8000090c 000000001234 00000000ffff 00000000
>
> +# actions=meter:5
> +001d 0008 00000005
> +
> ])
> sed '/^[[#&]]/d' < test-data > input.txt
> sed -n 's/^# //p; /^$/p' < test-data > expout
> --
> 2.20.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list