| 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 |
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
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.
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
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.
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 --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);
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(-)