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

Han Zhou hzhou at ovn.org
Wed Jan 22 19:56:35 UTC 2020


On Wed, Jan 22, 2020 at 10:10 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Wed, Jan 22, 2020 at 9:46 PM Han Zhou <hzhou at ovn.org> wrote:
> >
> > 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:
>
> Looks good to me.
>
> Acked-by: Numan Siddique <numans at ovn.org>
>
> Thanks
> Numan
>

Thanks Numan. I applied this to master.

> >
> > - - - - - - -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.
> >  ;
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >


More information about the dev mailing list