diff mbox series

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

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

Commit Message

Tao YunXiang May 11, 2021, 7:52 a.m. UTC
OVS uses a set of gentle mechanisms in ofproto-dpif-upcall.c to control
the number of flows in the datapath. However, this mechanism will make it
difficult for the number of datapath flows to reach a high level in a short time.

Through some tests, I found that the problem is that the flow-limit
increases too slowly, but decreases very quickly. We need adjust the
algorithm to make it increase faster as it decreases.

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/


Author: Tao YunXiang <taoyunxiang@cmss.chinamobile.com>
Signed-off-by: Tao YunXiang <taoyunxiang@cmss.chinamobile.com>
Cc: Ben Pfaff <blp@ovn.org>
---
 ofproto/ofproto-dpif-upcall.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

0-day Robot May 11, 2021, 9 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 is 85 characters long (recommended limit is 79)
#46 FILE: ofproto/ofproto-dpif-upcall.c:968:
             * when the duration is small, increase the flow-limit, and vice versa */

Lines checked: 61, Warnings: 1, Errors: 0


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

Thanks,
0-day Robot
Ben Pfaff May 11, 2021, 8 p.m. UTC | #2
On Tue, May 11, 2021 at 03:52:00PM +0800, Tao YunXiang wrote:
> Author: Tao YunXiang <taoyunxiang@cmss.chinamobile.com>

You don't need an Author: tag because Git stores the author in a
separate field.

> +            /* Use duration as a reference, adjust the number of flow_limit,
> +             * when the duration is small, increase the flow-limit, and vice versa */
> +            if (duration >= 1000) {
>                  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;
> +            } else {
> +                flow_limit *= 1000 / duration;
>              }
>              flow_limit = MIN(ofproto_flow_limit, MAX(flow_limit, 1000));
>              atomic_store_relaxed(&udpif->flow_limit, flow_limit);

The above is very abrupt.  It always tries to adjust the flow limit
upward or downward.  I think that this is a bad idea, especially in the
upward direction.  If there are only a few flows, which only take a few
milliseconds to revalidate, then it will keep increasing the flow limit
upward until it overflows the range of unsigned int.  It will happen
very quickly, in fact: if duration is 1 ms three times in a row, then we
will multiply flow_limit by 1,000,000,000 and overflow 32-bit UINT_MAX;
if it happens six times in a row, we will overflow 64-bit.

Furthermore, it doesn't work well even if we have longer durations.  If
revalidation takes 501 ms, then we can adjust the flow_limit upward, but
this won't do it.

On the downward direction, this new code does nothing if the duration is
less than 2 seconds.  We want to aim for 1-second revalidation times.

I don't think that this approach has been thought through very well.
taoyunupt May 12, 2021, 2:44 a.m. UTC | #3
At 2021-05-12 04:00:30, "Ben Pfaff" <blp@ovn.org> wrote:
>On Tue, May 11, 2021 at 03:52:00PM +0800, Tao YunXiang wrote:
>> Author: Tao YunXiang <taoyunxiang@cmss.chinamobile.com>
>
>You don't need an Author: tag because Git stores the author in a
>separate field.
>
>> +            /* Use duration as a reference, adjust the number of flow_limit,
>> +             * when the duration is small, increase the flow-limit, and vice versa */
>> +            if (duration >= 1000) {
>>                  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;
>> +            } else {
>> +                flow_limit *= 1000 / duration;
>>              }
>>              flow_limit = MIN(ofproto_flow_limit, MAX(flow_limit, 1000));
>>              atomic_store_relaxed(&udpif->flow_limit, flow_limit);
>
>The above is very abrupt.  It always tries to adjust the flow limit
>upward or downward.  I think that this is a bad idea, especially in the
>upward direction.  If there are only a few flows, which only take a few
>milliseconds to revalidate, then it will keep increasing the flow limit
>upward until it overflows the range of unsigned int.  It will happen
>very quickly, in fact: if duration is 1 ms three times in a row, then we
>will multiply flow_limit by 1,000,000,000 and overflow 32-bit UINT_MAX;
>if it happens six times in a row, we will overflow 64-bit.

>
Hi,Ben, 
Thanks for your review. 
It won't overflow, because of the following line of code(https://github.com/openvswitch/ovs/blob/master/ofproto/ofproto-dpif-upcall.c#L974),which makes it won't over   `ofproto_flow_limit` .
For example, if currently flow-limit is 200,000(the default ofproto_flow_limit), and duration is 1ms, it will become 200,000,000 ,which is much less than 32-bit UINT_MAX(4,294,967,295).  
And then,  it wil be back to 200,000,because of the limtitaion mentioned above, I think this is important.


