On Wed, Jan 22, 2020 at 9:46 PM Han Zhou <hz...@ovn.org> wrote: > > 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:
Looks good to me. Acked-by: Numan Siddique <num...@ovn.org> Thanks Numan > > - - - - - - -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 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev