| Message ID | 1534182499-75914-3-git-send-email-hzhou8@ebay.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | ovn-controller incremental processing | expand |
On Mon, Aug 13, 2018 at 10:48:01AM -0700, Han Zhou wrote: > There are places with the pattern: > if (!ovs_list_is_empty(&row->track_node)) { > ovs_list_remove(&row->track_node); > } > ovs_list_push_back(&row->table->track_list, > &row->track_node); > > It seems to be trying to prevent double removing the node from a list, > but actually it doesn't, and may be misleading. > > The function ovs_list_is_empty() returns true only if the node has been > initialized with ovs_list_init(). If a node is deleted but not > initialized, the check will return false and the above code will continue > deleting it again. But if a node is already initialized, calling > ovs_list_remove() is a no-op. So the check is not necessary and misleading. > > In fact there is already a double removal resulted by this code: the function > ovsdb_idl_db_clear() removes the node but it then calls ovsdb_idl_row_destroy() > immediately which removes the node again. It should not result in any real > issue yet since the list was not changed so the second removal just > assigns the pointers with same value. > > It is in fact not necessary to remove and then add back to the list, because > the purpose of the change tracking is just to tell if a row is changed > or not. So this patch removes the "check and remove" code before adding > the node to a list, but instead, adding it to the list only if it is > not in a list. This way it ensures the node is added to a list only once > in one idl loop. (ovsdb_idl_db_track_clear() will be called in each > iteration and the check ovs_list_is_empty() will return true at the first > check in next iteration). > > Signed-off-by: Han Zhou <hzhou8@ebay.com> Thanks. Applied to master.
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index b0ebe8c..4b2bc21 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -557,10 +557,6 @@ ovsdb_idl_db_clear(struct ovsdb_idl_db *db) /* No need to do anything with dst_arcs: some node has those arcs * as forward arcs and will destroy them itself. */ - if (!ovs_list_is_empty(&row->track_node)) { - ovs_list_remove(&row->track_node); - } - ovsdb_idl_row_destroy(row); } } @@ -2366,11 +2362,10 @@ ovsdb_idl_row_change__(struct ovsdb_idl_row *row, const struct json *row_json, = row->table->change_seqno[change] = row->table->db->change_seqno + 1; if (table->modes[column_idx] & OVSDB_IDL_TRACK) { - if (!ovs_list_is_empty(&row->track_node)) { - ovs_list_remove(&row->track_node); + if (ovs_list_is_empty(&row->track_node)) { + ovs_list_push_back(&row->table->track_list, + &row->track_node); } - ovs_list_push_back(&row->table->track_list, - &row->track_node); if (!row->updated) { row->updated = bitmap_allocate(class->n_columns); } @@ -2912,10 +2907,9 @@ ovsdb_idl_row_destroy(struct ovsdb_idl_row *row) = row->table->change_seqno[OVSDB_IDL_CHANGE_DELETE] = row->table->db->change_seqno + 1; } - if (!ovs_list_is_empty(&row->track_node)) { - ovs_list_remove(&row->track_node); + if (ovs_list_is_empty(&row->track_node)) { + ovs_list_push_back(&row->table->track_list, &row->track_node); } - ovs_list_push_back(&row->table->track_list, &row->track_node); } }
There are places with the pattern: if (!ovs_list_is_empty(&row->track_node)) { ovs_list_remove(&row->track_node); } ovs_list_push_back(&row->table->track_list, &row->track_node); It seems to be trying to prevent double removing the node from a list, but actually it doesn't, and may be misleading. The function ovs_list_is_empty() returns true only if the node has been initialized with ovs_list_init(). If a node is deleted but not initialized, the check will return false and the above code will continue deleting it again. But if a node is already initialized, calling ovs_list_remove() is a no-op. So the check is not necessary and misleading. In fact there is already a double removal resulted by this code: the function ovsdb_idl_db_clear() removes the node but it then calls ovsdb_idl_row_destroy() immediately which removes the node again. It should not result in any real issue yet since the list was not changed so the second removal just assigns the pointers with same value. It is in fact not necessary to remove and then add back to the list, because the purpose of the change tracking is just to tell if a row is changed or not. So this patch removes the "check and remove" code before adding the node to a list, but instead, adding it to the list only if it is not in a list. This way it ensures the node is added to a list only once in one idl loop. (ovsdb_idl_db_track_clear() will be called in each iteration and the check ovs_list_is_empty() will return true at the first check in next iteration). Signed-off-by: Han Zhou <hzhou8@ebay.com> --- lib/ovsdb-idl.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)