Could you elaborate a bit? The LS and LR stages are overlapping and LR ones are actually shorter so it should be based on the LS ones. Does that make sense?
On Wed, Jan 15, 2025 at 11:35 AM Rukomoinikova Aleksandra <[email protected]> wrote: > Hello! Thanks for the comment, but shouldn’t we take into account the > length > of the logical router pipeline in the pipeline length? > > On 14 Jan 2025, at 12:27, Ales Musil <[email protected]> wrote: > > > > 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
