| Message ID | CALDO+SYJj63i1CsdAB7Wx4bmq5jq3rh=B63rrdLrJVfVE-Xc2Q@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, Jan 22, 2016 at 03:12:08PM -0800, William Tu wrote: > I'm reading through the lib/cmap.c and I wonder if we need an extra lock at > cmap_insert_bfs(). > > When cmap_insert_bfs() found a path and started to move all elements > backward, it goes through the for loop below and calls cmap_set_bucket() > one-by-one, and eventually place the new_node. However, is it possible that > another writer at the same time inserts an element, which is among one of > the elements in the path, as a result, making the path no longer valid? No, cmap is single-writer, any writer has to hold an exclusive lock anyway.
--- a/lib/cmap.c +++ b/lib/cmap.c @@ -717,6 +717,7 @@ cmap_insert_bfs(struct cmap_impl *impl, struct cmap_node *new_node, * Move all the nodes across the path "backward". After each * step some node appears in two buckets. Thus, every node is * always visible to a concurrent search. */ +// add a lock() here
Hi, I'm reading through the lib/cmap.c and I wonder if we need an extra lock at cmap_insert_bfs(). When cmap_insert_bfs() found a path and started to move all elements backward, it goes through the for loop below and calls cmap_set_bucket() one-by-one, and eventually place the new_node. However, is it possible that another writer at the same time inserts an element, which is among one of the elements in the path, as a result, making the path no longer valid? Should we introduce another lock like below? for (k = path->n + 1; k > 0; k--) { int slot = slots[k - 1]; @@ -730,6 +731,7 @@ cmap_insert_bfs(struct cmap_impl *impl, struct cmap_node *new_node, * 'new_node'. */ cmap_set_bucket(buckets[0], slots[0], new_node, hash); +// unlock() return true; } Thanks William