Skip to content

Commit 6c9c906

Browse files
igsilyaapconole
igsilya
authored and
apconole
committed
ofproto-dpif: Fix dp_hash mapping after select group modification.
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: 2e3fd24 ("ofproto-dpif: Improve dp_hash selection method for select groups") Reported-at: openvswitch/ovs-issues#357 Acked-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Aaron Conole <aconole@redhat.com>
1 parent b53b745 commit 6c9c906

File tree

2 files changed

+110
-2
lines changed

2 files changed

+110
-2
lines changed

‎ofproto/ofproto-dpif.c

+17-2
Original file line numberDiff line numberDiff line change
@@ -5205,7 +5205,6 @@ group_set_selection_method(struct group_dpif *group)
52055205
const struct ofputil_group_props *props = &group->up.props;
52065206
const char *selection_method = props->selection_method;
52075207

5208-
VLOG_DBG("Constructing select group %"PRIu32, group->up.group_id);
52095208
if (selection_method[0] == '\0') {
52105209
VLOG_DBG("No selection method specified. Trying dp_hash.");
52115210
/* If the controller has not specified a selection method, check if
@@ -5272,6 +5271,7 @@ group_construct(struct ofgroup *group_)
52725271
group_construct_stats(group);
52735272
group->hash_map = NULL;
52745273
if (group->up.type == OFPGT11_SELECT) {
5274+
VLOG_DBG("Constructing select group %"PRIu32, group->up.group_id);
52755275
group_set_selection_method(group);
52765276
}
52775277
ovs_mutex_unlock(&group->stats_mutex);
@@ -5289,6 +5289,21 @@ group_destruct(struct ofgroup *group_)
52895289
}
52905290
}
52915291

5292+
static void
5293+
group_modify(struct ofgroup *group_)
5294+
{
5295+
struct group_dpif *group = group_dpif_cast(group_);
5296+
5297+
if (group->hash_map) {
5298+
free(group->hash_map);
5299+
group->hash_map = NULL;
5300+
}
5301+
if (group->up.type == OFPGT11_SELECT) {
5302+
VLOG_DBG("Modifying select group %"PRIu32, group->up.group_id);
5303+
group_set_selection_method(group);
5304+
}
5305+
}
5306+
52925307
static enum ofperr
52935308
group_get_stats(const struct ofgroup *group_, struct ofputil_group_stats *ogs)
52945309
{
@@ -6924,7 +6939,7 @@ const struct ofproto_class ofproto_dpif_class = {
69246939
group_construct, /* group_construct */
69256940
group_destruct, /* group_destruct */
69266941
group_dealloc, /* group_dealloc */
6927-
NULL, /* group_modify */
6942+
group_modify, /* group_modify */
69286943
group_get_stats, /* group_get_stats */
69296944
get_datapath_version, /* get_datapath_version */
69306945
get_datapath_cap,

‎tests/ofproto-dpif.at

+93
Original file line numberDiff line numberDiff line change
@@ -1335,6 +1335,99 @@ m4_for([id], [1], [64], [1], [
13351335
OVS_VSWITCHD_STOP
13361336
AT_CLEANUP
13371337

1338+
AT_SETUP([ofproto-dpif - select group with dp_hash, insert/remove buckets])
1339+
1340+
OVS_VSWITCHD_START
1341+
add_of_ports br0 1 10
1342+
1343+
AT_CHECK([ovs-appctl vlog/set ofproto_dpif:file:dbg])
1344+
1345+
dnl Add a group without buckets.
1346+
AT_CHECK([ovs-ofctl -O OpenFlow15 add-group br0 \
1347+
'group_id=1235,type=select,selection_method=dp_hash'])
1348+
AT_CHECK([grep -A3 "Constructing select group 1235" ovs-vswitchd.log \
1349+
| sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
1350+
ofproto_dpif|DBG|Constructing select group 1235
1351+
ofproto_dpif|DBG|Selection method specified: dp_hash.
1352+
ofproto_dpif|DBG| Don't apply dp_hash method without buckets.
1353+
ofproto_dpif|DBG|Falling back to default hash method.
1354+
])
1355+
1356+
m4_define([OFG_BUCKET], [bucket=weight=$1,bucket_id=$1,output:10])
1357+
1358+
dnl Add two buckets one by one.
1359+
get_log_next_line_num
1360+
AT_CHECK([ovs-ofctl -O OpenFlow15 insert-buckets br0 \
1361+
group_id=1235,command_bucket_id=last,OFG_BUCKET([5])])
1362+
AT_CHECK([tail -n +$LINENUM ovs-vswitchd.log | grep -E '(Bucket|group 1235)' \
1363+
| sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
1364+
ofproto_dpif|DBG|Constructing select group 1235
1365+
ofproto_dpif|DBG| Bucket 5: weight=5, target=16.00 hits=16
1366+
ofproto_dpif|DBG|Modifying select group 1235
1367+
ofproto_dpif|DBG| Bucket 5: weight=5, target=16.00 hits=16
1368+
])
1369+
get_log_next_line_num
1370+
AT_CHECK([ovs-ofctl -O OpenFlow15 insert-buckets br0 \
1371+
group_id=1235,command_bucket_id=last,OFG_BUCKET([6])])
1372+
AT_CHECK([tail -n +$LINENUM ovs-vswitchd.log | grep -E '(Bucket|group 1235)' \
1373+
| sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
1374+
ofproto_dpif|DBG|Constructing select group 1235
1375+
ofproto_dpif|DBG| Bucket 6: weight=6, target=16.00 hits=16
1376+
ofproto_dpif|DBG|Modifying select group 1235
1377+
ofproto_dpif|DBG| Bucket 5: weight=5, target=7.27 hits=7
1378+
ofproto_dpif|DBG| Bucket 6: weight=6, target=8.73 hits=9
1379+
])
1380+
dnl Add two more in the middle.
1381+
get_log_next_line_num
1382+
AT_CHECK([ovs-ofctl -O OpenFlow15 insert-buckets br0 \
1383+
group_id=1235,command_bucket_id=5,OFG_BUCKET([7]),OFG_BUCKET([8])])
1384+
AT_CHECK([tail -n +$LINENUM ovs-vswitchd.log | grep -E '(Bucket|group 1235)' \
1385+
| sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
1386+
ofproto_dpif|DBG|Constructing select group 1235
1387+
ofproto_dpif|DBG| Bucket 7: weight=7, target=7.47 hits=7
1388+
ofproto_dpif|DBG| Bucket 8: weight=8, target=8.53 hits=9
1389+
ofproto_dpif|DBG|Modifying select group 1235
1390+
ofproto_dpif|DBG| Bucket 5: weight=5, target=3.08 hits=3
1391+
ofproto_dpif|DBG| Bucket 7: weight=7, target=4.31 hits=4
1392+
ofproto_dpif|DBG| Bucket 8: weight=8, target=4.92 hits=5
1393+
ofproto_dpif|DBG| Bucket 6: weight=6, target=3.69 hits=4
1394+
])
1395+
dnl Remove the last bucket.
1396+
get_log_next_line_num
1397+
AT_CHECK([ovs-ofctl -O OpenFlow15 remove-buckets br0 \
1398+
group_id=1235,command_bucket_id=last])
1399+
AT_CHECK([tail -n +$LINENUM ovs-vswitchd.log | grep -E '(Bucket|group 1235)' \
1400+
| sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
1401+
ofproto_dpif|DBG|Constructing select group 1235
1402+
ofproto_dpif|DBG|Modifying select group 1235
1403+
ofproto_dpif|DBG| Bucket 5: weight=5, target=4.00 hits=4
1404+
ofproto_dpif|DBG| Bucket 7: weight=7, target=5.60 hits=6
1405+
ofproto_dpif|DBG| Bucket 8: weight=8, target=6.40 hits=6
1406+
])
1407+
dnl Remove the one in the middle.
1408+
get_log_next_line_num
1409+
AT_CHECK([ovs-ofctl -O OpenFlow15 remove-buckets br0 \
1410+
group_id=1235,command_bucket_id=7])
1411+
AT_CHECK([tail -n +$LINENUM ovs-vswitchd.log | grep -E '(Bucket|group 1235)' \
1412+
| sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
1413+
ofproto_dpif|DBG|Constructing select group 1235
1414+
ofproto_dpif|DBG|Modifying select group 1235
1415+
ofproto_dpif|DBG| Bucket 5: weight=5, target=6.15 hits=6
1416+
ofproto_dpif|DBG| Bucket 8: weight=8, target=9.85 hits=10
1417+
])
1418+
dnl Remove all the remaining.
1419+
get_log_next_line_num
1420+
AT_CHECK([ovs-ofctl -O OpenFlow15 remove-buckets br0 \
1421+
group_id=1235,command_bucket_id=all])
1422+
AT_CHECK([tail -n +$LINENUM ovs-vswitchd.log | grep -E '(Bucket|group 1235)' \
1423+
| sed 's/^.*ofproto_dpif/ofproto_dpif/'], [0], [dnl
1424+
ofproto_dpif|DBG|Constructing select group 1235
1425+
ofproto_dpif|DBG|Modifying select group 1235
1426+
])
1427+
1428+
OVS_VSWITCHD_STOP
1429+
AT_CLEANUP
1430+
13381431
AT_SETUP([ofproto-dpif - select group with explicit dp_hash selection method])
13391432

13401433
OVS_VSWITCHD_START

0 commit comments

Comments
 (0)