Skip to content

Commit 0b686a2

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 3dea4e0 commit 0b686a2

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
@@ -5392,7 +5392,6 @@ group_set_selection_method(struct group_dpif *group)
53925392
const struct ofputil_group_props *props = &group->up.props;
53935393
const char *selection_method = props->selection_method;
53945394

5395-
VLOG_DBG("Constructing select group %"PRIu32, group->up.group_id);
53965395
if (selection_method[0] == '\0') {
53975396
VLOG_DBG("No selection method specified. Trying dp_hash.");
53985397
/* If the controller has not specified a selection method, check if
@@ -5459,6 +5458,7 @@ group_construct(struct ofgroup *group_)
54595458
group_construct_stats(group);
54605459
group->hash_map = NULL;
54615460
if (group->up.type == OFPGT11_SELECT) {
5461+
VLOG_DBG("Constructing select group %"PRIu32, group->up.group_id);
54625462
group_set_selection_method(group);
54635463
}
54645464
ovs_mutex_unlock(&group->stats_mutex);
@@ -5476,6 +5476,21 @@ group_destruct(struct ofgroup *group_)
54765476
}
54775477
}
54785478

5479+
static void
5480+
group_modify(struct ofgroup *group_)
5481+
{
5482+
struct group_dpif *group = group_dpif_cast(group_);
5483+
5484+
if (group->hash_map) {
5485+
free(group->hash_map);
5486+
group->hash_map = NULL;
5487+
}
5488+
if (group->up.type == OFPGT11_SELECT) {
5489+
VLOG_DBG("Modifying select group %"PRIu32, group->up.group_id);
5490+
group_set_selection_method(group);
5491+
}
5492+
}
5493+
54795494
static enum ofperr
54805495
group_get_stats(const struct ofgroup *group_, struct ofputil_group_stats *ogs)
54815496
{
@@ -7357,7 +7372,7 @@ const struct ofproto_class ofproto_dpif_class = {
73577372
group_construct, /* group_construct */
73587373
group_destruct, /* group_destruct */
73597374
group_dealloc, /* group_dealloc */
7360-
NULL, /* group_modify */
7375+
group_modify, /* group_modify */
73617376
group_get_stats, /* group_get_stats */
73627377
get_datapath_version, /* get_datapath_version */
73637378
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)