[ovs-dev] [PATCH v2 16/26] meta-flow: Clean up masking with prerequisities checking.
Jarno Rajahalme
jarno at ovn.org
Fri Jul 29 00:56:08 UTC 2016
Change mf_are_prereqs_ok() take a flow_wildcards pointer, so that the
wildcards can be set at the same time as the prerequisiteis are
checked. This makes it easier to write more obviously correct code.
Remove the functions mf_mask_field_and_prereqs() and
mf_mask_field_and_prereqs__(), and make the callers first check the
prerequisites, while supplying 'wc' to mf_are_prereqs_ok(), and if
successful, mask the bits of the field that were read or set using
mf_mask_field_masked().
Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
---
include/openvswitch/meta-flow.h | 8 +---
lib/classifier.c | 2 +-
lib/flow.h | 45 ++++++++++++++++++++++
lib/meta-flow.c | 82 ++++++++---------------------------------
lib/nx-match.c | 26 ++++++++-----
lib/ofp-actions.c | 2 +-
lib/ofp-parse.c | 2 +-
ofproto/ofproto-dpif-xlate.c | 21 ++---------
ofproto/ofproto.c | 2 +-
utilities/ovs-ofctl.c | 6 ++-
10 files changed, 92 insertions(+), 104 deletions(-)
diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 900cd6a..10e1f77 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -2060,12 +2060,8 @@ void mf_get_mask(const struct mf_field *, const struct flow_wildcards *,
union mf_value *mask);
/* Prerequisites. */
-bool mf_are_prereqs_ok(const struct mf_field *, const struct flow *);
-void mf_mask_field_and_prereqs(const struct mf_field *,
- struct flow_wildcards *);
-void mf_mask_field_and_prereqs__(const struct mf_field *,
- const union mf_value *,
- struct flow_wildcards *);
+bool mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow,
+ struct flow_wildcards *wc);
void mf_bitmap_set_field_and_prereqs(const struct mf_field *mf, struct
mf_bitmap *bm);
diff --git a/lib/classifier.c b/lib/classifier.c
index d5fd759..3546b3d 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -1973,7 +1973,7 @@ trie_lookup(const struct cls_trie *trie, const struct flow *flow,
/* Check that current flow matches the prerequisites for the trie
* field. Some match fields are used for multiple purposes, so we
* must check that the trie is relevant for this flow. */
- if (mf_are_prereqs_ok(mf, flow)) {
+ if (mf_are_prereqs_ok(mf, flow, NULL)) {
return trie_lookup_value(&trie->root,
&((ovs_be32 *)flow)[mf->flow_be32ofs],
&plens->be32, mf->n_bits);
diff --git a/lib/flow.h b/lib/flow.h
index fd9c712..e3f88b2 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -854,11 +854,56 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const struct flow *flow)
md->ct_label = flow->ct_label;
}
+static inline bool is_vlan(const struct flow *flow,
+ struct flow_wildcards *wc)
+{
+ if (wc) {
+ WC_MASK_FIELD_MASK(wc, vlan_tci, htons(VLAN_CFI));
+ }
+ return (flow->vlan_tci & htons(VLAN_CFI)) != 0;
+}
+
static inline bool is_ip_any(const struct flow *flow)
{
return dl_type_is_ip_any(flow->dl_type);
}
+static inline bool is_tcp(const struct flow *flow,
+ struct flow_wildcards *wc)
+{
+ if (is_ip_any(flow)) {
+ if (wc) {
+ WC_MASK_FIELD(wc, nw_proto);
+ }
+ return flow->nw_proto == IPPROTO_TCP;
+ }
+ return false;
+}
+
+static inline bool is_udp(const struct flow *flow,
+ struct flow_wildcards *wc)
+{
+ if (is_ip_any(flow)) {
+ if (wc) {
+ WC_MASK_FIELD(wc, nw_proto);
+ }
+ return flow->nw_proto == IPPROTO_UDP;
+ }
+ return false;
+}
+
+static inline bool is_sctp(const struct flow *flow,
+ struct flow_wildcards *wc)
+{
+ if (is_ip_any(flow)) {
+ if (wc) {
+ WC_MASK_FIELD(wc, nw_proto);
+ }
+ return flow->nw_proto == IPPROTO_SCTP;
+ }
+ return false;
+}
+
static inline bool is_icmpv4(const struct flow *flow,
struct flow_wildcards *wc)
{
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 28079d7..1701520 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -365,99 +365,49 @@ mf_is_mask_valid(const struct mf_field *mf, const union mf_value *mask)
OVS_NOT_REACHED();
}
-/* Returns true if 'flow' meets the prerequisites for 'mf', false otherwise. */
+/* Returns true if 'flow' meets the prerequisites for 'mf', false otherwise.
+ * Sets inspected bits in 'wc', if non-NULL. */
bool
-mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow)
+mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow,
+ struct flow_wildcards *wc)
{
switch (mf->prereqs) {
case MFP_NONE:
return true;
-
case MFP_ARP:
- return (flow->dl_type == htons(ETH_TYPE_ARP) ||
- flow->dl_type == htons(ETH_TYPE_RARP));
+ return (flow->dl_type == htons(ETH_TYPE_ARP) ||
+ flow->dl_type == htons(ETH_TYPE_RARP));
case MFP_IPV4:
return flow->dl_type == htons(ETH_TYPE_IP);
case MFP_IPV6:
return flow->dl_type == htons(ETH_TYPE_IPV6);
case MFP_VLAN_VID:
- return (flow->vlan_tci & htons(VLAN_CFI)) != 0;
+ return is_vlan(flow, wc);
case MFP_MPLS:
return eth_type_mpls(flow->dl_type);
case MFP_IP_ANY:
return is_ip_any(flow);
-
case MFP_TCP:
- return is_ip_any(flow) && flow->nw_proto == IPPROTO_TCP
- && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
+ return is_tcp(flow, wc) && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
case MFP_UDP:
- return is_ip_any(flow) && flow->nw_proto == IPPROTO_UDP
- && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
+ return is_udp(flow, wc) && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
case MFP_SCTP:
- return is_ip_any(flow) && flow->nw_proto == IPPROTO_SCTP
- && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
+ return is_sctp(flow, wc) && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
case MFP_ICMPV4:
- return is_icmpv4(flow, NULL);
+ return is_icmpv4(flow, wc);
case MFP_ICMPV6:
- return is_icmpv6(flow, NULL);
-
+ return is_icmpv6(flow, wc);
case MFP_ND:
- return is_nd(flow, NULL);
+ return is_nd(flow, wc);
case MFP_ND_SOLICIT:
- return is_nd(flow, NULL) && flow->tp_src == htons(ND_NEIGHBOR_SOLICIT);
+ return is_nd(flow, wc) && flow->tp_src == htons(ND_NEIGHBOR_SOLICIT);
case MFP_ND_ADVERT:
- return is_nd(flow, NULL) && flow->tp_src == htons(ND_NEIGHBOR_ADVERT);
+ return is_nd(flow, wc) && flow->tp_src == htons(ND_NEIGHBOR_ADVERT);
}
OVS_NOT_REACHED();
}
-/* Set field and it's prerequisities in the mask.
- * This is only ever called for writeable 'mf's, but we do not make the
- * distinction here. */
-void
-mf_mask_field_and_prereqs(const struct mf_field *mf, struct flow_wildcards *wc)
-{
- mf_mask_field_and_prereqs__(mf, &exact_match_mask, wc);
-}
-
-void
-mf_mask_field_and_prereqs__(const struct mf_field *mf,
- const union mf_value *mask,
- struct flow_wildcards *wc)
-{
- mf_set_flow_value_masked(mf, &exact_match_mask, mask, &wc->masks);
-
- switch (mf->prereqs) {
- case MFP_ND:
- case MFP_ND_SOLICIT:
- case MFP_ND_ADVERT:
- WC_MASK_FIELD(wc, tp_src);
- WC_MASK_FIELD(wc, tp_dst);
- /* Fall through. */
- case MFP_TCP:
- case MFP_UDP:
- case MFP_SCTP:
- case MFP_ICMPV4:
- case MFP_ICMPV6:
- /* nw_frag always unwildcarded. */
- WC_MASK_FIELD(wc, nw_proto);
- /* Fall through. */
- case MFP_ARP:
- case MFP_IPV4:
- case MFP_IPV6:
- case MFP_MPLS:
- case MFP_IP_ANY:
- /* dl_type always unwildcarded. */
- break;
- case MFP_VLAN_VID:
- WC_MASK_FIELD_MASK(wc, vlan_tci, htons(VLAN_CFI));
- break;
- case MFP_NONE:
- break;
- }
-}
-
/* Set bits of 'bm' corresponding to the field 'mf' and it's prerequisities. */
void
mf_bitmap_set_field_and_prereqs(const struct mf_field *mf, struct mf_bitmap *bm)
@@ -2039,7 +1989,7 @@ mf_check__(const struct mf_subfield *sf, const struct flow *flow,
"of %s field %s", sf->ofs, sf->n_bits,
sf->field->n_bits, type, sf->field->name);
return OFPERR_OFPBAC_BAD_SET_LEN;
- } else if (flow && !mf_are_prereqs_ok(sf->field, flow)) {
+ } else if (flow && !mf_are_prereqs_ok(sf->field, flow, NULL)) {
VLOG_WARN_RL(&rl, "%s field %s lacks correct prerequisites",
type, sf->field->name);
return OFPERR_OFPBAC_MATCH_INCONSISTENT;
diff --git a/lib/nx-match.c b/lib/nx-match.c
index c71ff0d..297e361 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -505,7 +505,7 @@ nx_pull_raw(const uint8_t *p, unsigned int match_len, bool strict,
*cookie = value.be64;
*cookie_mask = mask.be64;
}
- } else if (!mf_are_prereqs_ok(field, &match->flow)) {
+ } else if (!mf_are_prereqs_ok(field, &match->flow, NULL)) {
error = OFPERR_OFPBMC_BAD_PREREQ;
} else if (!mf_is_all_wild(field, &match->wc)) {
error = OFPERR_OFPBMC_DUP_FIELD;
@@ -1620,16 +1620,24 @@ void
nxm_execute_reg_move(const struct ofpact_reg_move *move,
struct flow *flow, struct flow_wildcards *wc)
{
- union mf_value src_value;
- union mf_value dst_value;
+ /* A flow may wildcard nw_frag. Do nothing if the packet does not have
+ * the fields. */
+ if (mf_are_prereqs_ok(move->dst.field, flow, wc)
+ && mf_are_prereqs_ok(move->src.field, flow, wc)) {
+ union mf_value src_value;
+ union mf_value dst_value;
+ union mf_value mask;
- mf_mask_field_and_prereqs(move->dst.field, wc);
- mf_mask_field_and_prereqs(move->src.field, wc);
+ /* Should only mask the bits affected. */
+ memset(&mask, 0, sizeof mask);
+ bitwise_one(&mask, move->dst.field->n_bytes, move->dst.ofs,
+ move->src.n_bits);
+ mf_mask_field_masked(move->dst.field, &mask, wc);
- /* A flow may wildcard nw_frag. Do nothing if setting a transport
- * header field on a packet that does not have them. */
- if (mf_are_prereqs_ok(move->dst.field, flow)
- && mf_are_prereqs_ok(move->src.field, flow)) {
+ memset(&mask, 0, sizeof mask);
+ bitwise_one(&mask, move->src.field->n_bytes, move->src.ofs,
+ move->src.n_bits);
+ mf_mask_field_masked(move->src.field, &mask, wc);
mf_get_value(move->dst.field, flow, &dst_value);
mf_get_value(move->src.field, flow, &src_value);
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 4900add..456b9cb 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -6893,7 +6893,7 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a,
case OFPACT_SET_FIELD:
mf = ofpact_get_SET_FIELD(a)->field;
/* Require OXM_OF_VLAN_VID to have an existing VLAN header. */
- if (!mf_are_prereqs_ok(mf, flow) ||
+ if (!mf_are_prereqs_ok(mf, flow, NULL) ||
(mf->id == MFF_VLAN_VID && !(flow->vlan_tci & htons(VLAN_CFI)))) {
VLOG_WARN_RL(&rl, "set_field %s lacks correct prerequisities",
mf->name);
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 73072d6..8659079 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1169,7 +1169,7 @@ parse_ofp_exact_flow(struct flow *flow, struct flow_wildcards *wc,
goto exit;
}
- if (!mf_are_prereqs_ok(mf, flow)) {
+ if (!mf_are_prereqs_ok(mf, flow, NULL)) {
error = xasprintf("%s: prerequisites not met for setting %s",
s, key);
goto exit;
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index c32d9bc..1fc738d 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3517,7 +3517,7 @@ xlate_hash_fields_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
/* Only hash a field if it and its pre-requisites are present
* in the flow. */
- if (!mf_are_prereqs_ok(mf, &ctx->xin->flow)) {
+ if (!mf_are_prereqs_ok(mf, &ctx->xin->flow, ctx->wc)) {
continue;
}
@@ -4935,22 +4935,9 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
set_field = ofpact_get_SET_FIELD(a);
mf = set_field->field;
- /* Set field action only ever overwrites packet's outermost
- * applicable header fields. Do nothing if no header exists. */
- if (mf->id == MFF_VLAN_VID) {
- wc->masks.vlan_tci |= htons(VLAN_CFI);
- if (!(flow->vlan_tci & htons(VLAN_CFI))) {
- break;
- }
- } else if ((mf->id == MFF_MPLS_LABEL || mf->id == MFF_MPLS_TC)
- /* 'dl_type' is already unwildcarded. */
- && !eth_type_mpls(flow->dl_type)) {
- break;
- }
- /* A flow may wildcard nw_frag. Do nothing if setting a transport
- * header field on a packet that does not have them. */
- mf_mask_field_and_prereqs__(mf, &set_field->mask, wc);
- if (mf_are_prereqs_ok(mf, flow)) {
+ /* Set the field only if the packet actually has it. */
+ if (mf_are_prereqs_ok(mf, flow, wc)) {
+ mf_mask_field_masked(mf, &set_field->mask, wc);
mf_set_flow_value_masked(mf, &set_field->value,
&set_field->mask, flow);
}
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 7f63925..dca620c 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -7843,7 +7843,7 @@ eviction_group_hash_rule(struct rule *rule)
sf < &table->eviction_fields[table->n_eviction_fields];
sf++)
{
- if (mf_are_prereqs_ok(sf->field, &flow)) {
+ if (mf_are_prereqs_ok(sf->field, &flow, NULL)) {
union mf_value value;
mf_get_value(sf->field, &flow, &value);
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index dd82e8d..3ee7879 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -1200,8 +1200,10 @@ compare_flows(const void *afs_, const void *bfs_)
} else {
bool ina, inb;
- ina = mf_are_prereqs_ok(f, &a->flow) && !mf_is_all_wild(f, &a->wc);
- inb = mf_are_prereqs_ok(f, &b->flow) && !mf_is_all_wild(f, &b->wc);
+ ina = mf_are_prereqs_ok(f, &a->flow, NULL)
+ && !mf_is_all_wild(f, &a->wc);
+ inb = mf_are_prereqs_ok(f, &b->flow, NULL)
+ && !mf_is_all_wild(f, &b->wc);
if (ina != inb) {
/* Skip the test for sc->order, so that missing fields always
* sort to the end whether we're sorting in ascending or
--
2.1.4
More information about the dev
mailing list