When a new bucket is inserted to an existing group, OVS creates a new
group with just that one bucket and then copies old buckets into it.
The problem is that dp_hash mappings remain initialized for that one
bucket and no traffic can be sent to any of the old buckets.
If a bucket is removed, then OVS creates an empty new group and then
copies old buckets into it, except for the removed one. Mappings are
also not updated in this case and the group behaves as if it had no
buckets at all.
We need to re-hash all the buckets after the copy of the old buckets.
ofproto-provider API already has a callback for this case, but it just
wasn't implemented for ofproto-dpif. It wasn't necessary in the past,
but it became necessary when the hash map construction was moved to
the ofproto-dpif layer.
No locking in this function is required, because the new group must
not be visible during modification. It becomes visible only after the
GROUP_MOD processing is finished.
Fixes: 2e3fd24c7c44 ("ofproto-dpif: Improve dp_hash selection method for select
groups")
Reported-at: https://github.com/openvswitch/ovs-issues/issues/357
Signed-off-by: Ilya Maximets <[email protected]>
---
ofproto/ofproto-dpif.c | 19 ++++++++-
tests/ofproto-dpif.at | 93 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 110 insertions(+), 2 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 25b1d9322..8a18233d6 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5392,7 +5392,6 @@ group_set_selection_method(struct group_dpif *group)
const struct ofputil_group_props *props = &group->up.props;
const char *selection_method = props->selection_method;
- VLOG_DBG("Constructing select group %"PRIu32, group->up.group_id);
if (selection_method[0] == '\0') {
VLOG_DBG("No selection method specified. Trying dp_hash.");
/* If the controller has not specified a selection method, check if
@@ -5459,6 +5458,7 @@ group_construct(struct ofgroup *group_)
group_construct_stats(group);
group->hash_map = NULL;
if (group->up.type == OFPGT11_SELECT) {
+ VLOG_DBG("Constructing select group %"PRIu32, group->up.group_id);
group_set_selection_method(group);
}
ovs_mutex_unlock(&group->stats_mutex);
@@ -5476,6 +5476,21 @@ group_destruct(struct ofgroup *group_)
}
}
+static void
+group_modify(struct ofgroup *group_)
+{
+ struct group_dpif *group = group_dpif_cast(group_);
+
+ if (group->hash_map) {
+ free(group->hash_map);
+ group->hash_map = NULL;
+ }
+ if (group->up.type == OFPGT11_SELECT) {
+ VLOG_DBG("Modifying select group %"PRIu32, group->up.group_id);
+ group_set_selection_method(group);
+ }
+}
+
static enum ofperr
group_get_stats(const struct ofgroup *group_, struct ofputil_group_stats *ogs)
{
@@ -7357,7 +7372,7 @@ const struct ofproto_class ofproto_dpif_class = {
group_construct, /* group_construct */
group_destruct, /* group_destruct */
group_dealloc, /* group_dealloc */
- NULL, /* group_modify */
+ group_modify, /* group_modify */
group_get_stats, /* group_get_stats */
get_datapath_version, /* get_datapath_version */
get_datapath_cap,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index dfdd50052..e79e096db 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1335,6 +1335,99 @@ m4_for([id], [1], [64], [1], [
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - select group with dp_hash, insert/remove buckets])
+
+OVS_VSWITCHD_START
+add_of_ports br0 1 10
+
+AT_CHECK([ovs-appctl vlog/set ofproto_dpif:file:dbg])
+
+dnl Add a group without buckets.
+AT_CHECK([ovs-ofctl -O OpenFlow15 add-group br0 \
+ 'group_id=1235,type=select,selection_method=dp_hash'])
+AT_CHECK([grep -A3 "Constructing select group 1235" ovs-vswitchd.log \
+ | sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
+ofproto_dpif|DBG|Constructing select group 1235
+ofproto_dpif|DBG|Selection method specified: dp_hash.
+ofproto_dpif|DBG| Don't apply dp_hash method without buckets.
+ofproto_dpif|DBG|Falling back to default hash method.
+])
+
+m4_define([OFG_BUCKET], [bucket=weight=$1,bucket_id=$1,output:10])
+
+dnl Add two buckets one by one.
+get_log_next_line_num
+AT_CHECK([ovs-ofctl -O OpenFlow15 insert-buckets br0 \
+ group_id=1235,command_bucket_id=last,OFG_BUCKET([5])])
+AT_CHECK([tail -n +$LINENUM ovs-vswitchd.log | grep -E '(Bucket|group 1235)' \
+ | sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
+ofproto_dpif|DBG|Constructing select group 1235
+ofproto_dpif|DBG| Bucket 5: weight=5, target=16.00 hits=16
+ofproto_dpif|DBG|Modifying select group 1235
+ofproto_dpif|DBG| Bucket 5: weight=5, target=16.00 hits=16
+])
+get_log_next_line_num
+AT_CHECK([ovs-ofctl -O OpenFlow15 insert-buckets br0 \
+ group_id=1235,command_bucket_id=last,OFG_BUCKET([6])])
+AT_CHECK([tail -n +$LINENUM ovs-vswitchd.log | grep -E '(Bucket|group 1235)' \
+ | sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
+ofproto_dpif|DBG|Constructing select group 1235
+ofproto_dpif|DBG| Bucket 6: weight=6, target=16.00 hits=16
+ofproto_dpif|DBG|Modifying select group 1235
+ofproto_dpif|DBG| Bucket 5: weight=5, target=7.27 hits=7
+ofproto_dpif|DBG| Bucket 6: weight=6, target=8.73 hits=9
+])
+dnl Add two more in the middle.
+get_log_next_line_num
+AT_CHECK([ovs-ofctl -O OpenFlow15 insert-buckets br0 \
+ group_id=1235,command_bucket_id=5,OFG_BUCKET([7]),OFG_BUCKET([8])])
+AT_CHECK([tail -n +$LINENUM ovs-vswitchd.log | grep -E '(Bucket|group 1235)' \
+ | sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
+ofproto_dpif|DBG|Constructing select group 1235
+ofproto_dpif|DBG| Bucket 7: weight=7, target=7.47 hits=7
+ofproto_dpif|DBG| Bucket 8: weight=8, target=8.53 hits=9
+ofproto_dpif|DBG|Modifying select group 1235
+ofproto_dpif|DBG| Bucket 5: weight=5, target=3.08 hits=3
+ofproto_dpif|DBG| Bucket 7: weight=7, target=4.31 hits=4
+ofproto_dpif|DBG| Bucket 8: weight=8, target=4.92 hits=5
+ofproto_dpif|DBG| Bucket 6: weight=6, target=3.69 hits=4
+])
+dnl Remove the last bucket.
+get_log_next_line_num
+AT_CHECK([ovs-ofctl -O OpenFlow15 remove-buckets br0 \
+ group_id=1235,command_bucket_id=last])
+AT_CHECK([tail -n +$LINENUM ovs-vswitchd.log | grep -E '(Bucket|group 1235)' \
+ | sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
+ofproto_dpif|DBG|Constructing select group 1235
+ofproto_dpif|DBG|Modifying select group 1235
+ofproto_dpif|DBG| Bucket 5: weight=5, target=4.00 hits=4
+ofproto_dpif|DBG| Bucket 7: weight=7, target=5.60 hits=6
+ofproto_dpif|DBG| Bucket 8: weight=8, target=6.40 hits=6
+])
+dnl Remove the one in the middle.
+get_log_next_line_num
+AT_CHECK([ovs-ofctl -O OpenFlow15 remove-buckets br0 \
+ group_id=1235,command_bucket_id=7])
+AT_CHECK([tail -n +$LINENUM ovs-vswitchd.log | grep -E '(Bucket|group 1235)' \
+ | sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
+ofproto_dpif|DBG|Constructing select group 1235
+ofproto_dpif|DBG|Modifying select group 1235
+ofproto_dpif|DBG| Bucket 5: weight=5, target=6.15 hits=6
+ofproto_dpif|DBG| Bucket 8: weight=8, target=9.85 hits=10
+])
+dnl Remove all the remaining.
+get_log_next_line_num
+AT_CHECK([ovs-ofctl -O OpenFlow15 remove-buckets br0 \
+ group_id=1235,command_bucket_id=all])
+AT_CHECK([tail -n +$LINENUM ovs-vswitchd.log | grep -E '(Bucket|group 1235)' \
+ | sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
+ofproto_dpif|DBG|Constructing select group 1235
+ofproto_dpif|DBG|Modifying select group 1235
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - select group with explicit dp_hash selection method])
OVS_VSWITCHD_START
--
2.47.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
