On Sun, Mar 23, 2025 at 3:43 PM Alexandra Rukomoinikova
<[email protected]> wrote:

> Logical pipeline length was correctly calculated only for ingress.
> The calculation has been extended to include egress as well.
>
> Signed-off-by: Alexandra Rukomoinikova <[email protected]>
> ---
> v1 --> v2: fixed failed tests, rebased.
> ---
>

Hi Alexandra,

thank you for the patch.

 controller/lflow.c    | 6 ++++--
>  lib/actions.c         | 8 ++++++--
>  lib/ovn-util.h        | 3 ++-
>  tests/ovn.at          | 4 ++--
>  tests/test-ovn.c      | 3 ++-
>  utilities/ovn-trace.c | 9 +++++----
>  6 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 6be1eec13..e445602e9 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -332,8 +332,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 158c367ae..251a13ea7 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -362,10 +362,14 @@ parse_NEXT(struct action_context *ctx)
>          }
>      }
>
> -    if (table >= ctx->pp->n_tables) {
> +    int max_table = pipeline == OVNACT_P_INGRESS
> +                             ? LOG_PIPELINE_INGRESS_LEN
> +                             : LOG_PIPELINE_EGRESS_LEN;
> +
> +    if (table >= max_table) {
>          lexer_error(ctx->lexer,
>                      "\"next\" action cannot advance beyond table %d.",
> -                    ctx->pp->n_tables - 1);
> +                    max_table);
>          return;
>      }
>
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index ce8cc0568..62bf1797b 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -313,7 +313,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 30
> +#define LOG_PIPELINE_EGRESS_LEN 13
>
>  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 afde2576f..bbc4b7f31 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -832,8 +832,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(30);
> +#    "next" action cannot advance beyond table 29.
>
>  next(table=lflow_table);
>      formats as next;
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index 59328dc6c..3c757bfe6 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -1341,7 +1341,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 = 30,
>

I think it would be better to use the constant here too, it can be done
during merge.

             .cur_ltable = 10,
>          };
>
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index d25c612c7..b8a9e0f0a 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -990,16 +990,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.48.1
>
>
Other than that:
Acked-by: Ales Musil <[email protected]>

Thanks,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to