Re: [ovs-dev] [PATCH v2] ofproto-dpif-xlate: Fix continuations with associated metering.
Aaron Conole writes:
> Ilya Maximets writes:
>
>> On 3/22/24 14:40, 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
>>> Signed-off-by: Aaron Conole
>>> ---
>>> v1 -> v2:
>>> Clean up unrelated whitespace change
>>> Fix style issues around test comments
>>> Fix a line length issue truncating action in a comment
>>> Change from `` to $() in the test
>>
>> Nit: May also change `` to $() in on_exit hook.
>
> I'll fix on apply.
>
>> But anyway:
>>
>> Acked-by: Ilya Maximets
>
> Thanks for the review!
I fixed the nit and applied to all branches back to 2.17
Thanks all!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ofproto-dpif-xlate: Fix continuations with associated metering.
Ilya Maximets writes:
> On 3/22/24 14:40, 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
>> Signed-off-by: Aaron Conole
>> ---
>> v1 -> v2:
>> Clean up unrelated whitespace change
>> Fix style issues around test comments
>> Fix a line length issue truncating action in a comment
>> Change from `` to $() in the test
>
> Nit: May also change `` to $() in on_exit hook.
I'll fix on apply.
> But anyway:
>
> Acked-by: Ilya Maximets
Thanks for the review!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ofproto-dpif-xlate: Fix continuations with associated metering.
On 3/22/24 14:40, 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
> Signed-off-by: Aaron Conole
> ---
> v1 -> v2:
> Clean up unrelated whitespace change
> Fix style issues around test comments
> Fix a line length issue truncating action in a comment
> Change from `` to $() in the test
Nit: May also change `` to $() in on_exit hook.
But anyway:
Acked-by: Ilya Maximets
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] ofproto-dpif-xlate: Fix continuations with associated metering.
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
Signed-off-by: Aaron Conole
---
v1 -> v2:
Clean up unrelated whitespace change
Fix style issues around test comments
Fix a line length issue truncating action in a comment
Change from `` to $() in the test
ofproto/ofproto-dpif-xlate.c | 66 +++-
tests/ofproto-dpif.at| 51
2 files changed, 85 insertions(+), 32 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 89f183182e..7c49508950 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(, 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, , 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(>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;
