[ovs-dev] [PATCH ovn v2 1/4] ovn-controller: A new action "select".

Han Zhou hzhou at ovn.org
Wed Jan 22 16:15:46 UTC 2020


On Wed, Jan 22, 2020 at 1:17 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Tue, Jan 21, 2020 at 4:27 AM Han Zhou <hzhou at ovn.org> wrote:
> >
> > Support a new logical flow action "select", which can be used to
> > implement features such as ECMP.  The action uses OpenFlow group
> > action to select an integer (uint16_t) from a list of integers,
> > and assign it to specified field, e.g.:
> >     reg0 = select(1, 2, 3)
> > A weight can be specified for each member as well, e.g.:
> >     reg0 = select(1=20, 2=30, 3=50)
> >
> > Signed-off-by: Han Zhou <hzhou at ovn.org>
>
> Hi Han,
>
> Thanks for v2.
>
> I have one comment. Please see below.
>
> With that addressed - Acked-by: Numan Siddique <numans at ovn.org> for
> the entire series.
>
> Thanks
> Numan
>
> > ---
> > v1 -> v2: updated according to Numan's comment. Changed the select
> >     action format from select(<result>, ...) to <result> = select(...).
> >
> >  include/ovn/actions.h     |  15 ++++++
> >  lib/actions.c             | 130
++++++++++++++++++++++++++++++++++++++++++++--
> >  ovn-sb.xml                |  34 ++++++++++++
> >  tests/ovn.at              |  23 ++++++++
> >  tests/test-ovn.c          |  26 +++++++++-
> >  utilities/ovn-trace.8.xml |   9 ++++
> >  utilities/ovn-trace.c     |  66 ++++++++++++++++++++++-
> >  7 files changed, 296 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index 047a8d7..2d4b05b 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -61,6 +61,7 @@ struct ovn_extend_table;
> >      OVNACT(CT_DNAT,           ovnact_ct_nat)          \
> >      OVNACT(CT_SNAT,           ovnact_ct_nat)          \
> >      OVNACT(CT_LB,             ovnact_ct_lb)           \
> > +    OVNACT(SELECT,            ovnact_select)          \
> >      OVNACT(CT_CLEAR,          ovnact_null)            \
> >      OVNACT(CLONE,             ovnact_nest)            \
> >      OVNACT(ARP,               ovnact_nest)            \
> > @@ -251,6 +252,20 @@ struct ovnact_ct_lb {
> >      uint8_t ltable;             /* Logical table ID of next table. */
> >  };
> >
> > +struct ovnact_select_dst {
> > +    uint16_t id;
> > +    uint16_t weight;
> > +};
> > +
> > +/* OVNACT_SELECT. */
> > +struct ovnact_select {
> > +    struct ovnact ovnact;
> > +    struct ovnact_select_dst *dsts;
> > +    size_t n_dsts;
> > +    uint8_t ltable;             /* Logical table ID of next table. */
> > +    struct expr_field res_field;
> > +};
> > +
> >  /* OVNACT_ARP, OVNACT_ND_NA, OVNACT_CLONE. */
> >  struct ovnact_nest {
> >      struct ovnact ovnact;
> > diff --git a/lib/actions.c b/lib/actions.c
> > index 051e6c8..b4743bd 100644
> > --- a/lib/actions.c
> > +++ b/lib/actions.c
> > @@ -218,17 +218,18 @@ action_parse_field(struct action_context *ctx,
> >  }
> >
> >  static bool
> > -action_parse_port(struct action_context *ctx, uint16_t *port)
> > +action_parse_uint16(struct action_context *ctx, uint16_t *_value,
> > +                    const char *msg)
> >  {
> >      if (lexer_is_int(ctx->lexer)) {
> >          int value = ntohll(ctx->lexer->token.value.integer);
> >          if (value <= UINT16_MAX) {
> > -            *port = value;
> > +            *_value = value;
> >              lexer_get(ctx->lexer);
> >              return true;
> >          }
> >      }
> > -    lexer_syntax_error(ctx->lexer, "expecting port number");
> > +    lexer_syntax_error(ctx->lexer, "expecting %s", msg);
> >      return false;
> >  }
> >
> > @@ -927,7 +928,7 @@ parse_ct_lb_action(struct action_context *ctx)
> >                  }
> >                  dst.port = 0;
> >                  if (lexer_match(ctx->lexer, LEX_T_COLON)
> > -                    && !action_parse_port(ctx, &dst.port)) {
> > +                    && !action_parse_uint16(ctx, &dst.port, "port
number")) {
> >                      free(dsts);
> >                      return;
> >                  }
> > @@ -957,7 +958,8 @@ parse_ct_lb_action(struct action_context *ctx)
> >                          lexer_syntax_error(ctx->lexer, "IPv6 address
needs "
> >                                  "square brackets if port is included");
> >                          return;
> > -                    } else if (!action_parse_port(ctx, &dst.port)) {
> > +                    } else if (!action_parse_uint16(ctx, &dst.port,
> > +                                                    "port number")) {
> >                          free(dsts);
> >                          return;
> >                      }
> > @@ -1099,6 +1101,121 @@ ovnact_ct_lb_free(struct ovnact_ct_lb *ct_lb)
> >  }
> >
> >  static void
> > +parse_select_action(struct action_context *ctx, struct expr_field
*res_field)
> > +{
> > +    if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
> > +        lexer_error(ctx->lexer,
> > +                    "\"select\" action not allowed in last table.");
> > +        return;
> > +    }
> > +
> > +    struct ovnact_select_dst *dsts = NULL;
> > +    size_t allocated_dsts = 0;
> > +    size_t n_dsts = 0;
>
> I think it is better to validate the result field to make sure that it
> is a writable field with at least 16-bits in it ?
>
> Something like -
>
> char *error = expr_type_check(dst, 16, true);
> if (error) {
> lexer_error(ctx->lexer, "%s", error);
> free(error);
> return;
> }
>
> I think it's  better  to expect the result register to be at least
> 16-bit writable field.
>
> I added the below in ovn.at in the action parsing test case and I see
> the below output.
> If the result field is not a register, is it expected for the
> parse_select_action()  to fail ?
> I think it should.
>
> ****
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 25ce34d34..241dd26cd 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1504,6 +1504,9 @@ reg0 = select(1=123456, 2);
>  reg0 = select(123);
>      Syntax error at `;' expecting at least 2 group members.
>
> +ip4.dst = select(1, 2);
> +    Syntax error at `;' expecting at least 2 group members.
> +
>  # Miscellaneous negative tests.
>  ;
>      Syntax error at `;'.
> ****
>
> And the output of testsuite.log is
>
> *********
> ip4.dst = select(1, 2);
> -    Syntax error at `;' expecting at least 2 group members.
> +    formats as ip4.dst = select(1=100, 2=100);
> +    encodes as group:6
> +    uses group: id(6),
>
name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->ip_dst[0..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->ip_dst[0..31],resubmit(,19))
> +    has prereqs eth.type == 0x800
> ********
>
> Although ovn-northd will use the proper result field, it is still
> better to test with invalid result fields.
>
> Thanks
> Numan

