| Message ID | 20220802132333.2258491-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | d1864effeb26c85242b791197ae7309f47690a9d |
| Headers | show |
| Series | [ovs-dev] ovsdb: Fix copying weak references into transaction history. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 8/2/22 15:23, Ilya Maximets wrote: > Transaction history is used only to construct row data updates for > clients, it's not used for checking data integrity, hence it doesn't > need a copy of weak references. > > Not copying this data saves a lot of CPU cycles and memory in some > cases. For example, in 250-node density-heavy scenario in ovn-heater > these references can take up to 70% of RSS, which is about 8 GB of > essentially wasted memory as reported by valgrind massif: > > ------------------------------------------------------------------------------- > n time(i) total(B) useful-heap(B) extra-heap(B) stacks(B) > ------------------------------------------------------------------------------- > 20 1,011,495,832,314 11,610,557,104 10,217,785,620 1,392,771,484 0 > > 88.00% (10,217,785,620B) (heap allocation functions) malloc/new/new[] > ->70.47% (8,181,819,064B) 0x455372: xcalloc__ (util.c:121) > ->70.07% (8,135,785,424B) 0x41609D: ovsdb_weak_ref_clone (row.c:66) > ->70.07% (8,135,785,424B) 0x41609D: ovsdb_row_clone (row.c:151) > ->34.74% (4,034,041,440B) 0x41B7C9: ovsdb_txn_clone (transaction.c:1124) > | ->34.74% (4,034,041,440B) 0x41B7C9: ovsdb_txn_add_to_history (transaction.c:1163) > | ->34.74% (4,034,041,440B) 0x41B7C9: ovsdb_txn_replay_commit (transaction.c:1198) > | ->34.74% (4,034,041,440B) 0x408C35: parse_txn (ovsdb-server.c:633) > | ->34.74% (4,034,041,440B) 0x408C35: read_db (ovsdb-server.c:663) > | ->34.74% (4,034,041,440B) 0x406C9D: main_loop (ovsdb-server.c:238) > | ->34.74% (4,034,041,440B) 0x406C9D: main (ovsdb-server.c:500) > | > ->34.74% (4,034,041,440B) 0x41B7DE: ovsdb_txn_clone (transaction.c:1125) > ->34.74% (4,034,041,440B) 0x41B7DE: ovsdb_txn_add_to_history (transaction.c:1163) > ->34.74% (4,034,041,440B) 0x41B7DE: ovsdb_txn_replay_commit (transaction.c:1198) > ->34.74% (4,034,041,440B) 0x408C35: parse_txn (ovsdb-server.c:633) > ->34.74% (4,034,041,440B) 0x408C35: read_db (ovsdb-server.c:663) > ->34.74% (4,034,041,440B) 0x406C9D: main_loop (ovsdb-server.c:238) > ->34.74% (4,034,041,440B) 0x406C9D: main (ovsdb-server.c:500) > > Replacing ovsdb_row_clone() with ovsdb_row_datum_clone() to avoid > cloning unnecessary metadata. The ovsdb_txn_clone() function re-named > to avoid issues if it will be re-used in the future for some other > use-case. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- This is great, looks good to me, thanks! Acked-by: Dumitru Ceara <dceara@redhat.com>
On 8/2/22 15:38, Dumitru Ceara wrote: > On 8/2/22 15:23, Ilya Maximets wrote: >> Transaction history is used only to construct row data updates for >> clients, it's not used for checking data integrity, hence it doesn't >> need a copy of weak references. >> >> Not copying this data saves a lot of CPU cycles and memory in some >> cases. For example, in 250-node density-heavy scenario in ovn-heater >> these references can take up to 70% of RSS, which is about 8 GB of >> essentially wasted memory as reported by valgrind massif: >> >> ------------------------------------------------------------------------------- >> n time(i) total(B) useful-heap(B) extra-heap(B) stacks(B) >> ------------------------------------------------------------------------------- >> 20 1,011,495,832,314 11,610,557,104 10,217,785,620 1,392,771,484 0 >> >> 88.00% (10,217,785,620B) (heap allocation functions) malloc/new/new[] >> ->70.47% (8,181,819,064B) 0x455372: xcalloc__ (util.c:121) >> ->70.07% (8,135,785,424B) 0x41609D: ovsdb_weak_ref_clone (row.c:66) >> ->70.07% (8,135,785,424B) 0x41609D: ovsdb_row_clone (row.c:151) >> ->34.74% (4,034,041,440B) 0x41B7C9: ovsdb_txn_clone (transaction.c:1124) >> | ->34.74% (4,034,041,440B) 0x41B7C9: ovsdb_txn_add_to_history (transaction.c:1163) >> | ->34.74% (4,034,041,440B) 0x41B7C9: ovsdb_txn_replay_commit (transaction.c:1198) >> | ->34.74% (4,034,041,440B) 0x408C35: parse_txn (ovsdb-server.c:633) >> | ->34.74% (4,034,041,440B) 0x408C35: read_db (ovsdb-server.c:663) >> | ->34.74% (4,034,041,440B) 0x406C9D: main_loop (ovsdb-server.c:238) >> | ->34.74% (4,034,041,440B) 0x406C9D: main (ovsdb-server.c:500) >> | >> ->34.74% (4,034,041,440B) 0x41B7DE: ovsdb_txn_clone (transaction.c:1125) >> ->34.74% (4,034,041,440B) 0x41B7DE: ovsdb_txn_add_to_history (transaction.c:1163) >> ->34.74% (4,034,041,440B) 0x41B7DE: ovsdb_txn_replay_commit (transaction.c:1198) >> ->34.74% (4,034,041,440B) 0x408C35: parse_txn (ovsdb-server.c:633) >> ->34.74% (4,034,041,440B) 0x408C35: read_db (ovsdb-server.c:663) >> ->34.74% (4,034,041,440B) 0x406C9D: main_loop (ovsdb-server.c:238) >> ->34.74% (4,034,041,440B) 0x406C9D: main (ovsdb-server.c:500) >> >> Replacing ovsdb_row_clone() with ovsdb_row_datum_clone() to avoid >> cloning unnecessary metadata. The ovsdb_txn_clone() function re-named >> to avoid issues if it will be re-used in the future for some other >> use-case. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- > > > This is great, looks good to me, thanks! > > Acked-by: Dumitru Ceara <dceara@redhat.com> > Applied. Thanks! Also backported to 3.0 as this seems to be an important simple fix and all the prerequisites are already in place. We could backport to 2.17 as well, but that will require some bits of other patches, so I'm not sure. Best regards, Ilya Maximets.
diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c index 6864fe099..d8d677d31 100644 --- a/ovsdb/transaction.c +++ b/ovsdb/transaction.c @@ -1098,7 +1098,7 @@ ovsdb_txn_precommit(struct ovsdb_txn *txn) } static struct ovsdb_txn* -ovsdb_txn_clone(const struct ovsdb_txn *txn) +ovsdb_txn_clone_for_history(const struct ovsdb_txn *txn) { struct ovsdb_txn *txn_cloned = xzalloc(sizeof *txn_cloned); ovs_list_init(&txn_cloned->txn_tables); @@ -1121,8 +1121,8 @@ ovsdb_txn_clone(const struct ovsdb_txn *txn) r_cloned->uuid = r->uuid; r_cloned->table = r->table; - r_cloned->old = r->old ? ovsdb_row_clone(r->old) : NULL; - r_cloned->new = r->new ? ovsdb_row_clone(r->new) : NULL; + r_cloned->old = r->old ? ovsdb_row_datum_clone(r->old) : NULL; + r_cloned->new = r->new ? ovsdb_row_datum_clone(r->new) : NULL; memcpy(&r_cloned->changed, &r->changed, bitmap_n_bytes(n_columns)); hmap_insert(&t_cloned->txn_rows, &r_cloned->hmap_node, uuid_hash(&r_cloned->uuid)); @@ -1160,7 +1160,7 @@ ovsdb_txn_add_to_history(struct ovsdb_txn *txn) { if (txn->db->need_txn_history) { struct ovsdb_txn_history_node *node = xzalloc(sizeof *node); - node->txn = ovsdb_txn_clone(txn); + node->txn = ovsdb_txn_clone_for_history(txn); ovs_list_push_back(&txn->db->txn_history, &node->node); txn->db->n_txn_history++; txn->db->n_txn_history_atoms += txn->n_atoms;
Transaction history is used only to construct row data updates for clients, it's not used for checking data integrity, hence it doesn't need a copy of weak references. Not copying this data saves a lot of CPU cycles and memory in some cases. For example, in 250-node density-heavy scenario in ovn-heater these references can take up to 70% of RSS, which is about 8 GB of essentially wasted memory as reported by valgrind massif: ------------------------------------------------------------------------------- n time(i) total(B) useful-heap(B) extra-heap(B) stacks(B) ------------------------------------------------------------------------------- 20 1,011,495,832,314 11,610,557,104 10,217,785,620 1,392,771,484 0 88.00% (10,217,785,620B) (heap allocation functions) malloc/new/new[] ->70.47% (8,181,819,064B) 0x455372: xcalloc__ (util.c:121) ->70.07% (8,135,785,424B) 0x41609D: ovsdb_weak_ref_clone (row.c:66) ->70.07% (8,135,785,424B) 0x41609D: ovsdb_row_clone (row.c:151) ->34.74% (4,034,041,440B) 0x41B7C9: ovsdb_txn_clone (transaction.c:1124) | ->34.74% (4,034,041,440B) 0x41B7C9: ovsdb_txn_add_to_history (transaction.c:1163) | ->34.74% (4,034,041,440B) 0x41B7C9: ovsdb_txn_replay_commit (transaction.c:1198) | ->34.74% (4,034,041,440B) 0x408C35: parse_txn (ovsdb-server.c:633) | ->34.74% (4,034,041,440B) 0x408C35: read_db (ovsdb-server.c:663) | ->34.74% (4,034,041,440B) 0x406C9D: main_loop (ovsdb-server.c:238) | ->34.74% (4,034,041,440B) 0x406C9D: main (ovsdb-server.c:500) | ->34.74% (4,034,041,440B) 0x41B7DE: ovsdb_txn_clone (transaction.c:1125) ->34.74% (4,034,041,440B) 0x41B7DE: ovsdb_txn_add_to_history (transaction.c:1163) ->34.74% (4,034,041,440B) 0x41B7DE: ovsdb_txn_replay_commit (transaction.c:1198) ->34.74% (4,034,041,440B) 0x408C35: parse_txn (ovsdb-server.c:633) ->34.74% (4,034,041,440B) 0x408C35: read_db (ovsdb-server.c:663) ->34.74% (4,034,041,440B) 0x406C9D: main_loop (ovsdb-server.c:238) ->34.74% (4,034,041,440B) 0x406C9D: main (ovsdb-server.c:500) Replacing ovsdb_row_clone() with ovsdb_row_datum_clone() to avoid cloning unnecessary metadata. The ovsdb_txn_clone() function re-named to avoid issues if it will be re-used in the future for some other use-case. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- ovsdb/transaction.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)