Ilya Maximets <[email protected]> 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 <[email protected]>
>> Signed-off-by: Aaron Conole <[email protected]>
>> ---
>> 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(
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
