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