On Sat, Jul 25, 2020 at 12:25 AM Mark Michelson <[email protected]> wrote:
> On 7/24/20 9:46 AM, Mark Michelson wrote: > > On 7/24/20 4:56 AM, Numan Siddique wrote: > >> > >> > >> On Fri, Jul 24, 2020 at 1:07 AM Mark Michelson <[email protected] > >> <mailto:[email protected]>> wrote: > >> > >> ct_commit allow for ct_label and ct_mark to be set within. However, > >> there are some restrictions with the current implementation: > >> > >> * It is not possible to address the indiviual bits within the > ct_mark or > >> ct_label. > >> * It is not possible to set these to the value of a register. Only > >> explicit integer setting can be used. > >> > >> With this change, ct_commit now can have arbitrary nested actions > >> inside. This makes it similar to how the "exec" option works in > OVS's > >> ct() action. > >> > >> In this commit, the only noticeable effect is that it allows for > >> slightly more expressive setting of ct_label.blocked. A future > commit > >> will take further advantage of this. > >> > >> Signed-off-by: Mark Michelson <[email protected] > >> <mailto:[email protected]>> > >> > >> > >> Hi Mark, > >> > >> I have a few comments. Please see below. > >> > >> --- > >> include/ovn/actions.h | 9 +--- > >> lib/actions.c | 109 > +++++++----------------------------------- > >> northd/ovn-northd.c | 8 ++-- > >> tests/ovn.at <http://ovn.at> | 50 +++++++++---------- > >> 4 files changed, 47 insertions(+), 129 deletions(-) > >> > >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h > >> index 0f7f4cdb8..12b7ab0df 100644 > >> --- a/include/ovn/actions.h > >> +++ b/include/ovn/actions.h > >> @@ -57,7 +57,7 @@ struct ovn_extend_table; > >> OVNACT(EXCHANGE, ovnact_move) \ > >> OVNACT(DEC_TTL, ovnact_null) \ > >> OVNACT(CT_NEXT, ovnact_ct_next) \ > >> - OVNACT(CT_COMMIT, ovnact_ct_commit) \ > >> + OVNACT(CT_COMMIT, ovnact_nest) \ > >> OVNACT(CT_DNAT, ovnact_ct_nat) \ > >> OVNACT(CT_SNAT, ovnact_ct_nat) \ > >> OVNACT(CT_LB, ovnact_ct_lb) \ > >> @@ -220,13 +220,6 @@ struct ovnact_ct_next { > >> uint8_t ltable; /* Logical table ID of next > >> table. */ > >> }; > >> > >> -/* OVNACT_CT_COMMIT. */ > >> -struct ovnact_ct_commit { > >> - struct ovnact ovnact; > >> - uint32_t ct_mark, ct_mark_mask; > >> - ovs_be128 ct_label, ct_label_mask; > >> -}; > >> - > >> /* OVNACT_CT_DNAT, OVNACT_CT_SNAT. */ > >> struct ovnact_ct_nat { > >> struct ovnact ovnact; > >> diff --git a/lib/actions.c b/lib/actions.c > >> index e14907e3d..330fd616b 100644 > >> --- a/lib/actions.c > >> +++ b/lib/actions.c > >> @@ -199,6 +199,14 @@ struct action_context { > >> > >> static void parse_actions(struct action_context *, enum lex_type > >> sentinel); > >> > >> +static void parse_nested_action(struct action_context *ctx, > >> + enum ovnact_type type, > >> + const char *prereq); > >> + > >> +static void format_nested_action(const struct ovnact_nest *on, > >> + const char *name, > >> + struct ds *s); > >> + > >> static bool > >> action_parse_field(struct action_context *ctx, > >> int n_bits, bool rw, struct expr_field *f) > >> @@ -617,125 +625,40 @@ ovnact_ct_next_free(struct ovnact_ct_next *a > >> OVS_UNUSED) > >> { > >> } > >> > >> -static void > >> -parse_ct_commit_arg(struct action_context *ctx, > >> - struct ovnact_ct_commit *cc) > >> -{ > >> - if (lexer_match_id(ctx->lexer, "ct_mark")) { > >> - if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { > >> - return; > >> - } > >> - if (ctx->lexer->token.type == LEX_T_INTEGER) { > >> - cc->ct_mark = ntohll(ctx->lexer->token.value.integer); > >> - cc->ct_mark_mask = UINT32_MAX; > >> - } else if (ctx->lexer->token.type == > LEX_T_MASKED_INTEGER) { > >> - cc->ct_mark = ntohll(ctx->lexer->token.value.integer); > >> - cc->ct_mark_mask = > ntohll(ctx->lexer->token.mask.integer); > >> - } else { > >> - lexer_syntax_error(ctx->lexer, "expecting integer"); > >> - return; > >> - } > >> - lexer_get(ctx->lexer); > >> - } else if (lexer_match_id(ctx->lexer, "ct_label")) { > >> - if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { > >> - return; > >> - } > >> - if (ctx->lexer->token.type == LEX_T_INTEGER) { > >> - cc->ct_label = ctx->lexer->token.value.be128_int; > >> - cc->ct_label_mask = OVS_BE128_MAX; > >> - } else if (ctx->lexer->token.type == > LEX_T_MASKED_INTEGER) { > >> - cc->ct_label = ctx->lexer->token.value.be128_int; > >> - cc->ct_label_mask = ctx->lexer->token.mask.be128_int; > >> - } else { > >> - lexer_syntax_error(ctx->lexer, "expecting integer"); > >> - return; > >> - } > >> - lexer_get(ctx->lexer); > >> - } else { > >> - lexer_syntax_error(ctx->lexer, NULL); > >> - } > >> -} > >> - > >> static void > >> parse_CT_COMMIT(struct action_context *ctx) > >> { > >> - add_prerequisite(ctx, "ip"); > >> - > >> - struct ovnact_ct_commit *ct_commit = > >> ovnact_put_CT_COMMIT(ctx->ovnacts); > >> - if (lexer_match(ctx->lexer, LEX_T_LPAREN)) { > >> - while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) { > >> - parse_ct_commit_arg(ctx, ct_commit); > >> - if (ctx->lexer->error) { > >> - return; > >> - } > >> - lexer_match(ctx->lexer, LEX_T_COMMA); > >> - } > >> - } > >> + parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip"); > >> > >> > >> I like the idea of allowing ct_commit to have nested actions. > >> > >> There is one issue here. The OVS ct action when used with commit, > >> allows exec > >> to set only 2 fields - ct_mark and ct_label. > >> > >> But with this patch, ovn-northd can add a logical flow like ct_commit { > >> ip4.src = 10.0.0.4} > >> and the OVN parse will not complain about it. > >> > >> I think we can have something like below to restrict the exec to only > >> ct_mark and ct_label. > >> > >> ********* > >> struct shash ct_symtab; > >> shash_init(&ct_symtab); > >> > >> expr_symtab_add_field(&ct_symtab, "ct_mark", MFF_CT_MARK, NULL, false); > >> expr_symtab_add_field(&ct_symtab, "ct_label", MFF_CT_LABEL, NULL, > false); > >> expr_symtab_add_subfield(&ct_symtab, "ct_label.blocked", NULL, > >> "ct_label[0]"); > >> > >> struct shash *orig_symtab = ctx->pp->symtab; > >> ctx->pp->symtab = &ct_symtab; > >> parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip"); > >> ctx->pp->symtab = orig_symtab; > >> > >> ********* > >> > >> This ct_symtab can be initialized in logical-fields.c too. > >> > >> This is the approach I thought of. There can be a better approach too. > >> Please feel free to propose a better one. > >> > >> But I think we should restrict the actions inside ct_commit{}. > >> > >> Thanks > >> Numan > > > > Yes, thanks for the explanation. I had misread the OVS ct() action > > documentation. I thought that exec() allowed for any arbitrary action to > > be used. But I also thought that setting ct_mark and ct_label were > > restricted to within ct(exec()) command. I see now how that is incorrect > > and I'll alter the OVN action parser to restrict what actions can be > > performed within ct_commit. > > Hi Numan, > > The method you suggested (with a few tweaks) worked well up to a point. > The problem is that limiting the number of symbols to "ct_mark" and > "ct_label" means you can't do things like: > > ct_commit { ct_label[0..47] = eth.src; }; > > because "eth.src" is an unrecognized symbol. What I realized is that I > want access to all the symbols, but I want most of them to be read-only. > So in my next version of the patch, I have introduced writeability > scopes. It gets us exactly what we want: ct_commit can only set ct_mark > and ct_label. It also gets us the bonus that you can no longer set > ct_mark and ct_label outside of ct_commit. I'll post it once I address > your comments on patch 3 in the series. > Awesome. This is even better. Looking forward to next version. Thanks Numan > > > > >> > >> > >> > >> > >> } > >> > >> static void > >> -format_CT_COMMIT(const struct ovnact_ct_commit *cc, struct ds *s) > >> +format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s) > >> { > >> - ds_put_cstr(s, "ct_commit("); > >> - if (cc->ct_mark_mask) { > >> - ds_put_format(s, "ct_mark=%#"PRIx32, cc->ct_mark); > >> - if (cc->ct_mark_mask != UINT32_MAX) { > >> - ds_put_format(s, "/%#"PRIx32, cc->ct_mark_mask); > >> - } > >> - } > >> - if (!ovs_be128_is_zero(cc->ct_label_mask)) { > >> - if (ds_last(s) != '(') { > >> - ds_put_cstr(s, ", "); > >> - } > >> - > >> - ds_put_format(s, "ct_label="); > >> - ds_put_hex(s, &cc->ct_label, sizeof cc->ct_label); > >> - if (!ovs_be128_equals(cc->ct_label_mask, OVS_BE128_MAX)) { > >> - ds_put_char(s, '/'); > >> - ds_put_hex(s, &cc->ct_label_mask, sizeof > >> cc->ct_label_mask); > >> - } > >> - } > >> - if (!ds_chomp(s, '(')) { > >> - ds_put_char(s, ')'); > >> - } > >> - ds_put_char(s, ';'); > >> + format_nested_action(on, "ct_commit", s); > >> } > >> > >> static void > >> -encode_CT_COMMIT(const struct ovnact_ct_commit *cc, > >> +encode_CT_COMMIT(const struct ovnact_nest *on, > >> const struct ovnact_encode_params *ep > OVS_UNUSED, > >> struct ofpbuf *ofpacts) > >> { > >> struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > >> ct->flags = NX_CT_F_COMMIT; > >> ct->recirc_table = NX_CT_RECIRC_NONE; > >> - ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); > >> + ct->zone_src.field = ep->is_switch > >> + ? mf_from_id(MFF_LOG_CT_ZONE) > >> + : mf_from_id(MFF_LOG_DNAT_ZONE); > >> ct->zone_src.ofs = 0; > >> ct->zone_src.n_bits = 16; > >> > >> size_t set_field_offset = ofpacts->size; > >> ofpbuf_pull(ofpacts, set_field_offset); > >> > >> - if (cc->ct_mark_mask) { > >> - const ovs_be32 value = htonl(cc->ct_mark); > >> - const ovs_be32 mask = htonl(cc->ct_mark_mask); > >> - ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_MARK), > >> &value, &mask); > >> - } > >> - > >> - if (!ovs_be128_is_zero(cc->ct_label_mask)) { > >> - ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_LABEL), > >> &cc->ct_label, > >> - &cc->ct_label_mask); > >> - } > >> - > >> + ovnacts_encode(on->nested, on->nested_len, ep, ofpacts); > >> ofpacts->header = ofpbuf_push_uninit(ofpacts, > set_field_offset); > >> ct = ofpacts->header; > >> ofpact_finish(ofpacts, &ct->ofpact); > >> } > >> - > >> -static void > >> -ovnact_ct_commit_free(struct ovnact_ct_commit *cc OVS_UNUSED) > >> -{ > >> -} > >> > >> static void > >> parse_ct_nat(struct action_context *ctx, const char *name, > >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > >> index 192198272..d10e5ee5d 100644 > >> --- a/northd/ovn-northd.c > >> +++ b/northd/ovn-northd.c > >> @@ -5357,7 +5357,7 @@ consider_acl(struct hmap *lflows, struct > >> ovn_datapath *od, > >> ds_clear(&match); > >> ds_clear(&actions); > >> ds_put_cstr(&match, "ct.est && ct_label.blocked == > 0"); > >> - ds_put_cstr(&actions, "ct_commit(ct_label=1/1); "); > >> + ds_put_cstr(&actions, "ct_commit { ct_label.blocked = > >> 1; }; "); > >> if (!strcmp(acl->action, "reject")) { > >> build_reject_acl_rules(od, lflows, stage, acl, > &match, > >> &actions, &acl->header_); > >> @@ -5881,9 +5881,11 @@ build_stateful(struct ovn_datapath *od, > >> struct hmap *lflows, struct hmap *lbs) > >> * any packet that makes it this far is part of a connection > we > >> * want to allow to continue. */ > >> ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, > >> - REGBIT_CONNTRACK_COMMIT" == 1", > >> "ct_commit(ct_label=0/1); next;"); > >> + REGBIT_CONNTRACK_COMMIT" == 1", > >> + "ct_commit { ct_label.blocked = 0; }; next;"); > >> ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100, > >> - REGBIT_CONNTRACK_COMMIT" == 1", > >> "ct_commit(ct_label=0/1); next;"); > >> + REGBIT_CONNTRACK_COMMIT" == 1", > >> + "ct_commit { ct_label.blocked = 0; }; next;"); > >> > >> /* If REGBIT_CONNTRACK_NAT is set as 1, then packets should > >> just be sent > >> * through nat (without committing). > >> diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at < > http://ovn.at> > >> index e19efafbe..3afdcca1e 100644 > >> --- a/tests/ovn.at <http://ovn.at> > >> +++ b/tests/ovn.at <http://ovn.at> > >> @@ -1048,51 +1048,51 @@ ct_next; > >> has prereqs ip > >> > >> # ct_commit > >> -ct_commit; > >> +ct_commit { }; > >> + formats as ct_commit { drop; }; > >> encodes as ct(commit,zone=NXM_NX_REG13[0..15]) > >> has prereqs ip > >> -ct_commit(); > >> - formats as ct_commit; > >> - encodes as ct(commit,zone=NXM_NX_REG13[0..15]) > >> - has prereqs ip > >> -ct_commit(ct_mark=1); > >> - formats as ct_commit(ct_mark=0x1); > >> +ct_commit { ct_mark=1; }; > >> + formats as ct_commit { ct_mark = 1; }; > >> encodes as > >> ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark)) > >> has prereqs ip > >> -ct_commit(ct_mark=1/1); > >> - formats as ct_commit(ct_mark=0x1/0x1); > >> +ct_commit { ct_mark=1/1; }; > >> + formats as ct_commit { ct_mark = 1/1; }; > >> encodes as > >> > ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_mark)) > >> has prereqs ip > >> -ct_commit(ct_label=1); > >> - formats as ct_commit(ct_label=0x1); > >> +ct_commit { ct_label=1; }; > >> + formats as ct_commit { ct_label = 1; }; > >> encodes as > >> ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_label)) > >> has prereqs ip > >> -ct_commit(ct_label=1/1); > >> - formats as ct_commit(ct_label=0x1/0x1); > >> +ct_commit { ct_label=1/1; }; > >> + formats as ct_commit { ct_label = 1/1; }; > >> encodes as > >> > ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_label)) > >> has prereqs ip > >> -ct_commit(ct_mark=1, ct_label=2); > >> - formats as ct_commit(ct_mark=0x1, ct_label=0x2); > >> +ct_commit { ct_mark=1; ct_label=2; }; > >> + formats as ct_commit { ct_mark = 1; ct_label = 2; }; > >> encodes as > >> > ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label)) > >> has prereqs ip > >> > >> -ct_commit(ct_label=0x01020304050607080910111213141516); > >> - formats as > ct_commit(ct_label=0x1020304050607080910111213141516); > >> +ct_commit { ct_label=0x01020304050607080910111213141516; }; > >> + formats as ct_commit { ct_label = > >> 0x1020304050607080910111213141516; }; > >> encodes as > >> > ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1020304050607080910111213141516->ct_label)) > >> has prereqs ip > >> -ct_commit(ct_label=0x181716151413121110090807060504030201); > >> - formats as > ct_commit(ct_label=0x16151413121110090807060504030201); > >> - encodes as > >> > ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x16151413121110090807060504030201->ct_label)) > >> - has prereqs ip > >> > -ct_commit(ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000); > >> +ct_commit { > >> > ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000; > >> }; > >> + formats as ct_commit { ct_label = > >> > 0x1000000000000000000000000000000/0x1000000000000000000000000000000; }; > >> encodes as > >> > ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1000000000000000000000000000000/0x1000000000000000000000000000000->ct_label)) > >> has prereqs ip > >> -ct_commit(ct_label=18446744073709551615); > >> - formats as ct_commit(ct_label=0xffffffffffffffff); > >> +ct_commit { ct_label=18446744073709551615; }; > >> + formats as ct_commit { ct_label = 18446744073709551615; }; > >> encodes as > >> > ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0xffffffffffffffff->ct_label)) > >> has prereqs ip > >> -ct_commit(ct_label=18446744073709551616); > >> +ct_commit { ct_label[0..47] = 0x00000f040201; ct_label[48..63] = > >> 0x0002; }; > >> + formats as ct_commit { ct_label[0..47] = 0xf040201; > >> ct_label[48..63] = 0x2; }; > >> + encodes as > >> > ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0xf040201/0xffffffffffff->ct_label,set_field:0x2000000000000/0xffff000000000000->ct_label)) > >> + has prereqs ip > >> +ct_commit { ct_label=18446744073709551616; }; > >> Decimal constants must be less than 2**64. > >> +ct_commit { ct_label=0x181716151413121110090807060504030201; }; > >> + 141-bit constant is not compatible with 128-bit field > ct_label. > >> > >> # ct_dnat > >> ct_dnat; > >> -- > >> 2.25.4 > >> > >> _______________________________________________ > >> dev mailing list > >> [email protected] <mailto:[email protected]> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
