diff mbox series

[ovs-dev] ofproto-dpif-xlate: Optimize select group performance.

Message ID 20210621084510.1820-1-rudrasurya.r@acldigital.com
State Changes Requested
Headers show
Series [ovs-dev] ofproto-dpif-xlate: Optimize select group performance. | expand

Commit Message

Surya Rudra June 21, 2021, 8:45 a.m. UTC 
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 <rudrasurya.r@acldigital.com>
---
 ofproto/ofproto-dpif-xlate.c |  5 +++++
 ofproto/ofproto-dpif.c       |  3 +--
 tests/ofproto-dpif.at        | 43 +++++++++++++++++++++++++++---------
 3 files changed, 38 insertions(+), 13 deletions(-)

Comments

Ben Pfaff July 2, 2021, 9:09 p.m. UTC | #1 
On Mon, Jun 21, 2021 at 02:15:10PM +0530, Surya Rudra via dev wrote:
> 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.

This does sound like a problem, but on the other hand, the reason we
round up to 16 is to reduce the inaccuracy entailed by having to round
up to a power of 2.

Here's another idea: if the number of slots was already a power of 2,
then don't round up to 16, like this:

    /* We need at least 'min_slots' slots to accurately implement the
     * weighting, but we can only implement total weights that are a power of
     * 2.  If rounding up to a power of 2 changes the weighting, then make sure
     * we have at least 16 hash buckets so that we aren't too inaccurate. */
    uint64_t min_slots = DIV_ROUND_UP(total_weight, min_weight);
    uint64_t min_slots2 = ROUND_UP_POW2(min_slots);
    uint64_t n_hash = (min_slots == min_slots2 ? min_slots2
                       : MAX(16, min_slots2));

What do you think?
diff mbox series

Patch

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=0xXXXX\/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, target=8.00 hits=8
-ofproto_dpif|DBG|  Bucket 1: weight=1, target=8.00 hits=8
-ofproto_dpif|DBG|Use dp_hash with 16 hash values using algorithm 1.
+ofproto_dpif|DBG|  Using 2 hash values:
+ofproto_dpif|DBG|  Bucket 0: weight=1, target=1.00 hits=1
+ofproto_dpif|DBG|  Bucket 1: weight=1, target=1.00 hits=1
+ofproto_dpif|DBG|Use dp_hash with 2 hash values using algorithm 1.
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip actions=write_actions(group:1234)'])
 
@@ -706,7 +727,7 @@  for d in 0 1 2 3; do
     done
 done
 
-AT_CHECK([ovs-appctl dpctl/dump-flows | sort | strip_ufid | strip_used | check_dpflow_stats 5 2 dp_hash], [0], [dnl
+AT_CHECK([ovs-appctl dpctl/dump-flows | sort | strip_ufid | strip_used | check_dpflow_stats 1 2 dp_hash], [0], [dnl
 recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:15, bytes:1590, used:0.0s, actions:hash(sym_l4(0)),recirc(0x1)
 n_flows=ok n_buckets=ok
 ])
@@ -726,7 +747,7 @@  for d in 0 1 2 3; do
         AT_CHECK([ovs-appctl netdev-dummy/receive p1 $pkt])
 done
 
-AT_CHECK([ovs-appctl dpctl/dump-flows | sort| sed 's/dp_hash(.*\/0xf)/dp_hash(0xXXXX\/0xf)/' | strip_ufid | strip_used], [0], [dnl
+AT_CHECK([ovs-appctl dpctl/dump-flows | sort| sed 's/dp_hash(.*\/0x1)/dp_hash(0xXXXX\/0xf)/' | strip_ufid | strip_used], [0], [dnl
 flow-dump from the main thread:
 recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:3, bytes:318, used:0.0s, actions:hash(sym_l4(0)),recirc(0x1)
 recirc_id(0x1),dp_hash(0xXXXX/0xf),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:3, bytes:318, used:0.0s, actions:11
@@ -820,10 +841,10 @@  AT_CHECK([grep -A6 "Constructing select group 1234" ovs-vswitchd.log | sed 's/^.
 ofproto_dpif|DBG|Constructing select group 1234
 ofproto_dpif|DBG|Selection method specified: 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, target=8.00 hits=8
-ofproto_dpif|DBG|  Bucket 1: weight=1, target=8.00 hits=8
-ofproto_dpif|DBG|Use dp_hash with 16 hash values using algorithm 0.
+ofproto_dpif|DBG|  Using 2 hash values:
+ofproto_dpif|DBG|  Bucket 0: weight=1, target=1.00 hits=1
+ofproto_dpif|DBG|  Bucket 1: weight=1, target=1.00 hits=1
+ofproto_dpif|DBG|Use dp_hash with 2 hash values using algorithm 0.
 ])
 
 # Fall back to legacy hash with zero buckets