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

[Ben Pfaff]

On Wed, May 12, 2021 at 10:44:12AM +0800, taoyunupt wrote:
> 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:
> >> +            /* 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.