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

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(-)

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

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

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.