diff mbox series

[ovs-dev,v2] ofproto-dpif-upcall: Improve concurrency by adjust flow-limit

Message ID 20210526024416.29247-1-taoyunxiang@cmss.chinamobile.com
State New
Headers show
Series [ovs-dev,v2] ofproto-dpif-upcall: Improve concurrency by adjust flow-limit | expand

Commit Message

Tao YunXiang May 26, 2021, 2:44 a.m. UTC
OVS uses a set of gentle mechanisms to control the number of flows in 
datapath. However, it is difficult for the number of datapath flows to 
reach a high level in a short time.

Through some tests with Apache benchmark, I found the point is that
the value of flow-limit increases too slowly, but decreases very quickly. 

This change has two improvements. One is the flow-limit will adjust more 
linearly. The other one is, when the duration is small, the flow-limit will
increase faster.

Through this change, the test result of Apache benchmark can reach 12k from 5k,
the test command is as follows:

ab -n 200000 -c 500 -r http://1.1.1.2/

v1->v2: Make flow-limit changes linearly. Add a limitation to make flow_limit
won't overflow UINT_MAX.


Signed-off-by: Tao YunXiang <taoyunxiang@cmss.chinamobile.com>
Cc: Ben Pfaff <blp@ovn.org>

---
 ofproto/ofproto-dpif-upcall.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Comments

0-day Robot May 26, 2021, 2:59 a.m. UTC | #1
Bleep bloop.  Greetings Tao YunXiang, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line has trailing whitespace
#50 FILE: ofproto/ofproto-dpif-upcall.c:970:
             * and float is to make the flow_limit change more linear. */ 

WARNING: Line is 80 characters long (recommended limit is 79)
#53 FILE: ofproto/ofproto-dpif-upcall.c:973:
                flow_limit = (int) ((float) (flow_limit) / (duration / 1000.0));

Lines checked: 65, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
taoyunupt June 7, 2021, 11:33 a.m. UTC | #2
Hi ,
      Please help to review this patch. It  has passed couple of days. Hope to know your idea. Thanks.





Thanks,

YUN







At 2021-05-26 10:44:16, "Tao YunXiang" <taoyunxiang@cmss.chinamobile.com> wrote:
>OVS uses a set of gentle mechanisms to control the number of flows in 
>datapath. However, it is difficult for the number of datapath flows to 
>reach a high level in a short time.
>
>Through some tests with Apache benchmark, I found the point is that
>the value of flow-limit increases too slowly, but decreases very quickly. 
>
>This change has two improvements. One is the flow-limit will adjust more 
>linearly. The other one is, when the duration is small, the flow-limit will
>increase faster.
>
>Through this change, the test result of Apache benchmark can reach 12k from 5k,
>the test command is as follows:
>
>ab -n 200000 -c 500 -r http://1.1.1.2/
>
>v1->v2: Make flow-limit changes linearly. Add a limitation to make flow_limit
>won't overflow UINT_MAX.
>
>
>Signed-off-by: Tao YunXiang <taoyunxiang@cmss.chinamobile.com>
>Cc: Ben Pfaff <blp@ovn.org>
>
>---
> ofproto/ofproto-dpif-upcall.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
>diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>index ccf97266c..805409165 100644
>--- a/ofproto/ofproto-dpif-upcall.c
>+++ b/ofproto/ofproto-dpif-upcall.c
>@@ -963,14 +963,20 @@ udpif_revalidator(void *arg)
> 
>             duration = MAX(time_msec() - start_time, 1);
>             udpif->dump_duration = duration;
>-            if (duration > 2000) {
>-                flow_limit /= duration / 1000;
>-            } else if (duration > 1300) {
>-                flow_limit = flow_limit * 3 / 4;
>-            } else if (duration < 1000 &&
>-                       flow_limit < n_flows * 1000 / duration) {
>-                flow_limit += 1000;
>+
>+            /* Use duration as a reference, adjust the value of flow_limit,
>+             * when the duration is short, increase the flow_limit, and vice
>+             * versa. The purpose of using multiple conversions between int
>+             * and float is to make the flow_limit change more linear. */ 
>+
>+            if (duration >= 1000) {
>+                flow_limit = (int) ((float) (flow_limit) / (duration / 1000.0));
>+            } else if (flow_limit * (1000.0 / duration) >= UINT_MAX) {
>+                flow_limit = ofproto_flow_limit;
>+            } else {
>+                flow_limit = (int) (flow_limit * (1000.0 / duration));
>             }
>+
>             flow_limit = MIN(ofproto_flow_limit, MAX(flow_limit, 1000));
>             atomic_store_relaxed(&udpif->flow_limit, flow_limit);
> 
>-- 
>2.17.1
>
>
>
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff June 10, 2021, 11:08 p.m. UTC | #3
On Wed, May 26, 2021 at 10:44:16AM +0800, Tao YunXiang wrote:
> +            /* Use duration as a reference, adjust the value of flow_limit,
> +             * when the duration is short, increase the flow_limit, and vice
> +             * versa. The purpose of using multiple conversions between int
> +             * and float is to make the flow_limit change more linear. */ 
> +
> +            if (duration >= 1000) {
> +                flow_limit = (int) ((float) (flow_limit) / (duration / 1000.0));
> +            } else if (flow_limit * (1000.0 / duration) >= UINT_MAX) {
> +                flow_limit = ofproto_flow_limit;
> +            } else {
> +                flow_limit = (int) (flow_limit * (1000.0 / duration));
>              }

The casts above are pointless.  The cast to float might even be harmful,
since float normally performs worse than double on modern CPUs.  You can
remove all of the casts.

I'm still not sure that I agree with this change, since it seems like it
hasn't been tested very much.
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index ccf97266c..805409165 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -963,14 +963,20 @@  udpif_revalidator(void *arg)
 
             duration = MAX(time_msec() - start_time, 1);
             udpif->dump_duration = duration;
-            if (duration > 2000) {
-                flow_limit /= duration / 1000;
-            } else if (duration > 1300) {
-                flow_limit = flow_limit * 3 / 4;
-            } else if (duration < 1000 &&
-                       flow_limit < n_flows * 1000 / duration) {
-                flow_limit += 1000;
+
+            /* Use duration as a reference, adjust the value of flow_limit,
+             * when the duration is short, increase the flow_limit, and vice
+             * versa. The purpose of using multiple conversions between int
+             * and float is to make the flow_limit change more linear. */ 
+
+            if (duration >= 1000) {
+                flow_limit = (int) ((float) (flow_limit) / (duration / 1000.0));
+            } else if (flow_limit * (1000.0 / duration) >= UINT_MAX) {
+                flow_limit = ofproto_flow_limit;
+            } else {
+                flow_limit = (int) (flow_limit * (1000.0 / duration));
             }
+
             flow_limit = MIN(ofproto_flow_limit, MAX(flow_limit, 1000));
             atomic_store_relaxed(&udpif->flow_limit, flow_limit);