Skip to content

Commit 0a8f6be

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 f31d704 commit 0a8f6be

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
@@ -5217,7 +5217,6 @@ group_set_selection_method(struct group_dpif *group)
52175217
const struct ofputil_group_props *props = &group->up.props;
52185218
const char *selection_method = props->selection_method;
52195219

5220-
VLOG_DBG("Constructing select group %"PRIu32, group->up.group_id);
52215220
if (selection_method[0] == '\0') {
52225221
VLOG_DBG("No selection method specified. Trying dp_hash.");
52235222
/* If the controller has not specified a selection method, check if
@@ -5284,6 +5283,7 @@ group_construct(struct ofgroup *group_)
52845283
group_construct_stats(group);
52855284
group->hash_map = NULL;
52865285
if (group->up.type == OFPGT11_SELECT) {
5286+
VLOG_DBG("Constructing select group %"PRIu32, group->up.group_id);
52875287
group_set_selection_method(group);
52885288
}
52895289
ovs_mutex_unlock(&group->stats_mutex);
@@ -5301,6 +5301,21 @@ group_destruct(struct ofgroup *group_)
53015301
}
53025302
}
53035303

5304+
static void
5305+
group_modify(struct ofgroup *group_)
5306+
{
5307+
struct group_dpif *group = group_dpif_cast(group_);
5308+
5309+
if (group->hash_map) {
5310+
free(group->hash_map);
5311+
group->hash_map = NULL;
5312+
}
5313+
if (group->up.type == OFPGT11_SELECT) {
5314+
VLOG_DBG("Modifying select group %"PRIu32, group->up.group_id);
5315+
group_set_selection_method(group);
5316+
}
5317+
}
5318+
53045319
static enum ofperr
53055320
group_get_stats(const struct ofgroup *group_, struct ofputil_group_stats *ogs)
53065321
{
@@ -6988,7 +7003,7 @@ const struct ofproto_class ofproto_dpif_class = {
69887003
group_construct, /* group_construct */
69897004
group_destruct, /* group_destruct */
69907005
group_dealloc, /* group_dealloc */
6991-
NULL, /* group_modify */
7006+
group_modify, /* group_modify */
69927007
group_get_stats, /* group_get_stats */
69937008
get_datapath_version, /* get_datapath_version */
69947009
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)