At 2021-05-13 01:20:29, "Ben Pfaff" <[email protected]> wrote:
>On Wed, May 12, 2021 at 10:44:12AM +0800, taoyunupt wrote:
>> At 2021-05-12 04:00:30, "Ben Pfaff" <[email protected]> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
