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

taoyunupt taoyunupt at 126.com
Wed May 12 02:44:12 UTC 2021




At 2021-05-12 04:00:30, "Ben Pfaff" <blp at ovn.org> wrote:
>On Tue, May 11, 2021 at 03:52:00PM +0800, Tao YunXiang wrote:
>> Author: Tao YunXiang <taoyunxiang at 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 at openvswitch.org
>https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list