[ovs-dev] [PATCH ovn v2 2/3] Use nested actions in ct_commit.
Mark Michelson
mmichels at redhat.com
Thu Jul 23 19:36:50 UTC 2020
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 <mmichels at redhat.com>
---
include/ovn/actions.h | 9 +---
lib/actions.c | 109 +++++++-----------------------------------
northd/ovn-northd.c | 8 ++--
tests/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");
}
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 b/tests/ovn.at
index e19efafbe..3afdcca1e 100644
--- a/tests/ovn.at
+++ b/tests/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
More information about the dev
mailing list