| Message ID | 1579560985-36919-1-git-send-email-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn,v2,1/4] ovn-controller: A new action "select". | expand |
On Tue, Jan 21, 2020 at 4:27 AM Han Zhou <hzhou@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@ovn.org> Hi Han, Thanks for v2. I have one comment. Please see below. With that addressed - Acked-by: Numan Siddique <numans@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 > + > + lexer_get(ctx->lexer); /* Skip "select". */ > + lexer_get(ctx->lexer); /* Skip '('. */ > + > + while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) { > + struct ovnact_select_dst dst; > + if (!action_parse_uint16(ctx, &dst.id, "id")) { > + free(dsts); > + return; > + } > + > + dst.weight = 0; > + if (lexer_match(ctx->lexer, LEX_T_EQUALS)) { > + if (!action_parse_uint16(ctx, &dst.weight, "weight")) { > + free(dsts); > + return; > + } > + if (dst.weight == 0) { > + lexer_syntax_error(ctx->lexer, "weight can't be 0"); > + } > + } > + lexer_match(ctx->lexer, LEX_T_COMMA); > + > + /* Append to dsts. */ > + if (n_dsts >= allocated_dsts) { > + dsts = x2nrealloc(dsts, &allocated_dsts, sizeof *dsts); > + } > + dsts[n_dsts++] = dst; > + } > + if (n_dsts <= 1) { > + lexer_syntax_error(ctx->lexer, "expecting at least 2 group members"); > + return; > + } > + > + struct ovnact_select *select = ovnact_put_SELECT(ctx->ovnacts); > + select->ltable = ctx->pp->cur_ltable + 1; > + select->dsts = dsts; > + select->n_dsts = n_dsts; > + select->res_field = *res_field; > +} > + > +static void > +format_SELECT(const struct ovnact_select *select, struct ds *s) > +{ > + expr_field_format(&select->res_field, s); > + ds_put_cstr(s, " = "); > + ds_put_cstr(s, "select"); > + ds_put_char(s, '('); > + for (size_t i = 0; i < select->n_dsts; i++) { > + if (i) { > + ds_put_cstr(s, ", "); > + } > + > + const struct ovnact_select_dst *dst = &select->dsts[i]; > + ds_put_format(s, "%"PRIu16, dst->id); > + ds_put_format(s, "=%"PRIu16, dst->weight ? dst->weight : 100); > + } > + ds_put_char(s, ')'); > + ds_put_char(s, ';'); > +} > + > +static void > +encode_SELECT(const struct ovnact_select *select, > + const struct ovnact_encode_params *ep, > + struct ofpbuf *ofpacts) > +{ > + ovs_assert(select->n_dsts >= 1); > + uint8_t resubmit_table = select->ltable + first_ptable(ep, ep->pipeline); > + uint32_t table_id = 0; > + struct ofpact_group *og; > + > + struct ds ds = DS_EMPTY_INITIALIZER; > + ds_put_format(&ds, "type=select,selection_method=dp_hash"); > + > + struct mf_subfield sf = expr_resolve_field(&select->res_field); > + > + for (size_t bucket_id = 0; bucket_id < select->n_dsts; bucket_id++) { > + const struct ovnact_select_dst *dst = &select->dsts[bucket_id]; > + ds_put_format(&ds, ",bucket=bucket_id=%"PRIuSIZE",weight:%"PRIu16 > + ",actions=", bucket_id, dst->weight ? dst->weight : 100); > + ds_put_format(&ds, "load:%u->%s[%u..%u],", dst->id, sf.field->name, > + sf.ofs, sf.ofs + sf.n_bits - 1); > + ds_put_format(&ds, "resubmit(,%d)", resubmit_table); > + } > + > + table_id = ovn_extend_table_assign_id(ep->group_table, ds_cstr(&ds), > + ep->lflow_uuid); > + ds_destroy(&ds); > + if (table_id == EXT_TABLE_ID_INVALID) { > + return; > + } > + > + /* Create an action to set the group. */ > + og = ofpact_put_GROUP(ofpacts); > + og->group_id = table_id; > +} > + > +static void > +ovnact_select_free(struct ovnact_select *select) > +{ > + free(select->dsts); > +} > + > +static void > format_CT_CLEAR(const struct ovnact_null *null OVS_UNUSED, struct ds *s) > { > ds_put_cstr(s, "ct_clear;"); > @@ -2868,6 +2985,9 @@ parse_set_action(struct action_context *ctx) > } else if (lexer_match(ctx->lexer, LEX_T_EQUALS)) { > if (ctx->lexer->token.type != LEX_T_ID) { > parse_LOAD(ctx, &lhs); > + } else if (!strcmp(ctx->lexer->token.s, "select") > + && lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) { > + parse_select_action(ctx, &lhs); > } else if (!strcmp(ctx->lexer->token.s, "put_dhcp_opts") > && lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) { > parse_put_dhcp_opts(ctx, &lhs, ovnact_put_PUT_DHCPV4_OPTS( > diff --git a/ovn-sb.xml b/ovn-sb.xml > index 82167c4..9635dcc 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -2114,6 +2114,40 @@ tcp.flags = RST; > > <p><b>Example:</b> <code>handle_svc_check(inport);</code></p> > </dd> > + > + <dt><code><var>R</var> = select(<var>N1</var>[=<var>W1</var>], <var>N2</var>[=<var>W2</var>], ...);</code></dt> > + <dd> > + <p> > + <b>Parameters</b>: Integer <var>N1</var>, <var>N2</var>..., with > + optional weight <var>W1</var>, <var>W2</var>, ... > + </p> > + > + <p> > + <b>Result</b>: stored to a logical field or subfield <var>R</var>. > + </p> > + > + <p> > + Select from a list of integers <var>N1</var>, <var>N2</var>..., > + each within the range 0 ~ 65535, and store the selected one in the > + field <var>R</var>. There must be 2 or more integers listed, each > + with an optional weight, which is an integer within the range 1 ~ > + 65535. If weight is not specified, it defaults to 100. The > + selection method is based on the 5-tuple hash of packet header. > + </p> > + > + <p> > + Processing automatically moves on to the next table, as if > + <code>next;</code> were specified. The <code>select</code> action > + must be put as the last action of the logical flow when there are > + multiple actions (actions put after <code>select</code> will not > + take effect). > + </p> > + > + <p> > + <b>Example:</b> <code>reg8[16..31] = select(1=20, 2=30, 3=50);</code> > + </p> > + </dd> > + > </dl> > </column> > > diff --git a/tests/ovn.at b/tests/ovn.at > index 411b768..de80bce 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -965,14 +965,17 @@ ct_lb(); > has prereqs ip > ct_lb(192.168.1.2:80, 192.168.1.3:80); > encodes as group:1 > + uses group: id(1), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=192.168.1.2:80),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=192.168.1.3:80),commit,table=19,zone=NXM_NX_REG13[0..15])) > has prereqs ip > ct_lb(192.168.1.2, 192.168.1.3, ); > formats as ct_lb(192.168.1.2, 192.168.1.3); > encodes as group:2 > + uses group: id(2), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=192.168.1.2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=192.168.1.3),commit,table=19,zone=NXM_NX_REG13[0..15])) > has prereqs ip > ct_lb(fd0f::2, fd0f::3, ); > formats as ct_lb(fd0f::2, fd0f::3); > encodes as group:3 > + uses group: id(3), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) > has prereqs ip > > ct_lb(192.168.1.2:); > @@ -1481,6 +1484,26 @@ handle_svc_check(); > handle_svc_check(reg0); > Cannot use numeric field reg0 where string field is required. > > +# select > +reg9[16..31] = select(1=50, 2=100, 3, ); > + formats as reg9[16..31] = select(1=50, 2=100, 3=100); > + encodes as group:4 > + uses group: id(4), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19)) > + > +reg0 = select(1, 2); > + formats as reg0 = select(1=100, 2=100); > + encodes as group:5 > + uses group: id(5), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19)) > + > +reg0 = select(1=, 2); > + Syntax error at `,' expecting weight. > +reg0 = select(1=0, 2); > + Syntax error at `,' weight can't be 0. > +reg0 = select(1=123456, 2); > + Syntax error at `123456' expecting weight. > +reg0 = select(123); > + Syntax error at `;' expecting at least 2 group members. > + > # Miscellaneous negative tests. > ; > Syntax error at `;'. > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > index 5366905..476bc33 100644 > --- a/tests/test-ovn.c > +++ b/tests/test-ovn.c > @@ -1227,6 +1227,28 @@ test_expr_to_packets(struct ovs_cmdl_context *ctx OVS_UNUSED) > /* Actions. */ > > static void > +print_group_info(struct ovn_extend_table *group_table, const char *action) > +{ > + struct ovn_extend_table_info *g; > + HMAP_FOR_EACH (g, hmap_node, &group_table->desired) { > + char buf[64]; > + sprintf(buf, "group:%"PRIu32, g->table_id); > + char *match = strstr(buf, action); > + if (match) { > + if (match[strlen(buf)] != '\0') { > + /* Add ',' and match again. */ > + sprintf(buf, "group:%"PRIu32",", g->table_id); > + match = strstr(buf, action); > + } > + } > + if (match) { > + printf(" uses group: id(%"PRIu32"), name(%s)\n", > + g->table_id, g->name); > + } > + } > +} > + > +static void > test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) > { > struct shash symtab; > @@ -1305,7 +1327,9 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) > struct ds ofpacts_s = DS_EMPTY_INITIALIZER; > struct ofpact_format_params fp = { .s = &ofpacts_s }; > ofpacts_format(ofpacts.data, ofpacts.size, &fp); > - printf(" encodes as %s\n", ds_cstr(&ofpacts_s)); > + char *ofpacts_cstr = ds_cstr(&ofpacts_s); > + printf(" encodes as %s\n", ofpacts_cstr); > + print_group_info(&group_table, ofpacts_cstr); > ds_destroy(&ofpacts_s); > ofpbuf_uninit(&ofpacts); > > diff --git a/utilities/ovn-trace.8.xml b/utilities/ovn-trace.8.xml > index 01e7411..485928c 100644 > --- a/utilities/ovn-trace.8.xml > +++ b/utilities/ovn-trace.8.xml > @@ -438,6 +438,15 @@ > <code>--lb-dst</code> is not available in daemon mode. > </dd> > > + <dt><code>--select-id=</code><var>id</var></dt> > + <dd> > + Specify the <var>id</var> to be selected by the <code>select</code> > + action. <var>id</var> must be one of the values listed in the > + <code>select</code> action. Otherwise, a random id is selected from > + the list, as if <code>--select-id</code> were not specified. > + <code>--select-id</code> is not available in daemon mode. > + </dd> > + > <dt><code>--friendly-names</code></dt> > <dt><code>--no-friendly-names</code></dt> > <dd> > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > index 2645438..a97195b 100644 > --- a/utilities/ovn-trace.c > +++ b/utilities/ovn-trace.c > @@ -81,6 +81,9 @@ static size_t ct_state_idx; > /* --lb-dst: load balancer destination info. */ > static struct ovnact_ct_lb_dst lb_dst; > > +/* --select-id: "select" action member id. */ > +static uint16_t select_id; > + > /* --friendly-names, --no-friendly-names: Whether to substitute human-friendly > * port and datapath names for the awkward UUIDs typically used in the actual > * logical flows. */ > @@ -209,6 +212,16 @@ parse_lb_option(const char *s) > } > > static void > +parse_select_option(const char *s) > +{ > + unsigned long int integer = strtoul(s, NULL, 10); > + if (!integer) { > + ovs_fatal(0, "%s: bad select-id", s); > + } > + select_id = integer; > +} > + > +static void > parse_options(int argc, char *argv[]) > { > enum { > @@ -225,7 +238,8 @@ parse_options(int argc, char *argv[]) > DAEMON_OPTION_ENUMS, > SSL_OPTION_ENUMS, > VLOG_OPTION_ENUMS, > - OPT_LB_DST > + OPT_LB_DST, > + OPT_SELECT_ID > }; > static const struct option long_options[] = { > {"db", required_argument, NULL, OPT_DB}, > @@ -241,6 +255,7 @@ parse_options(int argc, char *argv[]) > {"help", no_argument, NULL, 'h'}, > {"version", no_argument, NULL, 'V'}, > {"lb-dst", required_argument, NULL, OPT_LB_DST}, > + {"select-id", required_argument, NULL, OPT_SELECT_ID}, > DAEMON_LONG_OPTIONS, > VLOG_LONG_OPTIONS, > STREAM_SSL_LONG_OPTIONS, > @@ -302,6 +317,10 @@ parse_options(int argc, char *argv[]) > parse_lb_option(optarg); > break; > > + case OPT_SELECT_ID: > + parse_select_option(optarg); > + break; > + > case 'h': > usage(); > > @@ -1991,6 +2010,46 @@ execute_ct_lb(const struct ovnact_ct_lb *ct_lb, > } > > static void > +execute_select(const struct ovnact_select *select, > + const struct ovntrace_datapath *dp, struct flow *uflow, > + enum ovnact_pipeline pipeline, struct ovs_list *super) > +{ > + struct flow select_flow = *uflow; > + > + const struct ovnact_select_dst *dst = NULL; > + ovs_assert(select->n_dsts > 1); > + bool user_specified = false; > + for (int i = 0; i < select->n_dsts; i++) { > + const struct ovnact_select_dst *d = &select->dsts[i]; > + > + /* Check for the dst specified by --select-id, if any. */ > + if (select_id == d->id) { > + dst = d; > + user_specified = true; > + break; > + } > + } > + > + if (!dst) { > + /* Select a random dst as a fallback. */ > + dst = &select->dsts[random_range(select->n_dsts)]; > + } > + > + struct mf_subfield sf = expr_resolve_field(&select->res_field); > + union mf_subvalue sv = { .be16_int = htons(dst->id) }; > + mf_write_subfield_flow(&sf, &sv, &select_flow); > + struct ds sf_str = DS_EMPTY_INITIALIZER; > + expr_field_format(&select->res_field, &sf_str); > + struct ovntrace_node *node = ovntrace_node_append( > + super, OVNTRACE_NODE_TRANSFORMATION, > + "select: %s = %"PRIu16"%s", > + ds_cstr(&sf_str), dst->id, user_specified ? "" : > + " /* Randomly selected. Use --select-id to specify. */"); > + ds_destroy(&sf_str); > + trace__(dp, &select_flow, select->ltable, pipeline, &node->subs); > +} > + > +static void > execute_log(const struct ovnact_log *log, struct flow *uflow, > struct ovs_list *super) > { > @@ -2100,6 +2159,11 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len, > execute_ct_lb(ovnact_get_CT_LB(a), dp, uflow, pipeline, super); > break; > > + case OVNACT_SELECT: > + execute_select(ovnact_get_SELECT(a), dp, uflow, > + pipeline, super); > + break; > + > case OVNACT_CT_CLEAR: > flow_clear_conntrack(uflow); > break; > -- > 2.1.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Jan 22, 2020 at 1:17 AM Numan Siddique <numans@ovn.org> wrote: > > On Tue, Jan 21, 2020 at 4:27 AM Han Zhou <hzhou@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@ovn.org> > > Hi Han, > > Thanks for v2. > > I have one comment. Please see below. > > With that addressed - Acked-by: Numan Siddique <numans@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. ;
On Wed, Jan 22, 2020 at 9:46 PM Han Zhou <hzhou@ovn.org> wrote: > > On Wed, Jan 22, 2020 at 1:17 AM Numan Siddique <numans@ovn.org> wrote: > > > > On Tue, Jan 21, 2020 at 4:27 AM Han Zhou <hzhou@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@ovn.org> > > > > Hi Han, > > > > Thanks for v2. > > > > I have one comment. Please see below. > > > > With that addressed - Acked-by: Numan Siddique <numans@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@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Jan 22, 2020 at 10:10 AM Numan Siddique <numans@ovn.org> wrote: > > On Wed, Jan 22, 2020 at 9:46 PM Han Zhou <hzhou@ovn.org> wrote: > > > > On Wed, Jan 22, 2020 at 1:17 AM Numan Siddique <numans@ovn.org> wrote: > > > > > > On Tue, Jan 21, 2020 at 4:27 AM Han Zhou <hzhou@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@ovn.org> > > > > > > Hi Han, > > > > > > Thanks for v2. > > > > > > I have one comment. Please see below. > > > > > > With that addressed - Acked-by: Numan Siddique <numans@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@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@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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; + + lexer_get(ctx->lexer); /* Skip "select". */ + lexer_get(ctx->lexer); /* Skip '('. */ + + while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) { + struct ovnact_select_dst dst; + if (!action_parse_uint16(ctx, &dst.id, "id")) { + free(dsts); + return; + } + + dst.weight = 0; + if (lexer_match(ctx->lexer, LEX_T_EQUALS)) { + if (!action_parse_uint16(ctx, &dst.weight, "weight")) { + free(dsts); + return; + } + if (dst.weight == 0) { + lexer_syntax_error(ctx->lexer, "weight can't be 0"); + } + } + lexer_match(ctx->lexer, LEX_T_COMMA); + + /* Append to dsts. */ + if (n_dsts >= allocated_dsts) { + dsts = x2nrealloc(dsts, &allocated_dsts, sizeof *dsts); + } + dsts[n_dsts++] = dst; + } + if (n_dsts <= 1) { + lexer_syntax_error(ctx->lexer, "expecting at least 2 group members"); + return; + } + + struct ovnact_select *select = ovnact_put_SELECT(ctx->ovnacts); + select->ltable = ctx->pp->cur_ltable + 1; + select->dsts = dsts; + select->n_dsts = n_dsts; + select->res_field = *res_field; +} + +static void +format_SELECT(const struct ovnact_select *select, struct ds *s) +{ + expr_field_format(&select->res_field, s); + ds_put_cstr(s, " = "); + ds_put_cstr(s, "select"); + ds_put_char(s, '('); + for (size_t i = 0; i < select->n_dsts; i++) { + if (i) { + ds_put_cstr(s, ", "); + } + + const struct ovnact_select_dst *dst = &select->dsts[i]; + ds_put_format(s, "%"PRIu16, dst->id); + ds_put_format(s, "=%"PRIu16, dst->weight ? dst->weight : 100); + } + ds_put_char(s, ')'); + ds_put_char(s, ';'); +} + +static void +encode_SELECT(const struct ovnact_select *select, + const struct ovnact_encode_params *ep, + struct ofpbuf *ofpacts) +{ + ovs_assert(select->n_dsts >= 1); + uint8_t resubmit_table = select->ltable + first_ptable(ep, ep->pipeline); + uint32_t table_id = 0; + struct ofpact_group *og; + + struct ds ds = DS_EMPTY_INITIALIZER; + ds_put_format(&ds, "type=select,selection_method=dp_hash"); + + struct mf_subfield sf = expr_resolve_field(&select->res_field); + + for (size_t bucket_id = 0; bucket_id < select->n_dsts; bucket_id++) { + const struct ovnact_select_dst *dst = &select->dsts[bucket_id]; + ds_put_format(&ds, ",bucket=bucket_id=%"PRIuSIZE",weight:%"PRIu16 + ",actions=", bucket_id, dst->weight ? dst->weight : 100); + ds_put_format(&ds, "load:%u->%s[%u..%u],", dst->id, sf.field->name, + sf.ofs, sf.ofs + sf.n_bits - 1); + ds_put_format(&ds, "resubmit(,%d)", resubmit_table); + } + + table_id = ovn_extend_table_assign_id(ep->group_table, ds_cstr(&ds), + ep->lflow_uuid); + ds_destroy(&ds); + if (table_id == EXT_TABLE_ID_INVALID) { + return; + } + + /* Create an action to set the group. */ + og = ofpact_put_GROUP(ofpacts); + og->group_id = table_id; +} + +static void +ovnact_select_free(struct ovnact_select *select) +{ + free(select->dsts); +} + +static void format_CT_CLEAR(const struct ovnact_null *null OVS_UNUSED, struct ds *s) { ds_put_cstr(s, "ct_clear;"); @@ -2868,6 +2985,9 @@ parse_set_action(struct action_context *ctx) } else if (lexer_match(ctx->lexer, LEX_T_EQUALS)) { if (ctx->lexer->token.type != LEX_T_ID) { parse_LOAD(ctx, &lhs); + } else if (!strcmp(ctx->lexer->token.s, "select") + && lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) { + parse_select_action(ctx, &lhs); } else if (!strcmp(ctx->lexer->token.s, "put_dhcp_opts") && lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) { parse_put_dhcp_opts(ctx, &lhs, ovnact_put_PUT_DHCPV4_OPTS( diff --git a/ovn-sb.xml b/ovn-sb.xml index 82167c4..9635dcc 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -2114,6 +2114,40 @@ tcp.flags = RST; <p><b>Example:</b> <code>handle_svc_check(inport);</code></p> </dd> + + <dt><code><var>R</var> = select(<var>N1</var>[=<var>W1</var>], <var>N2</var>[=<var>W2</var>], ...);</code></dt> + <dd> + <p> + <b>Parameters</b>: Integer <var>N1</var>, <var>N2</var>..., with + optional weight <var>W1</var>, <var>W2</var>, ... + </p> + + <p> + <b>Result</b>: stored to a logical field or subfield <var>R</var>. + </p> + + <p> + Select from a list of integers <var>N1</var>, <var>N2</var>..., + each within the range 0 ~ 65535, and store the selected one in the + field <var>R</var>. There must be 2 or more integers listed, each + with an optional weight, which is an integer within the range 1 ~ + 65535. If weight is not specified, it defaults to 100. The + selection method is based on the 5-tuple hash of packet header. + </p> + + <p> + Processing automatically moves on to the next table, as if + <code>next;</code> were specified. The <code>select</code> action + must be put as the last action of the logical flow when there are + multiple actions (actions put after <code>select</code> will not + take effect). + </p> + + <p> + <b>Example:</b> <code>reg8[16..31] = select(1=20, 2=30, 3=50);</code> + </p> + </dd> + </dl> </column> diff --git a/tests/ovn.at b/tests/ovn.at index 411b768..de80bce 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -965,14 +965,17 @@ ct_lb(); has prereqs ip ct_lb(192.168.1.2:80, 192.168.1.3:80); encodes as group:1 + uses group: id(1), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=192.168.1.2:80),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=192.168.1.3:80),commit,table=19,zone=NXM_NX_REG13[0..15])) has prereqs ip ct_lb(192.168.1.2, 192.168.1.3, ); formats as ct_lb(192.168.1.2, 192.168.1.3); encodes as group:2 + uses group: id(2), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=192.168.1.2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=192.168.1.3),commit,table=19,zone=NXM_NX_REG13[0..15])) has prereqs ip ct_lb(fd0f::2, fd0f::3, ); formats as ct_lb(fd0f::2, fd0f::3); encodes as group:3 + uses group: id(3), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) has prereqs ip ct_lb(192.168.1.2:); @@ -1481,6 +1484,26 @@ handle_svc_check(); handle_svc_check(reg0); Cannot use numeric field reg0 where string field is required. +# select +reg9[16..31] = select(1=50, 2=100, 3, ); + formats as reg9[16..31] = select(1=50, 2=100, 3=100); + encodes as group:4 + uses group: id(4), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19)) + +reg0 = select(1, 2); + formats as reg0 = select(1=100, 2=100); + encodes as group:5 + uses group: id(5), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19)) + +reg0 = select(1=, 2); + Syntax error at `,' expecting weight. +reg0 = select(1=0, 2); + Syntax error at `,' weight can't be 0. +reg0 = select(1=123456, 2); + Syntax error at `123456' expecting weight. +reg0 = select(123); + Syntax error at `;' expecting at least 2 group members. + # Miscellaneous negative tests. ; Syntax error at `;'. diff --git a/tests/test-ovn.c b/tests/test-ovn.c index 5366905..476bc33 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -1227,6 +1227,28 @@ test_expr_to_packets(struct ovs_cmdl_context *ctx OVS_UNUSED) /* Actions. */ static void +print_group_info(struct ovn_extend_table *group_table, const char *action) +{ + struct ovn_extend_table_info *g; + HMAP_FOR_EACH (g, hmap_node, &group_table->desired) { + char buf[64]; + sprintf(buf, "group:%"PRIu32, g->table_id); + char *match = strstr(buf, action); + if (match) { + if (match[strlen(buf)] != '\0') { + /* Add ',' and match again. */ + sprintf(buf, "group:%"PRIu32",", g->table_id); + match = strstr(buf, action); + } + } + if (match) { + printf(" uses group: id(%"PRIu32"), name(%s)\n", + g->table_id, g->name); + } + } +} + +static void test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) { struct shash symtab; @@ -1305,7 +1327,9 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) struct ds ofpacts_s = DS_EMPTY_INITIALIZER; struct ofpact_format_params fp = { .s = &ofpacts_s }; ofpacts_format(ofpacts.data, ofpacts.size, &fp); - printf(" encodes as %s\n", ds_cstr(&ofpacts_s)); + char *ofpacts_cstr = ds_cstr(&ofpacts_s); + printf(" encodes as %s\n", ofpacts_cstr); + print_group_info(&group_table, ofpacts_cstr); ds_destroy(&ofpacts_s); ofpbuf_uninit(&ofpacts); diff --git a/utilities/ovn-trace.8.xml b/utilities/ovn-trace.8.xml index 01e7411..485928c 100644 --- a/utilities/ovn-trace.8.xml +++ b/utilities/ovn-trace.8.xml @@ -438,6 +438,15 @@ <code>--lb-dst</code> is not available in daemon mode. </dd> + <dt><code>--select-id=</code><var>id</var></dt> + <dd> + Specify the <var>id</var> to be selected by the <code>select</code> + action. <var>id</var> must be one of the values listed in the + <code>select</code> action. Otherwise, a random id is selected from + the list, as if <code>--select-id</code> were not specified. + <code>--select-id</code> is not available in daemon mode. + </dd> + <dt><code>--friendly-names</code></dt> <dt><code>--no-friendly-names</code></dt> <dd> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c index 2645438..a97195b 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -81,6 +81,9 @@ static size_t ct_state_idx; /* --lb-dst: load balancer destination info. */ static struct ovnact_ct_lb_dst lb_dst; +/* --select-id: "select" action member id. */ +static uint16_t select_id; + /* --friendly-names, --no-friendly-names: Whether to substitute human-friendly * port and datapath names for the awkward UUIDs typically used in the actual * logical flows. */ @@ -209,6 +212,16 @@ parse_lb_option(const char *s) } static void +parse_select_option(const char *s) +{ + unsigned long int integer = strtoul(s, NULL, 10); + if (!integer) { + ovs_fatal(0, "%s: bad select-id", s); + } + select_id = integer; +} + +static void parse_options(int argc, char *argv[]) { enum { @@ -225,7 +238,8 @@ parse_options(int argc, char *argv[]) DAEMON_OPTION_ENUMS, SSL_OPTION_ENUMS, VLOG_OPTION_ENUMS, - OPT_LB_DST + OPT_LB_DST, + OPT_SELECT_ID }; static const struct option long_options[] = { {"db", required_argument, NULL, OPT_DB}, @@ -241,6 +255,7 @@ parse_options(int argc, char *argv[]) {"help", no_argument, NULL, 'h'}, {"version", no_argument, NULL, 'V'}, {"lb-dst", required_argument, NULL, OPT_LB_DST}, + {"select-id", required_argument, NULL, OPT_SELECT_ID}, DAEMON_LONG_OPTIONS, VLOG_LONG_OPTIONS, STREAM_SSL_LONG_OPTIONS, @@ -302,6 +317,10 @@ parse_options(int argc, char *argv[]) parse_lb_option(optarg); break; + case OPT_SELECT_ID: + parse_select_option(optarg); + break; + case 'h': usage(); @@ -1991,6 +2010,46 @@ execute_ct_lb(const struct ovnact_ct_lb *ct_lb, } static void +execute_select(const struct ovnact_select *select, + const struct ovntrace_datapath *dp, struct flow *uflow, + enum ovnact_pipeline pipeline, struct ovs_list *super) +{ + struct flow select_flow = *uflow; + + const struct ovnact_select_dst *dst = NULL; + ovs_assert(select->n_dsts > 1); + bool user_specified = false; + for (int i = 0; i < select->n_dsts; i++) { + const struct ovnact_select_dst *d = &select->dsts[i]; + + /* Check for the dst specified by --select-id, if any. */ + if (select_id == d->id) { + dst = d; + user_specified = true; + break; + } + } + + if (!dst) { + /* Select a random dst as a fallback. */ + dst = &select->dsts[random_range(select->n_dsts)]; + } + + struct mf_subfield sf = expr_resolve_field(&select->res_field); + union mf_subvalue sv = { .be16_int = htons(dst->id) }; + mf_write_subfield_flow(&sf, &sv, &select_flow); + struct ds sf_str = DS_EMPTY_INITIALIZER; + expr_field_format(&select->res_field, &sf_str); + struct ovntrace_node *node = ovntrace_node_append( + super, OVNTRACE_NODE_TRANSFORMATION, + "select: %s = %"PRIu16"%s", + ds_cstr(&sf_str), dst->id, user_specified ? "" : + " /* Randomly selected. Use --select-id to specify. */"); + ds_destroy(&sf_str); + trace__(dp, &select_flow, select->ltable, pipeline, &node->subs); +} + +static void execute_log(const struct ovnact_log *log, struct flow *uflow, struct ovs_list *super) { @@ -2100,6 +2159,11 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len, execute_ct_lb(ovnact_get_CT_LB(a), dp, uflow, pipeline, super); break; + case OVNACT_SELECT: + execute_select(ovnact_get_SELECT(a), dp, uflow, + pipeline, super); + break; + case OVNACT_CT_CLEAR: flow_clear_conntrack(uflow); break;
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@ovn.org> --- 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(-)