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? > > 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. > > Cc: William Tu <[email protected]> > Signed-off-by: Han Zhou <[email protected]> > --- > lib/ovsdb-idl.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index ef3b97b..d8f221c 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -4631,8 +4631,7 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_, > * transaction only does writes of existing values, without making any > real > * changes, we will drop the whole transaction later in > * ovsdb_idl_txn_commit().) */ > - if (datum->keys && datum->values && > - write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column), > + if (write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column), > datum, &column->type)) { > goto discard_datum; > } > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