>Furthermore, it doesn't work well even if we have longer durations.  If
>revalidation takes 501 ms, then we can adjust the flow_limit upward, but
>this won't do it.



emm...though,it won't change when `501ms`, but it will change when `500ms` or times of 2 or 5 .
If you think it's needed to make  process more linear, we can change its type from int to float. 
what do you think?


>On the downward direction, this new code does nothing if the duration is
>less than 2 seconds.  We want to aim for 1-second revalidation times.

>


In this code ,I changed the benchmark value from 2s and 1.3s to 1s. It aims for 1-second revalidation times. 
Do I misunderstand your point?  please let me know , thanks. The following is the new code. 


             if (duration >= 1000) {
                 flow_limit /= duration / 1000;
             } else {
                 flow_limit *= 1000 / duration;
             }
             flow_limit = MIN(ofproto_flow_limit, MAX(flow_limit, 1000));


>I don't think that this approach has been thought through very well.


>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff May 12, 2021, 5:20 p.m. UTC | #4
On Wed, May 12, 2021 at 10:44:12AM +0800, taoyunupt wrote:
> At 2021-05-12 04:00:30, "Ben Pfaff" <blp@ovn.org> wrote:
> >On Tue, May 11, 2021 at 03:52:00PM +0800, Tao YunXiang wrote:
> >> +            /* Use duration as a reference, adjust the number of flow_limit,
> >> +             * when the duration is small, increase the flow-limit, and vice versa */
> >> +            if (duration >= 1000) {
> >>                  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;
> >> +            } else {
> >> +                flow_limit *= 1000 / duration;
> >>              }
> >>              flow_limit = MIN(ofproto_flow_limit, MAX(flow_limit, 1000));
> >>              atomic_store_relaxed(&udpif->flow_limit, flow_limit);
> >
> >The above is very abrupt.  It always tries to adjust the flow limit
> >upward or downward.  I think that this is a bad idea, especially in the
> >upward direction.  If there are only a few flows, which only take a few
> >milliseconds to revalidate, then it will keep increasing the flow limit
> >upward until it overflows the range of unsigned int.  It will happen
> >very quickly, in fact: if duration is 1 ms three times in a row, then we
> >will multiply flow_limit by 1,000,000,000 and overflow 32-bit UINT_MAX;
> >if it happens six times in a row, we will overflow 64-bit.
> 
> >
> Hi,Ben, 
> Thanks for your review. 
> It won't overflow, because of the following line of code(https://github.com/openvswitch/ovs/blob/master/ofproto/ofproto-dpif-upcall.c#L974),which makes it won't over   `ofproto_flow_limit` .
> For example, if currently flow-limit is 200,000(the default ofproto_flow_limit), and duration is 1ms, it will become 200,000,000 ,which is much less than 32-bit UINT_MAX(4,294,967,295).  
> And then,  it wil be back to 200,000,because of the limtitaion mentioned above, I think this is important.

OK.  Is there anything that ensures that ofproto_flow_limit is no more
than UINT_MAX/1000? Otherwise we still have the problem, if
ofproto_flow_limit is set high.

> >Furthermore, it doesn't work well even if we have longer durations.  If
> >revalidation takes 501 ms, then we can adjust the flow_limit upward, but
> >this won't do it.
> 
> 
> emm...though,it won't change when `501ms`, but it will change when `500ms` or times of 2 or 5 .
> If you think it's needed to make  process more linear, we can change its type from int to float. 
> what do you think?

I think that more linear makes more sense.  Always adjusting by a factor
of 2 will probably cause oscillation.

> >On the downward direction, this new code does nothing if the duration is
> >less than 2 seconds.  We want to aim for 1-second revalidation times.
> 
> >
> 
> 
> In this code ,I changed the benchmark value from 2s and 1.3s to 1s. It aims for 1-second revalidation times. 
> Do I misunderstand your point?  please let me know , thanks. The following is the new code. 
> 
> 
>              if (duration >= 1000) {
>                  flow_limit /= duration / 1000;
>              } else {
>                  flow_limit *= 1000 / duration;
>              }
>              flow_limit = MIN(ofproto_flow_limit, MAX(flow_limit, 1000));

