On Wed, Jan 22, 2020 at 1:17 AM Numan Siddique <num...@ovn.org> wrote:
>
> On Tue, Jan 21, 2020 at 4:27 AM Han Zhou <hz...@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 <hz...@ovn.org>
>
> Hi Han,
>
> Thanks for v2.
>
> I have one comment. Please see below.
>
> With that addressed - Acked-by: Numan Siddique <num...@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.
;
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
