Message ID | 20240307172500.318917-1-aconole@redhat.com |
---|---|
State | Changes Requested |
Delegated to: | Ilya Maximets |
Headers | show |
Series | [ovs-dev] ofproto-dpif-xlate: Fix continuations with associated metering. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/intel-ovs-compilation | success | test: success |
On 3/7/24 18:25, Aaron Conole wrote: > Open vSwitch supports the ability to invoke a controller action by way > of a sample action with a specified meter. In the normal case, this > sample action is transparently generated during xlate processing. However, > when executing via a continuation, the logic to generate the sample > action when finishing the context freeze was missing. The result is that > the behavior when action is 'controller(pause,meter_id=1)' does not match > the behavior when action is 'controller(meter_id=1)'. > > OVN and other controller solutions may rely on this metering to protect > the control path, so it is critical to preserve metering, whether we are > doing a plain old send to controller, or a continuation. > > Fixes: 77ab5fd2a95b ("Implement serializing the state of packet traversal in "continuations".") > Reported-at: https://issues.redhat.com/browse/FDP-455 > Tested-by: Alex Musil <amusil@redhat.com> > Signed-off-by: Aaron Conole <aconole@redhat.com> > --- > ofproto/ofproto-dpif-xlate.c | 66 +++++++++++++++++++----------------- > tests/ofproto-dpif.at | 50 +++++++++++++++++++++++++++ > 2 files changed, 84 insertions(+), 32 deletions(-) Thanks, Aaron! The change seems corrct to me. See a couple of comments inline. Best regrads, Ilya Maximets. > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 89f183182e..4f1e57e6d1 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -5080,10 +5080,37 @@ put_controller_user_action(struct xlate_ctx *ctx, > bool dont_send, bool continuation, > uint32_t recirc_id, int len, > enum ofp_packet_in_reason reason, > + uint32_t provider_meter_id, > uint16_t controller_id) > { > struct user_action_cookie cookie; > > + /* If the controller action didn't request a meter (indicated by a > + * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was > + * configured through the "controller" virtual meter. > + * > + * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is > + * configured. */ > + uint32_t meter_id; > + if (provider_meter_id == UINT32_MAX) { > + meter_id = ctx->xbridge->ofproto->up.controller_meter_id; > + } else { > + meter_id = provider_meter_id; > + } > + > + size_t offset; > + size_t ac_offset; > + if (meter_id != UINT32_MAX) { > + /* If controller meter is configured, generate clone(meter, > + * userspace) action. */ Might be better to keep the comment the way it were originally. I find it harder to read the action split in two lines. > + offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE); > + nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, > + UINT32_MAX); > + ac_offset = nl_msg_start_nested(ctx->odp_actions, > + OVS_SAMPLE_ATTR_ACTIONS); > + nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id); > + } > + > memset(&cookie, 0, sizeof cookie); > cookie.type = USER_ACTION_COOKIE_CONTROLLER; > cookie.ofp_in_port = OFPP_NONE, > @@ -5101,6 +5128,11 @@ put_controller_user_action(struct xlate_ctx *ctx, > uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port); > odp_put_userspace_action(pid, &cookie, sizeof cookie, ODPP_NONE, > false, ctx->odp_actions, NULL); > + > + if (meter_id != UINT32_MAX) { > + nl_msg_end_nested(ctx->odp_actions, ac_offset); > + nl_msg_end_nested(ctx->odp_actions, offset); > + } > } > > static void > @@ -5145,32 +5177,6 @@ xlate_controller_action(struct xlate_ctx *ctx, int len, > } > recirc_refs_add(&ctx->xout->recircs, recirc_id); > > - /* If the controller action didn't request a meter (indicated by a > - * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was > - * configured through the "controller" virtual meter. > - * > - * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is > - * configured. */ > - uint32_t meter_id; > - if (provider_meter_id == UINT32_MAX) { > - meter_id = ctx->xbridge->ofproto->up.controller_meter_id; > - } else { > - meter_id = provider_meter_id; > - } > - > - size_t offset; > - size_t ac_offset; > - if (meter_id != UINT32_MAX) { > - /* If controller meter is configured, generate clone(meter, userspace) > - * action. */ > - offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE); > - nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, > - UINT32_MAX); > - ac_offset = nl_msg_start_nested(ctx->odp_actions, > - OVS_SAMPLE_ATTR_ACTIONS); > - nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id); > - } > - > /* Generate the datapath flows even if we don't send the packet-in > * so that debugging more closely represents normal state. */ > bool dont_send = false; > @@ -5178,12 +5184,7 @@ xlate_controller_action(struct xlate_ctx *ctx, int len, > dont_send = true; > } > put_controller_user_action(ctx, dont_send, false, recirc_id, len, > - reason, controller_id); > - > - if (meter_id != UINT32_MAX) { > - nl_msg_end_nested(ctx->odp_actions, ac_offset); > - nl_msg_end_nested(ctx->odp_actions, offset); > - } > + reason, provider_meter_id, controller_id); > } > > /* Creates a frozen state, and allocates a unique recirc id for the given > @@ -5235,6 +5236,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table) > put_controller_user_action(ctx, false, true, recirc_id, > ctx->pause->max_len, > ctx->pause->reason, > + ctx->pause->provider_meter_id, > ctx->pause->controller_id); > } else { > if (ctx->recirc_update_dp_hash) { > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index a1393f7f8e..db205a5316 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -6040,6 +6040,7 @@ m4_define([CHECK_CONTINUATION], [dnl > ]) > ]) > > + > # Check that pause at the end of the pipeline works OK. > # > # (xlate_continuation() has a special case for no-op actions; this > @@ -6160,6 +6161,7 @@ AT_CHECK([test 1 = `$PYTHON3 "$top_srcdir/utilities/ovs-pcap.in" p2-tx.pcap | wc > OVS_VSWITCHD_STOP > AT_CLEANUP > > + > AT_SETUP([ofproto-dpif - continuation with conntrack]) > AT_KEYWORDS([continuations pause resume]) > OVS_VSWITCHD_START Unrelated changes. > @@ -6195,6 +6197,54 @@ AT_CHECK([test 1 = `$PYTHON3 "$top_srcdir/utilities/ovs-pcap.in" p2-tx.pcap | wc > OVS_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([ofproto-dpif - continuation with meters]) > +AT_KEYWORDS([continuations pause meters]) > +OVS_VSWITCHD_START > +add_of_ports br0 1 2 > + > +dnl Add meter with id=1 A period at the end. > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps bands=type=drop rate=1']) > + > +AT_DATA([flows.txt], [dnl > +table=0 dl_dst=50:54:00:00:00:0a actions=goto_table(1) > +table=1 dl_dst=50:54:00:00:00:0a actions=controller(pause,meter_id=1) > +]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flows br0 flows.txt]) > + > +AT_CAPTURE_FILE([ofctl_monitor.log]) > +AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl -P nxt_packet_in --detach --no-chdir --pidfile 2> ofctl_monitor.log]) May better to wrap the line before the '--detach'. Modern autotools handle '\' just fine. And we need to register an on_exit hook to kill this daemon if something goes wrong. > + > +ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)' AT_CHECK. And maybe also wrap after the port. > + > +OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 2]) $() is preffered over ``. > +OVS_APP_EXIT_AND_WAIT([ovs-ofctl]) > +AT_CHECK([cat ofctl_monitor.log], [0], [dnl > +NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via action) data_len=14 (unbuffered) > +vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234 > +]) > + > +AT_CHECK([ovs-appctl revalidator/purge], [0]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-flows br0 | ofctl_strip | sort], [0], [dnl > + n_packets=1, n_bytes=14, dl_dst=50:54:00:00:00:0a actions=goto_table:1 > + table=1, n_packets=1, n_bytes=14, dl_dst=50:54:00:00:00:0a actions=controller(pause,meter_id=1) > +OFPST_FLOW reply (OF1.3): > +]) > + > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 | ofctl_strip | sort], [0], [dnl > +OFPST_METER_CONFIG reply (OF1.3): > +meter=1 pktps bands= > +type=drop rate=1 > +]) > + > +AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | strip_timers], [0], [dnl > +OFPST_METER reply (OF1.3) (xid=0x2): > +meter:1 flow_count:0 packet_in_count:1 byte_in_count:14 duration:0.0s bands: > +0: packet_count:0 byte_count:0 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([ofproto-dpif - continuation with patch port]) > AT_KEYWORDS([continuations pause resume]) > OVS_VSWITCHD_START(
Ilya Maximets <i.maximets@ovn.org> writes: > On 3/7/24 18:25, Aaron Conole wrote: >> Open vSwitch supports the ability to invoke a controller action by way >> of a sample action with a specified meter. In the normal case, this >> sample action is transparently generated during xlate processing. However, >> when executing via a continuation, the logic to generate the sample >> action when finishing the context freeze was missing. The result is that >> the behavior when action is 'controller(pause,meter_id=1)' does not match >> the behavior when action is 'controller(meter_id=1)'. >> >> OVN and other controller solutions may rely on this metering to protect >> the control path, so it is critical to preserve metering, whether we are >> doing a plain old send to controller, or a continuation. >> >> Fixes: 77ab5fd2a95b ("Implement serializing the state of packet traversal in "continuations".") >> Reported-at: https://issues.redhat.com/browse/FDP-455 >> Tested-by: Alex Musil <amusil@redhat.com> >> Signed-off-by: Aaron Conole <aconole@redhat.com> >> --- >> ofproto/ofproto-dpif-xlate.c | 66 +++++++++++++++++++----------------- >> tests/ofproto-dpif.at | 50 +++++++++++++++++++++++++++ >> 2 files changed, 84 insertions(+), 32 deletions(-) > > Thanks, Aaron! The change seems corrct to me. > See a couple of comments inline. > > Best regrads, Ilya Maximets. > >> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index 89f183182e..4f1e57e6d1 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -5080,10 +5080,37 @@ put_controller_user_action(struct xlate_ctx *ctx, >> bool dont_send, bool continuation, >> uint32_t recirc_id, int len, >> enum ofp_packet_in_reason reason, >> + uint32_t provider_meter_id, >> uint16_t controller_id) >> { >> struct user_action_cookie cookie; >> >> + /* If the controller action didn't request a meter (indicated by a >> + * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was >> + * configured through the "controller" virtual meter. >> + * >> + * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is >> + * configured. */ >> + uint32_t meter_id; >> + if (provider_meter_id == UINT32_MAX) { >> + meter_id = ctx->xbridge->ofproto->up.controller_meter_id; >> + } else { >> + meter_id = provider_meter_id; >> + } >> + >> + size_t offset; >> + size_t ac_offset; >> + if (meter_id != UINT32_MAX) { >> + /* If controller meter is configured, generate clone(meter, >> + * userspace) action. */ > > Might be better to keep the comment the way it were originally. > I find it harder to read the action split in two lines. Okay - I will put it back. I think I split it because of the line lengths, but maybe it would be better to put clone(meter,userspace) on the second line. >> + offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE); >> + nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, >> + UINT32_MAX); >> + ac_offset = nl_msg_start_nested(ctx->odp_actions, >> + OVS_SAMPLE_ATTR_ACTIONS); >> + nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id); >> + } >> + >> memset(&cookie, 0, sizeof cookie); >> cookie.type = USER_ACTION_COOKIE_CONTROLLER; >> cookie.ofp_in_port = OFPP_NONE, >> @@ -5101,6 +5128,11 @@ put_controller_user_action(struct xlate_ctx *ctx, >> uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port); >> odp_put_userspace_action(pid, &cookie, sizeof cookie, ODPP_NONE, >> false, ctx->odp_actions, NULL); >> + >> + if (meter_id != UINT32_MAX) { >> + nl_msg_end_nested(ctx->odp_actions, ac_offset); >> + nl_msg_end_nested(ctx->odp_actions, offset); >> + } >> } >> >> static void >> @@ -5145,32 +5177,6 @@ xlate_controller_action(struct xlate_ctx *ctx, int len, >> } >> recirc_refs_add(&ctx->xout->recircs, recirc_id); >> >> - /* If the controller action didn't request a meter (indicated by a >> - * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was >> - * configured through the "controller" virtual meter. >> - * >> - * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is >> - * configured. */ >> - uint32_t meter_id; >> - if (provider_meter_id == UINT32_MAX) { >> - meter_id = ctx->xbridge->ofproto->up.controller_meter_id; >> - } else { >> - meter_id = provider_meter_id; >> - } >> - >> - size_t offset; >> - size_t ac_offset; >> - if (meter_id != UINT32_MAX) { >> - /* If controller meter is configured, generate clone(meter, userspace) >> - * action. */ >> - offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE); >> - nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, >> - UINT32_MAX); >> - ac_offset = nl_msg_start_nested(ctx->odp_actions, >> - OVS_SAMPLE_ATTR_ACTIONS); >> - nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id); >> - } >> - >> /* Generate the datapath flows even if we don't send the packet-in >> * so that debugging more closely represents normal state. */ >> bool dont_send = false; >> @@ -5178,12 +5184,7 @@ xlate_controller_action(struct xlate_ctx *ctx, int len, >> dont_send = true; >> } >> put_controller_user_action(ctx, dont_send, false, recirc_id, len, >> - reason, controller_id); >> - >> - if (meter_id != UINT32_MAX) { >> - nl_msg_end_nested(ctx->odp_actions, ac_offset); >> - nl_msg_end_nested(ctx->odp_actions, offset); >> - } >> + reason, provider_meter_id, controller_id); >> } >> >> /* Creates a frozen state, and allocates a unique recirc id for the given >> @@ -5235,6 +5236,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table) >> put_controller_user_action(ctx, false, true, recirc_id, >> ctx->pause->max_len, >> ctx->pause->reason, >> + ctx->pause->provider_meter_id, >> ctx->pause->controller_id); >> } else { >> if (ctx->recirc_update_dp_hash) { >> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> index a1393f7f8e..db205a5316 100644 >> --- a/tests/ofproto-dpif.at >> +++ b/tests/ofproto-dpif.at >> @@ -6040,6 +6040,7 @@ m4_define([CHECK_CONTINUATION], [dnl >> ]) >> ]) >> >> + >> # Check that pause at the end of the pipeline works OK. >> # >> # (xlate_continuation() has a special case for no-op actions; this >> @@ -6160,6 +6161,7 @@ AT_CHECK([test 1 = `$PYTHON3 "$top_srcdir/utilities/ovs-pcap.in" p2-tx.pcap | wc >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> >> + >> AT_SETUP([ofproto-dpif - continuation with conntrack]) >> AT_KEYWORDS([continuations pause resume]) >> OVS_VSWITCHD_START > > > Unrelated changes. Yuck... I don't know why that got in. Will trim. >> @@ -6195,6 +6197,54 @@ AT_CHECK([test 1 = `$PYTHON3 "$top_srcdir/utilities/ovs-pcap.in" p2-tx.pcap | wc >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([ofproto-dpif - continuation with meters]) >> +AT_KEYWORDS([continuations pause meters]) >> +OVS_VSWITCHD_START >> +add_of_ports br0 1 2 >> + >> +dnl Add meter with id=1 > > A period at the end. Ack >> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps bands=type=drop rate=1']) >> + >> +AT_DATA([flows.txt], [dnl >> +table=0 dl_dst=50:54:00:00:00:0a actions=goto_table(1) >> +table=1 dl_dst=50:54:00:00:00:0a actions=controller(pause,meter_id=1) >> +]) >> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flows br0 flows.txt]) >> + >> +AT_CAPTURE_FILE([ofctl_monitor.log]) >> +AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl -P nxt_packet_in --detach --no-chdir --pidfile 2> ofctl_monitor.log]) > > May better to wrap the line before the '--detach'. > Modern autotools handle '\' just fine. > > And we need to register an on_exit hook to kill this daemon > if something goes wrong. Ack to both >> + >> +ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)' > > AT_CHECK. > And maybe also wrap after the port. > >> + >> +OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 2]) > > $() is preffered over ``. Okay - I think I copied these from another test in the file, so I stuck with the original syntax. I can change it. >> +OVS_APP_EXIT_AND_WAIT([ovs-ofctl]) >> +AT_CHECK([cat ofctl_monitor.log], [0], [dnl >> +NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via action) data_len=14 (unbuffered) >> +vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234 >> +]) >> + >> +AT_CHECK([ovs-appctl revalidator/purge], [0]) >> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-flows br0 | ofctl_strip | sort], [0], [dnl >> + n_packets=1, n_bytes=14, dl_dst=50:54:00:00:00:0a actions=goto_table:1 >> + table=1, n_packets=1, n_bytes=14, dl_dst=50:54:00:00:00:0a actions=controller(pause,meter_id=1) >> +OFPST_FLOW reply (OF1.3): >> +]) >> + >> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 | ofctl_strip | sort], [0], [dnl >> +OFPST_METER_CONFIG reply (OF1.3): >> +meter=1 pktps bands= >> +type=drop rate=1 >> +]) >> + >> +AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | strip_timers], [0], [dnl >> +OFPST_METER reply (OF1.3) (xid=0x2): >> +meter:1 flow_count:0 packet_in_count:1 byte_in_count:14 duration:0.0s bands: >> +0: packet_count:0 byte_count:0 >> +]) >> + >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> + >> AT_SETUP([ofproto-dpif - continuation with patch port]) >> AT_KEYWORDS([continuations pause resume]) >> OVS_VSWITCHD_START(
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 89f183182e..4f1e57e6d1 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -5080,10 +5080,37 @@ put_controller_user_action(struct xlate_ctx *ctx, bool dont_send, bool continuation, uint32_t recirc_id, int len, enum ofp_packet_in_reason reason, + uint32_t provider_meter_id, uint16_t controller_id) { struct user_action_cookie cookie; + /* If the controller action didn't request a meter (indicated by a + * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was + * configured through the "controller" virtual meter. + * + * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is + * configured. */ + uint32_t meter_id; + if (provider_meter_id == UINT32_MAX) { + meter_id = ctx->xbridge->ofproto->up.controller_meter_id; + } else { + meter_id = provider_meter_id; + } + + size_t offset; + size_t ac_offset; + if (meter_id != UINT32_MAX) { + /* If controller meter is configured, generate clone(meter, + * userspace) action. */ + offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE); + nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, + UINT32_MAX); + ac_offset = nl_msg_start_nested(ctx->odp_actions, + OVS_SAMPLE_ATTR_ACTIONS); + nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id); + } + memset(&cookie, 0, sizeof cookie); cookie.type = USER_ACTION_COOKIE_CONTROLLER; cookie.ofp_in_port = OFPP_NONE, @@ -5101,6 +5128,11 @@ put_controller_user_action(struct xlate_ctx *ctx, uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port); odp_put_userspace_action(pid, &cookie, sizeof cookie, ODPP_NONE, false, ctx->odp_actions, NULL); + + if (meter_id != UINT32_MAX) { + nl_msg_end_nested(ctx->odp_actions, ac_offset); + nl_msg_end_nested(ctx->odp_actions, offset); + } } static void @@ -5145,32 +5177,6 @@ xlate_controller_action(struct xlate_ctx *ctx, int len, } recirc_refs_add(&ctx->xout->recircs, recirc_id); - /* If the controller action didn't request a meter (indicated by a - * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was - * configured through the "controller" virtual meter. - * - * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is - * configured. */ - uint32_t meter_id; - if (provider_meter_id == UINT32_MAX) { - meter_id = ctx->xbridge->ofproto->up.controller_meter_id; - } else { - meter_id = provider_meter_id; - } - - size_t offset; - size_t ac_offset; - if (meter_id != UINT32_MAX) { - /* If controller meter is configured, generate clone(meter, userspace) - * action. */ - offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE); - nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, - UINT32_MAX); - ac_offset = nl_msg_start_nested(ctx->odp_actions, - OVS_SAMPLE_ATTR_ACTIONS); - nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id); - } - /* Generate the datapath flows even if we don't send the packet-in * so that debugging more closely represents normal state. */ bool dont_send = false; @@ -5178,12 +5184,7 @@ xlate_controller_action(struct xlate_ctx *ctx, int len, dont_send = true; } put_controller_user_action(ctx, dont_send, false, recirc_id, len, - reason, controller_id); - - if (meter_id != UINT32_MAX) { - nl_msg_end_nested(ctx->odp_actions, ac_offset); - nl_msg_end_nested(ctx->odp_actions, offset); - } + reason, provider_meter_id, controller_id); } /* Creates a frozen state, and allocates a unique recirc id for the given @@ -5235,6 +5236,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table) put_controller_user_action(ctx, false, true, recirc_id, ctx->pause->max_len, ctx->pause->reason, + ctx->pause->provider_meter_id, ctx->pause->controller_id); } else { if (ctx->recirc_update_dp_hash) { diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index a1393f7f8e..db205a5316 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -6040,6 +6040,7 @@ m4_define([CHECK_CONTINUATION], [dnl ]) ]) + # Check that pause at the end of the pipeline works OK. # # (xlate_continuation() has a special case for no-op actions; this @@ -6160,6 +6161,7 @@ AT_CHECK([test 1 = `$PYTHON3 "$top_srcdir/utilities/ovs-pcap.in" p2-tx.pcap | wc OVS_VSWITCHD_STOP AT_CLEANUP + AT_SETUP([ofproto-dpif - continuation with conntrack]) AT_KEYWORDS([continuations pause resume]) OVS_VSWITCHD_START @@ -6195,6 +6197,54 @@ AT_CHECK([test 1 = `$PYTHON3 "$top_srcdir/utilities/ovs-pcap.in" p2-tx.pcap | wc OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - continuation with meters]) +AT_KEYWORDS([continuations pause meters]) +OVS_VSWITCHD_START +add_of_ports br0 1 2 + +dnl Add meter with id=1 +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps bands=type=drop rate=1']) + +AT_DATA([flows.txt], [dnl +table=0 dl_dst=50:54:00:00:00:0a actions=goto_table(1) +table=1 dl_dst=50:54:00:00:00:0a actions=controller(pause,meter_id=1) +]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flows br0 flows.txt]) + +AT_CAPTURE_FILE([ofctl_monitor.log]) +AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl -P nxt_packet_in --detach --no-chdir --pidfile 2> ofctl_monitor.log]) + +ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)' + +OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 2]) +OVS_APP_EXIT_AND_WAIT([ovs-ofctl]) +AT_CHECK([cat ofctl_monitor.log], [0], [dnl +NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via action) data_len=14 (unbuffered) +vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234 +]) + +AT_CHECK([ovs-appctl revalidator/purge], [0]) +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-flows br0 | ofctl_strip | sort], [0], [dnl + n_packets=1, n_bytes=14, dl_dst=50:54:00:00:00:0a actions=goto_table:1 + table=1, n_packets=1, n_bytes=14, dl_dst=50:54:00:00:00:0a actions=controller(pause,meter_id=1) +OFPST_FLOW reply (OF1.3): +]) + +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 | ofctl_strip | sort], [0], [dnl +OFPST_METER_CONFIG reply (OF1.3): +meter=1 pktps bands= +type=drop rate=1 +]) + +AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | strip_timers], [0], [dnl +OFPST_METER reply (OF1.3) (xid=0x2): +meter:1 flow_count:0 packet_in_count:1 byte_in_count:14 duration:0.0s bands: +0: packet_count:0 byte_count:0 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto-dpif - continuation with patch port]) AT_KEYWORDS([continuations pause resume]) OVS_VSWITCHD_START(