On 11/10/21 17:21, Simon Horman wrote: > From: Louis Peens <[email protected]> > > 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 <[email protected]> > Signed-off-by: Tianyu Yuan <[email protected]> > Signed-off-by: Simon Horman <[email protected]> > --- > 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
