> On 11 Jun 2024, at 00:07, Dumitru Ceara <[email protected]> wrote: > > On 6/10/24 18:05, [email protected] > <mailto:[email protected]> wrote: >> On Mon, 2024-06-10 at 12:54 +0000, Naveen Yerramneni wrote: >>> Action length is getting set incorrectly during ct_commit encode >>> due to which ct action is getting skipped during phsycial flows >>> creation. This issue is noticed only if ct_commit is prefixed >>> with other actions. >>> >>> logical flow: reg8[17] = 1; ct_commit { ct_mark.blocked = 1; }; >>> without fix: encodes as set_field:0x2000000000000/0x2000000000000- >>>> xreg4 >>> with fix: encodes as set_field:0x2000000000000/0x2000000000000- >>>> xreg4,ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1- >>>> ct_mark)) >>> >>> Signed-off-by: Naveen Yerramneni <[email protected]> >>> --- > > Thanks Naveen, for the catch! > >>> v1: > > Well, this should actually be v2 but that's a technicality. > >>> - Addressed review comments from Ales. >>> --- >>> lib/actions.c | 3 +++ >>> tests/ovn.at | 3 +++ >>> 2 files changed, 6 insertions(+) >>> >>> diff --git a/lib/actions.c b/lib/actions.c >>> index 361d55009..794d2e916 100644 >>> --- a/lib/actions.c >>> +++ b/lib/actions.c >>> @@ -760,6 +760,8 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, >>> const struct ovnact_encode_params *ep >>> OVS_UNUSED, >>> struct ofpbuf *ofpacts) >>> { >>> + size_t ct_offset = ofpacts->size; >>> + ofpbuf_pull(ofpacts, ct_offset); > > Nit: missing new line (but I can fix that up at apply time). > >>> struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); >>> ct->flags = NX_CT_F_COMMIT; >>> ct->recirc_table = NX_CT_RECIRC_NONE; >>> @@ -791,6 +793,7 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, >>> ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); >>> ct = ofpacts->header; >>> ofpact_finish(ofpacts, &ct->ofpact); >>> + ofpbuf_push_uninit(ofpacts, ct_offset); >> >> Just a thought here. I was just wondering if this wouldn't be a good >> opportunity to also replace the "manual" way of finishing the action >> encoding (as @amusil referred to it in this review [0]) with the more >> explicit approach (example included in [0]). >> > > I agree that we should do that but we should do it in multiple > functions, e.g.: > > encode_CT_NEXT() > encode_ct_nat() > encode_ct_lb() > > I think it's fine to do that in a follow up patch. Martin, do you have > time to post one? Otherwise I'll put it on my list of things to do in > the future.
Sure, I’ll find some time to make the proposal this week. > > Regarding the current bug fix I'm planning to apply and backport this > patch, pending ovsrobot re-run results. > > Recheck-request: github-robot > > Regards, > Dumitru > >> [0] >> https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html >> >>> } >>> >>> static void >>> diff --git a/tests/ovn.at b/tests/ovn.at >>> index dc6aafd53..d4f62f487 100644 >>> --- a/tests/ovn.at >>> +++ b/tests/ovn.at >>> @@ -1315,6 +1315,9 @@ ct_commit { >>> ct_label=0x181716151413121110090807060504030201; }; >>> 141-bit constant is not compatible with 128-bit field ct_label. >>> ct_commit { ip4.dst = 192.168.0.1; }; >>> Field ip4.dst is not modifiable. >>> +reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; >>> + encodes as set_field:0x2000000000000/0x2000000000000- >>>> xreg4,ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1- >>>> ct_mark)) >>> + has prereqs ip >>> >>> # Legact ct_commit_v1 action. >>> ct_commit(); >> >> Martin. >> _______________________________________________ >> dev mailing list >> [email protected] <mailto:[email protected]> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev Martin. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
