In a new OVS use case with a 3rd party SDN controller,
we observe two new typical patterns that are handled
sub-optimally:
1. Degenerate select groups with a single bucket.
For such select groups the overhead of computing a dp_hash
and recirculating the packet in the datapath to look up the
bucket is superfluous.
2. Select groups with 2 equally weighted buckets
The minimum lookup table size is set to 16 slots (4 bit hash)
even in this scenario where two slots would suffice (1 bit hash).
This implies an unnecessary 8-fold increase of needed
recirculation megaflow entries in the datapath. In select group
use cases where the number of megaflows is already high(i.e.10-100K)
an additional factor 8 can push the OVS megaflow cache over the edge.
Solution:
The patch contains two changes:
1. If the select group has only a single bucket, skip dp_hash
calculation an recirculation entirely and execute the single
bucket's actions directly.
2. Do not artificially increase the size of the dp_hash lookup
table to 16 slots if the bucket configuration allows an accurate
representation with 2, 4 or 8 slots. This minimizes the number of
recirculated megaflows.
Testing:
Updated existing unit testcases and added new testcase with one
bucket in select group.
Signed-off-by: Surya Rudra
---
ofproto/ofproto-dpif-xlate.c | 5 +
ofproto/ofproto-dpif.c | 3 +--
tests/ofproto-dpif.at| 43 +++-
3 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a6f4ea334..5c41357f1 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4612,6 +4612,11 @@ pick_hash_fields_select_group(struct xlate_ctx *ctx,
struct group_dpif *group)
static struct ofputil_bucket *
pick_dp_hash_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
{
+/* Skip dp_hash recirculation in the special case of a single bucket */
+if (group->up.n_buckets == 1) {
+return group_first_live_bucket(ctx, group, 0);
+}
+
uint32_t dp_hash = ctx->xin->flow.dp_hash;
/* dp_hash value 0 is special since it means that the dp_hash has not been
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 47db9bb57..3bd2136d8 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5080,8 +5080,7 @@ group_setup_dp_hash_table(struct group_dpif *group,
size_t max_hash)
min_weight, total_weight);
uint64_t min_slots = DIV_ROUND_UP(total_weight, min_weight);
-uint64_t min_slots2 = ROUND_UP_POW2(min_slots);
-uint64_t n_hash = MAX(16, min_slots2);
+uint64_t n_hash = ROUND_UP_POW2(min_slots);
if (n_hash > MAX_SELECT_GROUP_HASH_VALUES ||
(max_hash != 0 && n_hash > max_hash)) {
VLOG_DBG(" Too many hash values required: %"PRIu64, n_hash);
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 31064ed95..e41c44d57 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -607,10 +607,31 @@ AT_CHECK([tail -1 stdout], [0],
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - select group with one bucket in action list])
+OVS_VSWITCHD_START
+add_of_ports br0 1 10
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0
'group_id=1234,type=select,bucket=weight:10,set_field:192.168.3.90->ip_src,output:10'])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip
actions=group:1234,output:10'])
+AT_CHECK([ovs-appctl ofproto/trace br0
'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,icmp_type=8,icmp_code=0'],
[0], [stdout])
+AT_CHECK([tail -10 stdout | sed 's/dp_hash=.*\/0xf/dp_hash=0x\/0xf/'], [0],
+ [group:1234
+ -> using bucket 0
+bucket 0
+set_field:192.168.3.90->ip_src
+output:10
+output:10
+
+Final flow: unchanged
+Megaflow: recirc_id=0,eth,ip,in_port=1,nw_src=192.168.0.1,nw_frag=no
+Datapath actions: set(ipv4(src=192.168.3.90)),10,set(ipv4(src=192.168.0.1)),10
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - group actions have no effect afterwards])
OVS_VSWITCHD_START
add_of_ports br0 1 10
-AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0
'group_id=1234,type=select,bucket=set_field:192.168.3.90->ip_src,output:10'])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0
'group_id=1234,type=select,bucket=weight:10,set_field:192.168.3.90->ip_src,output:10,bucket=set_field:192.168.3.90->ip_src,output:10'])
AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip
actions=group:1234,output:10'])
for d in 0 1 2 3; do
@@ -690,10 +711,10 @@ AT_CHECK([grep -A6 "Constructing select group 1234"
ovs-vswitchd.log | sed 's/^.
ofproto_dpif|DBG|Constructing select group 1234
ofproto_dpif|DBG|No selection method specified. Trying dp_hash.
ofproto_dpif|DBG| Minimum weight: 1, total weight: 2
-ofproto_dpif|DBG| Using 16 hash values:
-ofproto_dpif|DBG| Bucket 0: weight=1,