Thanks Numan for the great suggestion.
I updated the patch as suggested by slightly different, as the result field
can have 16 *or more* bits. Please check the diff below and confirm if it
looks ok:

- - - - - - -8>< - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
- - - - - - - - - ><8 - - - - - - - -
diff --git a/lib/actions.c b/lib/actions.c
index b4743bd..cd3f586 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -1103,6 +1103,23 @@ ovnact_ct_lb_free(struct ovnact_ct_lb *ct_lb)
 static void
 parse_select_action(struct action_context *ctx, struct expr_field
*res_field)
 {
+    /* Check if the result field is modifiable. */
+    char *error = expr_type_check(res_field, res_field->n_bits, true);
+    if (error) {
+        lexer_error(ctx->lexer, "%s", error);
+        free(error);
+        return;
+    }
+
+    if (res_field->n_bits < 16) {
+        lexer_error(ctx->lexer, "cannot use %d-bit field %s[%d..%d] "
+                    "for \"select\", which requires at least 16 bits.",
+                    res_field->n_bits, res_field->symbol->name,
+                    res_field->ofs,
+                    res_field->ofs + res_field->n_bits - 1);
+        return;
+    }
+
     if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
         lexer_error(ctx->lexer,
                     "\"select\" action not allowed in last table.");
diff --git a/tests/ovn.at b/tests/ovn.at
index 25ce34d..f245083 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1503,6 +1503,10 @@ reg0 = select(1=123456, 2);
     Syntax error at `123456' expecting weight.
 reg0 = select(123);
     Syntax error at `;' expecting at least 2 group members.
+ip.proto = select(1, 2, 3);
+    Field ip.proto is not modifiable.
+reg0[0..14] = select(1, 2, 3);
+    cannot use 15-bit field reg0[0..14] for "select", which requires at
least 16 bits.

 # Miscellaneous negative tests.
 ;


More information about the dev mailing list