This will do nothing if the duration is between 501 ms and 1999 ms, and
if it's outside that range then it will always adjust by a factor of 2
or more.  I think that is too wide a range and I think that the minimum
adjustment factor is too high.
taoyunupt May 17, 2021, 7:33 a.m. UTC | #5
At 2021-05-13 01:20:29, "Ben Pfaff" <blp@ovn.org> wrote:
>On Wed, May 12, 2021 at 10:44:12AM +0800, taoyunupt wrote:
>> At 2021-05-12 04:00:30, "Ben Pfaff" <blp@ovn.org> wrote:
>> >On Tue, May 11, 2021 at 03:52:00PM +0800, Tao YunXiang wrote:
>> >> +            /* Use duration as a reference, adjust the number of flow_limit,
>> >> +             * when the duration is small, increase the flow-limit, and vice versa */
>> >> +            if (duration >= 1000) {
>> >>                  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;
>> >> +            } else {
>> >> +                flow_limit *= 1000 / duration;
>> >>              }
>> >>              flow_limit = MIN(ofproto_flow_limit, MAX(flow_limit, 1000));
>> >>              atomic_store_relaxed(&udpif->flow_limit, flow_limit);
>> >
>> >The above is very abrupt.  It always tries to adjust the flow limit
>> >upward or downward.  I think that this is a bad idea, especially in the
>> >upward direction.  If there are only a few flows, which only take a few
>> >milliseconds to revalidate, then it will keep increasing the flow limit
>> >upward until it overflows the range of unsigned int.  It will happen
>> >very quickly, in fact: if duration is 1 ms three times in a row, then we
>> >will multiply flow_limit by 1,000,000,000 and overflow 32-bit UINT_MAX;
>> >if it happens six times in a row, we will overflow 64-bit.
>> 
>> >
>> Hi,Ben, 
>> Thanks for your review. 
>> It won't overflow, because of the following line of code(https://github.com/openvswitch/ovs/blob/master/ofproto/ofproto-dpif-upcall.c#L974),which makes it won't over   `ofproto_flow_limit` .
>> For example, if currently flow-limit is 200,000(the default ofproto_flow_limit), and duration is 1ms, it will become 200,000,000 ,which is much less than 32-bit UINT_MAX(4,294,967,295).  
>> And then,  it wil be back to 200,000,because of the limtitaion mentioned above, I think this is important.
>
>OK.  Is there anything that ensures that ofproto_flow_limit is no more
>than UINT_MAX/1000? Otherwise we still have the problem, if
>ofproto_flow_limit is set high.

I sent v2 to add a limitation to avoid this.






>> >Furthermore, it doesn't work well even if we have longer durations.  If
>> >revalidation takes 501 ms, then we can adjust the flow_limit upward, but
>> >this won't do it.
>> 
>> 
>> emm...though,it won't change when `501ms`, but it will change when `500ms` or times of 2 or 5 .
>> If you think it's needed to make  process more linear, we can change its type from int to float. 
>> what do you think?
>
>I think that more linear makes more sense.  Always adjusting by a factor

>of 2 will probably cause oscillation.


I sent v2 to make adjustment more linear.






>
>> >On the downward direction, this new code does nothing if the duration is
>> >less than 2 seconds.  We want to aim for 1-second revalidation times.
>> 
>> >
>> 
>> 
>> In this code ,I changed the benchmark value from 2s and 1.3s to 1s. It aims for 1-second revalidation times. 
>> Do I misunderstand your point?  please let me know , thanks. The following is the new code. 
>> 
>> 
>>              if (duration >= 1000) {
>>                  flow_limit /= duration / 1000;
>>              } else {
>>                  flow_limit *= 1000 / duration;
>>              }
>>              flow_limit = MIN(ofproto_flow_limit, MAX(flow_limit, 1000));
>
>This will do nothing if the duration is between 501 ms and 1999 ms, and
>if it's outside that range then it will always adjust by a factor of 2
>or more.  I think that is too wide a range and I think that the minimum

>adjustment factor is too high.


I sent v2 to make adjustment more linear , I think it will avoid this situtaion you mentioned.


Thanks,
YUN
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index ccf97266c..1ae2ba5e2 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -445,7 +445,7 @@  udpif_create(struct dpif_backer *backer, struct dpif *dpif)
 
     udpif->dpif = dpif;
     udpif->backer = backer;
-    atomic_init(&udpif->flow_limit, MIN(ofproto_flow_limit, 10000));
+    atomic_init(&udpif->flow_limit, ofproto_flow_limit);
     udpif->reval_seq = seq_create();
     udpif->dump_seq = seq_create();
     latch_init(&udpif->exit_latch);
@@ -963,13 +963,13 @@  udpif_revalidator(void *arg)
 
             duration = MAX(time_msec() - start_time, 1);
             udpif->dump_duration = duration;
-            if (duration > 2000) {
+
+            /* Use duration as a reference, adjust the number of flow_limit,
+             * when the duration is small, increase the flow-limit, and vice versa */
+            if (duration >= 1000) {
                 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;
+            } else {
+                flow_limit *= 1000 / duration;
             }
             flow_limit = MIN(ofproto_flow_limit, MAX(flow_limit, 1000));
             atomic_store_relaxed(&udpif->flow_limit, flow_limit);