Hello Ales, I was wrong about the length of logical pipelines. I thought we should count them using table in OpenFlow.
For egress exit, for example, OFTABLE_SAVE_INPORT - OFTABLE_LOG_EGRESS_PIPELINE. In v2 I changed the length for ingress to 30 and to 11 for egress, as you wrote in your first comment. It might be worth having as well the ability to determine which pipeline switch or router it is, in order to process operations for them efficiently, because the length of pipelines are different for the router and switch in northd.h I will be glad to see your comments on the v2. Thank you! On 15 Jan 2025, at 13:43, Ales Musil <[email protected]> wrote: 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]<mailto:[email protected]>> wrote: On Fri, Jan 10, 2025 at 5:24 PM Alexandra Rukomoinikova <[email protected]<mailto:[email protected]>> wrote: Logical pipeline length was correctly calculated only for ingress. Calculation for egress was added. Signed-off-by: Alexandra Rukomoinikova <[email protected]<mailto:[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<http://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<http://ovn.at/> b/tests/ovn.at<http://ovn.at/> index 826b52051..3f32bf655 100644 --- a/tests/ovn.at<http://ovn.at/> +++ b/tests/ovn.at<http://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]<mailto:[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
