On Fri, Jan 10, 2025 at 5:24 PM Alexandra Rukomoinikova <[email protected]> wrote:
> Logical pipeline length was correctly calculated > only for ingress. Calculation for egress was added. > > Signed-off-by: Alexandra Rukomoinikova <[email protected]> > --- > Hi Alexandra, thank you for the patch. I have one comment/question down below. controller/lflow.c | 6 ++++-- > lib/actions.c | 5 ++++- > lib/ovn-util.h | 3 ++- > tests/ovn.at | 4 ++-- > tests/test-ovn.c | 3 ++- > utilities/ovn-trace.c | 9 +++++---- > 6 files changed, 19 insertions(+), 11 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 13c3a0d73..1a2500177 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -331,8 +331,10 @@ lflow_parse_actions(const struct sbrec_logical_flow > *lflow, > .nd_ra_opts = l_ctx_in->nd_ra_opts, > .controller_event_opts = l_ctx_in->controller_event_opts, > > - .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS, > - .n_tables = LOG_PIPELINE_LEN, > + .pipeline = ingress ? OVNACT_P_INGRESS > + : OVNACT_P_EGRESS, > + .n_tables = ingress ? LOG_PIPELINE_INGRESS_LEN > + : LOG_PIPELINE_EGRESS_LEN, > .cur_ltable = lflow->table_id, > }; > > diff --git a/lib/actions.c b/lib/actions.c > index c12d087e7..c15f42dfe 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -362,7 +362,10 @@ parse_NEXT(struct action_context *ctx) > } > } > > - if (table >= ctx->pp->n_tables) { > + if ((pipeline == OVNACT_P_INGRESS > + && table >= LOG_PIPELINE_INGRESS_LEN) || > + (pipeline == OVNACT_P_EGRESS > + && table >= LOG_PIPELINE_EGRESS_LEN)) { > lexer_error(ctx->lexer, > "\"next\" action cannot advance beyond table %d.", > ctx->pp->n_tables - 1); > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index 7b98b9b9a..001f7035b 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -308,7 +308,8 @@ BUILD_ASSERT_DECL( > #define SCTP_ABORT_CHUNK_FLAG_T (1 << 0) > > /* The number of tables for the ingress and egress pipelines. */ > -#define LOG_PIPELINE_LEN 30 > +#define LOG_PIPELINE_INGRESS_LEN 31 > +#define LOG_PIPELINE_EGRESS_LEN 19 > I don't think that those numbers are actually correct. According to the usage those are supposed to represent maximum logical flow table numbers so it should be "maximum + 1" from ingress/egress that we have in northd.h for pipeline stages. In case of ingress it would be 30 as the highest is "ls_in_l2_unknown" so we shouldn't be able to call next here. The same goes for the egress where the highest is "ls_out_apply_port_sec" which would give us 11. Could you please share how you ended up with the values above? I wonder if we could create those dynamically in any way so we don't have to adjust them every time the pipeline grows/shrinks. > > static inline uint32_t > hash_add_in6_addr(uint32_t hash, const struct in6_addr *addr) > diff --git a/tests/ovn.at b/tests/ovn.at > index 826b52051..3f32bf655 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1017,8 +1017,8 @@ next(); > Syntax error at `)' expecting "pipeline" or "table". > next(10; > Syntax error at `;' expecting `)'. > -next(24); > - "next" action cannot advance beyond table 23. > +next(31); > + "next" action cannot advance beyond table 30. > > next(table=lflow_table); > formats as next; > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > index 2ea68f212..709b411d3 100644 > --- a/tests/test-ovn.c > +++ b/tests/test-ovn.c > @@ -1334,7 +1334,8 @@ test_parse_actions(struct ovs_cmdl_context *ctx > OVS_UNUSED) > .dhcpv6_opts = &dhcpv6_opts, > .nd_ra_opts = &nd_ra_opts, > .controller_event_opts = &event_opts, > - .n_tables = 24, > + .pipeline = OVNACT_P_INGRESS, > + .n_tables = 31, > .cur_ltable = 10, > }; > struct lex_str exp_input = > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > index 13a4ea490..0d006ecbb 100644 > --- a/utilities/ovn-trace.c > +++ b/utilities/ovn-trace.c > @@ -988,16 +988,17 @@ parse_lflow_for_datapath(const struct > sbrec_logical_flow *sblf, > return; > } > > + bool ingress = !strcmp(sblf->pipeline, "ingress"); > struct ovnact_parse_params pp = { > .symtab = &symtab, > .dhcp_opts = &dhcp_opts, > .dhcpv6_opts = &dhcpv6_opts, > .nd_ra_opts = &nd_ra_opts, > .controller_event_opts = &event_opts, > - .pipeline = (!strcmp(sblf->pipeline, "ingress") > - ? OVNACT_P_INGRESS > - : OVNACT_P_EGRESS), > - .n_tables = LOG_PIPELINE_LEN, > + .pipeline = ingress ? OVNACT_P_INGRESS > + : OVNACT_P_EGRESS, > + .n_tables = ingress ? LOG_PIPELINE_INGRESS_LEN > + : LOG_PIPELINE_EGRESS_LEN, > .cur_ltable = sblf->table_id, > }; > uint64_t stub[1024 / 8]; > -- > 2.34.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
