From: Emeel Hakim <[email protected]>
A dp_hash select group computes a dp_hash only if the packet has none,
otherwise it uses the one that the packet carries, even if another
dp_hash select group computed it with its own hash algorithm and basis.
Make every dp_hash select group compute its own dp_hash. Mark that a
dp_hash select group used the dp_hash of the packet, and keep the mark
across re-circulations, except for the one that computes a new dp_hash,
so that a dp_hash is used only once, by the group that requested it.
Fixes: 53cc166ae5fe ("xlate: Use dp_hash for select groups.")
Signed-off-by: Emeel Hakim <[email protected]>
Signed-off-by: Eli Britstein <[email protected]>
---
ofproto/ofproto-dpif-rid.c | 2 ++
ofproto/ofproto-dpif-rid.h | 1 +
ofproto/ofproto-dpif-xlate.c | 34 +++++++++++++++++++++++++++++++---
tests/ofproto-dpif.at | 23 +++++++++++++++++++++++
4 files changed, 57 insertions(+), 3 deletions(-)
diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
index 820de1417..a407a1696 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -136,6 +136,7 @@ frozen_state_hash(const struct frozen_state *state)
sizeof state->metadata, hash);
hash = hash_boolean(state->conntracked, hash);
hash = hash_boolean(state->was_mpls, hash);
+ hash = hash_boolean(state->dp_hashed, hash);
if (state->stack && state->stack_size) {
hash = hash_bytes(state->stack, state->stack_size, hash);
}
@@ -165,6 +166,7 @@ frozen_state_equal(const struct frozen_state *a, const
struct frozen_state *b)
&& a->mirrors == b->mirrors
&& a->conntracked == b->conntracked
&& a->was_mpls == b->was_mpls
+ && a->dp_hashed == b->dp_hashed
&& ofpacts_equal(a->ofpacts, a->ofpacts_len,
b->ofpacts, b->ofpacts_len)
&& ofpacts_equal(a->action_set, a->action_set_len,
diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
index 8e9a16503..db5786d84 100644
--- a/ofproto/ofproto-dpif-rid.h
+++ b/ofproto/ofproto-dpif-rid.h
@@ -154,6 +154,7 @@ struct frozen_state {
mirror_mask_t mirrors; /* Mirrors already output. */
bool conntracked; /* Conntrack occurred prior to freeze. */
bool was_mpls; /* MPLS packet */
+ bool dp_hashed; /* dp_hash occurred prior to freeze. */
struct uuid xport_uuid; /* UUID of 1st port packet received on. */
/* Actions to be translated when thawing. */
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 4e7d6fb40..f9b9ea0bd 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -403,6 +403,11 @@ struct xlate_ctx {
* state from the datapath should be honored after thawing. */
bool conntracked;
+ /* True if a dp_hash select group has used the dp_hash of this packet.
+ * This is used to determine whether a dp_hash select group needs to
+ * compute a new dp_hash instead of using the one of the packet. */
+ bool dp_hashed;
+
/* Pointer to an embedded NAT action in a conntrack action, or NULL. */
struct ofpact_nat *ct_nat_action;
@@ -4180,6 +4185,7 @@ patch_port_output(struct xlate_ctx *ctx, const struct
xport *in_dev,
bool old_was_mpls = ctx->was_mpls;
struct flow *flow = &ctx->xin->flow;
bool old_conntrack = ctx->conntracked;
+ bool old_dp_hashed = ctx->dp_hashed;
struct xretained_state *retained_state;
struct ovs_list *old_trace = ctx->xin->trace;
ovs_version_t old_version = ctx->xin->tables_version;
@@ -4264,6 +4270,10 @@ patch_port_output(struct xlate_ctx *ctx, const struct
xport *in_dev,
* original bridge. */
ctx->conntracked = old_conntrack;
+ /* The out bridge's dp_hash select groups should have no effect on the
+ * original bridge. */
+ ctx->dp_hashed = old_dp_hashed;
+
/* The fact that the out bridge exits (for any reason) does not mean
* that the original bridge should exit. Specifically, if the out
* bridge freezes translation, the original bridge must continue
@@ -5026,8 +5036,9 @@ pick_dp_hash_select_group(struct xlate_ctx *ctx, struct
group_dpif *group)
/* dp_hash value 0 is special since it means that the dp_hash has not been
* computed, as all computed dp_hash values are non-zero. Therefore
* compare to zero can be used to decide if the dp_hash value is valid
- * without masking the dp_hash field. */
- if (!dp_hash) {
+ * without masking the dp_hash field. The value is also invalid once a
+ * dp_hash select group has used it, as each group computes its own. */
+ if (!dp_hash || ctx->dp_hashed) {
enum ovs_hash_alg hash_alg = group->hash_alg;
if (hash_alg > ctx->xbridge->support.max_hash_alg) {
/* Algorithm supported by all datapaths. */
@@ -5039,6 +5050,8 @@ pick_dp_hash_select_group(struct xlate_ctx *ctx, struct
group_dpif *group)
return NULL;
} else {
uint32_t hash_mask = group->hash_mask;
+
+ ctx->dp_hashed = true;
ctx->wc->masks.dp_hash |= hash_mask;
/* Starting from the original masked dp_hash value iterate over the
@@ -5305,6 +5318,7 @@ xlate_controller_action(struct xlate_ctx *ctx, int len,
.mirrors = ctx->mirrors,
.conntracked = ctx->conntracked,
.was_mpls = ctx->was_mpls,
+ .dp_hashed = ctx->dp_hashed,
.ofpacts = NULL,
.ofpacts_len = 0,
.action_set = NULL,
@@ -5349,6 +5363,9 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
.mirrors = ctx->mirrors,
.conntracked = ctx->conntracked,
.was_mpls = ctx->was_mpls,
+ /* No dp_hash select group has used a dp_hash computed right before
+ * the recirculation. */
+ .dp_hashed = ctx->dp_hashed && !ctx->recirc_update_dp_hash,
.xport_uuid = ctx->xin->xport_uuid,
.ofpacts = ctx->frozen_actions.data,
.ofpacts_len = ctx->frozen_actions.size,
@@ -6250,8 +6267,8 @@ clone_xlate_actions(const struct ofpact *actions, size_t
actions_len,
struct xlate_ctx *ctx, bool is_last_action,
bool group_bucket_action OVS_UNUSED)
{
+ bool old_was_mpls, old_conntracked, old_dp_hashed;
struct xretained_state *retained_state;
- bool old_was_mpls, old_conntracked;
size_t body_offset, body_size;
uint32_t old_observe_offset;
@@ -6269,6 +6286,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t
actions_len,
old_was_mpls = ctx->was_mpls;
old_conntracked = ctx->conntracked;
old_observe_offset = ctx->xout->last_observe_offset;
+ old_dp_hashed = ctx->dp_hashed;
body_offset = ctx->odp_actions->size;
@@ -6329,6 +6347,10 @@ clone_xlate_actions(const struct ofpact *actions, size_t
actions_len,
ctx->conntracked = old_conntracked;
ctx->was_mpls = old_was_mpls;
+ /* The clone's dp_hash select groups should have no effect on the original
+ * packet. */
+ ctx->dp_hashed = old_dp_hashed;
+
xretain_state_restore_and_free(ctx, retained_state);
}
@@ -6867,6 +6889,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx,
bool old_was_mpls = ctx->was_mpls;
bool old_conntracked = ctx->conntracked;
+ bool old_dp_hashed = ctx->dp_hashed;
size_t offset = nl_msg_start_nested(ctx->odp_actions,
OVS_ACTION_ATTR_CHECK_PKT_LEN);
@@ -6889,6 +6912,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx,
xretain_flow_restore(ctx, retained_state);
ctx->was_mpls = old_was_mpls;
ctx->conntracked = old_conntracked;
+ ctx->dp_hashed = old_dp_hashed;
/* If the flow translation for the IF_GREATER case requires freezing,
* then ctx->exit would be true. Reset to false so that we can
@@ -6913,6 +6937,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx,
ctx->was_mpls = old_was_mpls;
ctx->conntracked = old_conntracked;
+ ctx->dp_hashed = old_dp_hashed;
ctx->exit = old_exit;
xretain_base_flow_restore(ctx, retained_state);
xretain_state_restore_and_free(ctx, retained_state);
@@ -8365,6 +8390,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
*xout)
.was_mpls = false,
.conntracked = false,
+ .dp_hashed = false,
.ct_nat_action = NULL,
@@ -8431,6 +8457,8 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
*xout)
xlate_report(&ctx, OFT_THAW,
"Resuming from table %"PRIu8, ctx.table_id);
+ ctx.dp_hashed = state->dp_hashed;
+
ctx.conntracked = state->conntracked;
if (!state->conntracked) {
clear_conntrack(&ctx);
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index e50d77216..2fb3423ef 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1222,6 +1222,29 @@
recirc_id(0x1),dp_hash(0xXXXX/0xf),in_port(1),packet_type(ns=0,id=0),eth_type(0x
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - select group with dp_hash after another select group])
+
+OVS_VSWITCHD_START
+add_of_ports br0 1 10
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0
'group_id=1,type=select,bucket=group:2'])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0
'group_id=2,type=select,bucket=output:10'])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip actions=group:1'])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p1
"in_port(1),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"])
+
+dnl Group 2 computes its own dp_hash instead of using the one of group 1.
+AT_CHECK([ovs-appctl dpctl/dump-flows --names | strip_ufid | strip_used \
+ | strip_recirc | sed 's/dp_hash(.*\/0xf)/dp_hash(0xXXXX\/0xf)/' \
+ | sort], [0], [dnl
+flow-dump from the main thread:
+recirc_id(<recirc>),dp_hash(0xXXXX/0xf),in_port(p1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
packets:0, bytes:0, used:never, actions:hash(sym_l4(0)),recirc(<recirc>)
+recirc_id(<recirc>),dp_hash(0xXXXX/0xf),in_port(p1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
packets:0, bytes:0, used:never, actions:p10
+recirc_id(<recirc>),in_port(p1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
packets:0, bytes:0, used:never, actions:hash(sym_l4(0)),recirc(<recirc>)
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - select group with weights])
# Helper function to check the accuracy of distribution of packets over buckets
--
2.43.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
