On 4/11/25 12:24 PM, Ales Musil wrote:
> On Thu, Apr 10, 2025 at 11:42 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
>> The new action stores the 8 relevant bits of the OVS ct_state field into
>> a register. This is implemented through an additional OpenFlow table,
>> OFTABLE_CT_STATE_SAVE, that contains two (static) rules:
>> - high-priority, set the target register to 0 if ct_state -trk is set
>> - low-priority, copy the 8 bits ct_state to the target register
>> otherwise
>>
>> From a logical flow perspective, the action is supposed to be used in
>> the following way:
>> R = ct_state_save();
>>
>> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
>> ---
>>
>
> Hi Dumitru,
>
> thank you for the fix. I have one small comment down below.
>
Hi Ales,
Thanks for the review!
[...]
>> @@ -37003,8 +37025,10 @@ check_default_flows() {
>> # Tables 81, 82 and 83 are part of ct_nw_dst(), ct_ip6_dst() and
>> ct_tp_dst()
>> # actions respectively and its OK for them to not have default
>> flows.
>> # Table 84 is part of flood_remote; action and its OK for
>> - # it to not have default flows.
>> - if test ${table} -eq 68 -o ${table} -eq 70 -o ${table} -eq 81 -o
>> ${table} -eq 82 -o ${table} -eq 83 -o ${table} -eq 84; then
>> + # it to not have default flows.
>> + # Table 85 is the implementation of the ct_state_save action and
>> + # doesn't require a default flow.
>> + if test ${table} -eq 68 -o ${table} -eq 70 -o ${table} -eq 81 -o
>> ${table} -eq 82 -o ${table} -eq 83 -o ${table} -eq 84 -o ${table} -eq 85;
>> then
>> continue;
>>
>
> nit: We should really use the table names instead of numbers
> here. Let's do it as a follow up considering this was there
> before this change.
>
>
You're right, I'll post a separate patch for that.
>> fi
>> AT_CHECK([grep -qe "table=$table.*
>> priority=0\(,metadata=0x\w*\)\? actions" oflows], [0], [ignore], [ignore],
>> [echo "Table $table does not contain a default action"])
>> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
>> index de0cb0b444..c4eb2a7dfb 100644
>> --- a/tests/test-ovn.c
>> +++ b/tests/test-ovn.c
>> @@ -1388,6 +1388,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx
>> OVS_UNUSED)
>> .ct_ip6_dst_load_table = OFTABLE_CT_ORIG_IP6_DST_LOAD,
>> .ct_tp_dst_load_table = OFTABLE_CT_ORIG_TP_DST_LOAD,
>> .flood_remote_table = OFTABLE_FLOOD_REMOTE_CHASSIS,
>> + .ct_state_save_table = OFTABLE_CT_STATE_SAVE,
>> .lflow_uuid.parts =
>> { 0xaaaaaaaa, 0xbbbbbbbb, 0xcccccccc, 0xdddddddd},
>> .dp_key = 0xabcdef,
>> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
>> index b8a9e0f0a6..b801aaead1 100644
>> --- a/utilities/ovn-trace.c
>> +++ b/utilities/ovn-trace.c
>> @@ -3121,6 +3121,21 @@ execute_check_out_port_sec(const struct
>> ovnact_result *dl,
>> mf_write_subfield_flow(&sf, &sv, uflow);
>> }
>>
>> +static void
>> +execute_ct_save_state(const struct ovnact_result *dl, struct flow *uflow,
>> + struct ovs_list *super)
>> +{
>> + struct mf_subfield sf = expr_resolve_field(&dl->dst);
>> + union mf_subvalue sv = { .u8_val = uflow->ct_state };
>> + mf_write_subfield_flow(&sf, &sv, uflow);
>>
>
> This is not correct, we should copy only if the current CT state is not
> tracked.
>
>
Good catch! I fixed it in v2.
>> +
>> + struct ds s = DS_EMPTY_INITIALIZER;
>> + expr_field_format(&dl->dst, &s);
>> + ovntrace_node_append(super, OVNTRACE_NODE_MODIFY,
>> + "%s = %"PRIu8, ds_cstr(&s), uflow->ct_state);
Actually, I just realized this should be OVNTRACE_NODE_ACTION.
I changed it to:
static void
execute_ct_save_state(const struct ovnact_result *dl, struct flow *uflow,
struct ovs_list *super)
{
struct mf_subfield sf = expr_resolve_field(&dl->dst);
union mf_subvalue sv = {
.u8_val = uflow->ct_state & CS_TRACKED ? uflow->ct_state : 0,
};
mf_write_subfield_flow(&sf, &sv, uflow);
struct ds s = DS_EMPTY_INITIALIZER;
expr_field_format(&dl->dst, &s);
ovntrace_node_append(super, OVNTRACE_NODE_ACTION, "/* %s = %"PRIu8"; */",
ds_cstr(&s), sv.u8_val);
ds_destroy(&s);
}
This will now generate an ovn-trace ouptut like:
22. lr_in_chk_pkt_len (northd.c:13285): outport == "lrp0", priority 50, uuid
c665520f
reg9[1] = check_pkt_larger(8014);
reg4[0..7] = ct_state_save();
/* reg4[0..7] = 0; */
next;
>> + ds_destroy(&s);
>> +}
>> +
>> static void
>> trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
>> const struct ovntrace_datapath *dp, struct flow *uflow,
>> @@ -3464,6 +3479,10 @@ trace_actions(const struct ovnact *ovnacts, size_t
>> ovnacts_len,
>> case OVNACT_FLOOD_REMOTE:
>> ovntrace_node_append(super, OVNTRACE_NODE_OUTPUT,
>> "/* Flood to all remote chassis */");
>> + break;
>> + case OVNACT_CT_STATE_SAVE:
>> + execute_ct_save_state(ovnact_get_CT_STATE_SAVE(a), uflow,
>> super);
>> + break;
>> }
>> }
>> ofpbuf_uninit(&stack);
>> --
>> 2.48.1
>>
>>
> With the above addressed:
> Acked-by: Ales Musil <amu...@redhat.com>
>
> Thanks,
> Ales
>
It would be great if you could have another quick look at the new revision:
https://patchwork.ozlabs.org/project/ovn/list/?series=452578&state=*
Thanks,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
