On 8/11/20 9:08 PM, Han Zhou wrote: > > > On Tue, Aug 11, 2020 at 11:35 AM Ilya Maximets <i.maxim...@ovn.org > <mailto:i.maxim...@ovn.org>> wrote: >> >> On 8/11/20 8:15 AM, Han Zhou wrote: >> > This reverts commit 68bc6f88a3a36549fcd3b6248c25c5e2e6deb8f3. >> > The commit causes a regression in OVN scale test. ovn-northd's CPU >> > more than doubled for the test scenario: create and bind 12k ports. >> > Below are some perf data of ovn-northd when running command: >> > ovn-nbctl --wait=sb sync >> > >> > Before reverting this commit: >> > - 92.42% 0.62% ovn-northd ovn-northd [.] main >> > - 91.80% main >> > + 68.93% ovn_db_run (inlined) >> > + 22.45% ovsdb_idl_loop_commit_and_wait >> > >> > After reverting this commit: >> > - 92.84% 0.60% ovn-northd ovn-northd [.] main >> > - 92.24% main >> > + 92.03% ovn_db_run (inlined) >> > >> > Reverting this commit avoided 22.45% of the CPU caused by >> > ovsdb_idl_loop_commit_and_wait(). >> > >> > The commit changed the logic of ovsdb_idl_txn_write__() by adding >> > the check "datum->keys && datum->values" before discarding unchanged >> > data in a transaction. However, it is normal for OVSDB clients ( >> > such as ovn-northd) to try to set columns with same empty data >> > as it is before the transaction. IDL would discard these changes >> > and avoid sending big transactions to server (which would end up as >> > no-op on server side). In the ovn scale test scenario mentioned above, >> > each iteration of ovn-northd would send a transaction to server that >> > includes all rows of the huge Port_Binding table, which caused the >> > significant CPU increase of ovn-northd (and also the OVN SB DB server), >> > resulted in longer end to end latency of OVN configuration changes. >> >> Good catch! >> >> So, if the row has at least one empty column, the whole row will be sent >> as update and rejected by ovsdb-server because nothing really changed. >> Is it correct? >> > > To be more accurate, any columns that are set by the transaction (using one > of the xxx_set_xxx APIs) will be sent to server, even if they are not changed > at all. The columns that are not set by the transaction won't be sent. > The columns doesn't have to be empty, since in most cases datum->values are > empty (if it is not a map), even if the column is not empty. > The server wouldn't reject, instead, it makes no change and simply returns > success to the client. There is no correctness issue, but this impacts > performance - waste of CPU and bandwidth.
Oh. I missed that 'values' are almost always NULL since most of datum types doesn't have them. So, the issue is even worse than I though initially. > >> > >> > For the original problem the commit 68bc6f88 was trying to fix, it >> > doesn't seem to be a real problem. The NULL deref reported by >> > Coverity may be addressed in a future patch using a different approach, >> > if necessary. I looked through the code and this coverity defect looks like a false positive indeed, because ovsdb_datum_equals() or any other function here always checks for the type before using and NULL might be only in OVSDB_TYPE_VOID case or if n == 0, which is also checked. Thanks, Han! Applied to master and branch-2.14. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
