| Message ID | 20211110162134.19132-1-simon.horman@corigine.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif-xlate: support meter action in a group bucket | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
References: <20211110162134.19132-1-simon.horman@corigine.com> Bleep bloop. Greetings Simon Horman, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Tianyu Yuan <tianyu.yuan@corigine.com>, Simon Horman <simon.horman@corigine.com> Lines checked: 143, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 11/10/21 17:21, Simon Horman wrote: > From: Louis Peens <louis.peens@corigine.com> > > This patch is used to make a group bucket support a meter action. > When receiving action=meter:<id> we need to: > 1) accept this - pre-patch effectively only set_actions are supported > 2) use this id to lookup the meter that was added > 3) populate the provider id into the ofpact_meter id. > > Signed-off-by: Louis Peens <louis.peens@corigine.com> > Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> > Signed-off-by: Simon Horman <simon.horman@corigine.com> > --- > lib/ofp-actions.c | 2 +- > ofproto/ofproto-dpif-xlate.c | 15 +++++++++++++++ > ofproto/ofproto.c | 21 +-------------------- > ofproto/ofproto.h | 23 +++++++++++++++++++++++ > 4 files changed, 40 insertions(+), 21 deletions(-) > > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c > index ecf914eac..9b9968eb4 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -7879,6 +7879,7 @@ ofpact_copy(struct ofpbuf *out, const struct ofpact *a) > /* The order in which actions in an action set get executed. This is only for > * the actions where only the last instance added is used. */ > #define ACTION_SET_ORDER \ > + SLOT(OFPACT_METER) \ > SLOT(OFPACT_STRIP_VLAN) \ > SLOT(OFPACT_POP_MPLS) \ > SLOT(OFPACT_DECAP) \ Hi, folks. Sorry for the very late reply. I didn't review other parts of the patch, but the change above doesn't seem correct. OpenFlow specification requires a very specific order in which actions in the action set has to be executed. And 'meter' action falls into 'qos' section, which is 9th in the priority list. The change above makes it the highest priority instead. Also, documentation like ovs-actions page should be updated. One more thing is that meter action is only available in OF15, but not in earlier versions. ovs-actions page even documents restrictions OVS applies to meter actions in sets and lists: "OpenFlow 1.5 allows implementations to restrict meter to be the first action in an action list and to exclude meter from action sets, for better compatibility with OpenFlow 1.3 and 1.4. Open vSwitch restricts the meter action both ways." So, there needs to be a proper validation of the OpenFlow version being used while processing actions in the set. And we need unit tests for this new functionality, including negative tests with earlier OF versions. Best regards, Ilya Maximets.
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index ecf914eac..9b9968eb4 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -7879,6 +7879,7 @@ ofpact_copy(struct ofpbuf *out, const struct ofpact *a) /* The order in which actions in an action set get executed. This is only for * the actions where only the last instance added is used. */ #define ACTION_SET_ORDER \ + SLOT(OFPACT_METER) \ SLOT(OFPACT_STRIP_VLAN) \ SLOT(OFPACT_POP_MPLS) \ SLOT(OFPACT_DECAP) \ @@ -7970,7 +7971,6 @@ action_set_classify(const struct ofpact *a) case OFPACT_GOTO_TABLE: case OFPACT_LEARN: case OFPACT_CONJUNCTION: - case OFPACT_METER: case OFPACT_MULTIPATH: case OFPACT_NOTE: case OFPACT_OUTPUT_REG: diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 9d336bc6a..ff397b148 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4497,6 +4497,21 @@ xlate_group_bucket(struct xlate_ctx *ctx, struct ofputil_bucket *bucket, bool old_was_mpls = ctx->was_mpls; ofpacts_execute_action_set(&action_list, &action_set); + + const struct ofpact *cursor; + OFPACT_FOR_EACH (cursor, action_list.data, action_list.size) { + if (cursor->type == OFPACT_METER) { + struct ofpact_meter *meter = ofpact_get_METER(cursor); + struct ofproto *ofproto; + struct meter *ofmeter; + ofproto = &ctx->xbridge->ofproto->up; + ofmeter = ofproto_get_meter(ofproto, meter->meter_id); + if (ofmeter) { + meter->provider_meter_id = ofmeter->provider_meter_id.uint32; + } + } + } + ctx->depth++; do_xlate_actions(action_list.data, action_list.size, ctx, is_last_action, true); diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index bd6103b1c..1dad53ae8 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -6570,26 +6570,7 @@ handle_flow_monitor_cancel(struct ofconn *ofconn, const struct ofp_header *oh) return error; } -/* Meters implementation. - * - * Meter table entry, indexed by the OpenFlow meter_id. - * 'created' is used to compute the duration for meter stats. - * 'list rules' is needed so that we can delete the dependent rules when the - * meter table entry is deleted. - * 'provider_meter_id' is for the provider's private use. - */ -struct meter { - struct hmap_node node; /* In ofproto->meters. */ - long long int created; /* Time created. */ - struct ovs_list rules; /* List of "struct rule_dpif"s. */ - uint32_t id; /* OpenFlow meter_id. */ - ofproto_meter_id provider_meter_id; - uint16_t flags; /* Meter flags. */ - uint16_t n_bands; /* Number of meter bands. */ - struct ofputil_meter_band *bands; -}; - -static struct meter * +struct meter * ofproto_get_meter(const struct ofproto *ofproto, uint32_t meter_id) { struct meter *meter; diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h index b0262da2d..9a5038b38 100644 --- a/ofproto/ofproto.h +++ b/ofproto/ofproto.h @@ -26,6 +26,7 @@ #include "classifier.h" #include "flow.h" #include "openvswitch/meta-flow.h" +#include "openvswitch/ofp-meter.h" #include "netflow.h" #include "rstp.h" #include "smap.h" @@ -53,6 +54,28 @@ struct lldp_status; struct aa_settings; struct aa_mapping_settings; +/* Meters implementation. + * + * Meter table entry, indexed by the OpenFlow meter_id. + * 'created' is used to compute the duration for meter stats. + * 'list rules' is needed so that we can delete the dependent rules when the + * meter table entry is deleted. + * 'provider_meter_id' is for the provider's private use. + */ +struct meter { + struct hmap_node node; /* In ofproto->meters. */ + long long int created; /* Time created. */ + struct ovs_list rules; /* List of "struct rule_dpif"s. */ + uint32_t id; /* OpenFlow meter_id. */ + ofproto_meter_id provider_meter_id; + uint16_t flags; /* Meter flags. */ + uint16_t n_bands; /* Number of meter bands. */ + struct ofputil_meter_band *bands; +}; + +struct meter * +ofproto_get_meter(const struct ofproto *ofproto, uint32_t meter_id); + /* Needed for the lock annotations. */ extern struct ovs_mutex ofproto_mutex;