Skip to content
This repository was archived by the owner on Mar 22, 2023. It is now read-only.

Commit edba52c

Browse files
lukaszstolarczuklukaszstolarczuk
committed
radix_tree: disable operator-- when MtMode is enabled
Concurrency issue may occur if an element is removed during decrement.
1 parent 7b8eea2 commit edba52c

File tree

3 files changed

+236
-51
lines changed

3 files changed

+236
-51
lines changed

‎include/libpmemobj++/experimental/radix_tree.hpp‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,11 +98,11 @@ struct bytes_view;
9898
* swap() invalidates all references and iterators.
9999
*
100100
* MtMode enables single-writer multiple-readers concurrency with read
101-
* uncommitted isolation. In this, mode user HAS to call runtime_initialize_mt
101+
* uncommitted isolation. In this, mode user HAS TO call runtime_initialize_mt
102102
* after each application restart and runtime_finalize_mt before destroying
103103
* radix tree.
104104
*
105-
* This has the following effects:
105+
* Enabling MtMode has the following effects:
106106
* - erase and clear does not free nodes/leaves immediately, instead they are
107107
* added to a garbage list which can be freed by calling garbage_collect()
108108
* - insert_or_assign and iterator.assign_val do not perform an in-place update,
@@ -617,7 +617,9 @@ struct radix_tree<Key, Value, BytesView, MtMode>::radix_tree_iterator {
617617
value_type &>::type;
618618
using pointer = typename std::conditional<IsConst, value_type const *,
619619
value_type *>::type;
620-
using iterator_category = std::bidirectional_iterator_tag;
620+
using iterator_category = typename std::conditional<
621+
MtMode, std::forward_iterator_tag,
622+
std::bidirectional_iterator_tag>::type;
621623

622624
radix_tree_iterator() = default;
623625
radix_tree_iterator(leaf_ptr leaf_, tree_ptr tree);
@@ -646,9 +648,13 @@ struct radix_tree<Key, Value, BytesView, MtMode>::radix_tree_iterator {
646648
void assign_val(T &&rhs);
647649

648650
radix_tree_iterator &operator++();
651+
template <bool Mt = MtMode,
652+
typename Enable = typename std::enable_if<!Mt>::type>
649653
radix_tree_iterator &operator--();
650654

651655
radix_tree_iterator operator++(int);
656+
template <bool Mt = MtMode,
657+
typename Enable = typename std::enable_if<!Mt>::type>
652658
radix_tree_iterator operator--(int);
653659

654660
template <bool C>
@@ -3200,8 +3206,15 @@ radix_tree<Key, Value, BytesView,
32003206
return true;
32013207
}
32023208

3209+
/*
3210+
* If MtMode == true it's not safe to use this operator (iterator may end up
3211+
* invalid if concurrent erase happen).
3212+
* XXX: it's not enabled in MtMode due to:
3213+
* https://github.com/pmem/libpmemobj-cpp/issues/1159
3214+
*/
32033215
template <typename Key, typename Value, typename BytesView, bool MtMode>
32043216
template <bool IsConst>
3217+
template <bool Mt, typename Enable>
32053218
typename radix_tree<Key, Value, BytesView,
32063219
MtMode>::template radix_tree_iterator<IsConst> &
32073220
radix_tree<Key, Value, BytesView,
@@ -3278,8 +3291,15 @@ radix_tree<Key, Value, BytesView,
32783291
return tmp;
32793292
}
32803293

3294+
/*
3295+
* If MtMode == true it's not safe to use this operator (iterator may end up
3296+
* invalid if concurrent erase happen).
3297+
* XXX: it's not enabled in MtMode due to:
3298+
* https://github.com/pmem/libpmemobj-cpp/issues/1159
3299+
*/
32813300
template <typename Key, typename Value, typename BytesView, bool MtMode>
32823301
template <bool IsConst>
3302+
template <bool Mt, typename Enable>
32833303
typename radix_tree<Key, Value, BytesView,
32843304
MtMode>::template radix_tree_iterator<IsConst>
32853305
radix_tree<Key, Value, BytesView,

‎tests/radix_tree/radix_concurrent_erase.cpp‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,141 @@ test_erase_find(nvobj::pool<root> &pop,
6464
UT_ASSERTeq(num_allocs(pop), 0);
6565
}
6666

67+
/* operator-- does not work well when MtMode is enabled */
68+
69+
/* Insert INITAL_ELEMENTS elements to the radix. After that concurrently try to
70+
* erase some at the end and read them (and decrement) from the other threads.
71+
*/
72+
// static void
73+
// test_erase_decrement(nvobj::pool<root> &pop,
74+
// nvobj::persistent_ptr<container_int_int_mt> &ptr)
75+
// {
76+
// size_t threads = 4;
77+
// if (On_drd)
78+
// threads = 2;
79+
80+
// init_container(pop, ptr, INITIAL_ELEMENTS);
81+
// ptr->runtime_initialize_mt();
82+
83+
// std::vector<size_t> to_erase;
84+
85+
// /* order randomly elements to remove */
86+
// auto it = ptr->begin();
87+
// while (it != ptr->end()) {
88+
// to_erase.emplace_back(it->key());
89+
// ++it;
90+
// }
91+
// std::shuffle(to_erase.begin(), to_erase.end(), generator);
92+
93+
// auto erase_f = [&] {
94+
// for (size_t i = 0; i < to_erase.size(); ++i) {
95+
// ptr->erase(key<container_int_int_mt>(to_erase[i]));
96+
// ptr->garbage_collect();
97+
// }
98+
// };
99+
100+
// auto readers_f = std::vector<std::function<void()>>{
101+
// [&] {
102+
// auto w = ptr->register_worker();
103+
104+
// for (size_t i = INITIAL_ELEMENTS - 1; i > 1; --i) {
105+
// /* reading is slower than erasing - repeat it */
106+
// for (int j = 0; j < 15; ++j) {
107+
// w.critical([&] {
108+
// auto k = key<
109+
// container_int_int_mt>(
110+
// i);
111+
// auto v = value<
112+
// container_int_int_mt>(
113+
// i);
114+
// auto it = ptr->find(k);
115+
// UT_ASSERT(it == ptr->end() ||
116+
// it->value() == v);
117+
// if (it != ptr->end()) {
118+
// auto prev = --it;
119+
// if (prev != ptr->end())
120+
// UT_ASSERT(
121+
// prev->value()
122+
// < v);
123+
// }
124+
// });
125+
// }
126+
// }
127+
// },
128+
// };
129+
130+
// parallel_modify_read(erase_f, readers_f, threads);
131+
132+
// ptr->garbage_collect_force();
133+
// UT_ASSERT(num_allocs(pop) <= 5);
134+
135+
// ptr->runtime_finalize_mt();
136+
137+
// nvobj::transaction::run(pop, [&] {
138+
// nvobj::delete_persistent<container_int_int_mt>(ptr);
139+
// });
140+
141+
// UT_ASSERTeq(num_allocs(pop), 0);
142+
// }
143+
144+
/* Insert INITAL_ELEMENTS elements to the radix. After that concurrently try to
145+
* erase some at the beginning and read them (and increment) from the other
146+
* threads.
147+
*/
148+
static void
149+
test_erase_increment(nvobj::pool<root> &pop,
150+
nvobj::persistent_ptr<container_int_int_mt> &ptr)
151+
{
152+
size_t threads = 4;
153+
if (On_drd)
154+
threads = 2;
155+
156+
init_container(pop, ptr, INITIAL_ELEMENTS);
157+
ptr->runtime_initialize_mt();
158+
159+
auto erase_f = [&] {
160+
for (size_t i = 0; i < INITIAL_ELEMENTS; ++i) {
161+
ptr->erase(key<container_int_int_mt>(i));
162+
ptr->garbage_collect();
163+
}
164+
};
165+
166+
auto readers_f = std::vector<std::function<void()>>{
167+
[&] {
168+
auto w = ptr->register_worker();
169+
170+
/* start one element ahead */
171+
for (size_t i = 1; i < INITIAL_ELEMENTS - 1; ++i) {
172+
w.critical([&] {
173+
auto k = key<container_int_int_mt>(i);
174+
auto v = value<container_int_int_mt>(i);
175+
auto it = ptr->find(k);
176+
UT_ASSERT(it == ptr->end() ||
177+
it->value() == v);
178+
if (it != ptr->end()) {
179+
auto next = ++it;
180+
UT_ASSERT(next != ptr->end());
181+
UT_ASSERT(next->key() > k);
182+
}
183+
});
184+
}
185+
},
186+
};
187+
188+
parallel_modify_read(erase_f, readers_f, threads);
189+
190+
ptr->garbage_collect_force();
191+
UT_ASSERT(num_allocs(pop) <= 4);
192+
193+
ptr->runtime_finalize_mt();
194+
195+
nvobj::transaction::run(pop, [&] {
196+
nvobj::delete_persistent<container_int_int_mt>(ptr);
197+
});
198+
199+
UT_ASSERTeq(num_allocs(pop), 0);
200+
}
201+
67202
/* Insert and erase the same element in loop for INITAL_ELEMENTS times.
68203
* Concurrently try to read this element from other threads */
69204
static void
@@ -206,6 +341,8 @@ test(int argc, char *argv[])
206341
}
207342

208343
test_erase_find(pop, pop.root()->radix_str_mt);
344+
// test_erase_decrement(pop, pop.root()->radix_int_int_mt);
345+
test_erase_increment(pop, pop.root()->radix_int_int_mt);
209346
test_write_erase_find(pop, pop.root()->radix_str_mt);
210347
test_garbage_collection(pop, pop.root()->radix_str_mt);
211348

‎tests/radix_tree/radix_concurrent_iterate.cpp‎

Lines changed: 76 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -49,17 +49,21 @@ test_write_iterate(nvobj::pool<root> &pop,
4949
}
5050
UT_ASSERTeq(cnt, INITIAL_ELEMENTS);
5151
},
52-
[&]() {
53-
size_t cnt = 0;
54-
for (auto it = --ptr->end(); it != ptr->end(); --it) {
55-
if (it->value() !=
56-
value<Container>(INITIAL_ELEMENTS)) {
57-
++cnt;
58-
}
59-
}
60-
61-
UT_ASSERTeq(cnt, INITIAL_ELEMENTS);
62-
}};
52+
/* XXX: it's disabled due to:
53+
* https://github.com/pmem/libpmemobj-cpp/issues/1159
54+
*/
55+
// [&]() {
56+
// size_t cnt = 0;
57+
// for (auto it = --ptr->end(); it != ptr->end(); --it) {
58+
// if (it->value() !=
59+
// value<Container>(INITIAL_ELEMENTS)) {
60+
// ++cnt;
61+
// }
62+
// }
63+
64+
// UT_ASSERTeq(cnt, INITIAL_ELEMENTS);
65+
// }
66+
};
6367

6468
parallel_modify_read(writer, readers, threads);
6569

@@ -250,7 +254,7 @@ test_erase_upper_lower_bounds_neighbours(
250254
std::find(middle_key.begin(), middle_key.end(), separator[0]),
251255
middle_key.end());
252256

253-
auto last = ptr->rbegin();
257+
auto last = std::next(ptr->begin(), static_cast<int>(ptr->size() - 1));
254258
auto last_key = std::string(last->key().data(), last->key().size());
255259
last_key.erase(
256260
std::find(last_key.begin(), last_key.end(), separator[0]),
@@ -275,10 +279,14 @@ test_erase_upper_lower_bounds_neighbours(
275279
auto &tmp = std::next(it)->key();
276280
keys_to_erase.emplace_back(tmp.data(), tmp.size());
277281
}
278-
if (it != ptr->begin()) {
279-
auto &tmp = std::prev(it)->key();
280-
keys_to_erase.emplace_back(tmp.data(), tmp.size());
281-
}
282+
283+
/* XXX: it's disabled due to:
284+
* https://github.com/pmem/libpmemobj-cpp/issues/1159
285+
*/
286+
// if (it != ptr->begin()) {
287+
// auto &tmp = std::prev(it)->key();
288+
// keys_to_erase.emplace_back(tmp.data(), tmp.size());
289+
// }
282290

283291
std::shuffle(keys_to_erase.begin(), keys_to_erase.end(),
284292
generator);
@@ -295,31 +303,41 @@ test_erase_upper_lower_bounds_neighbours(
295303

296304
/* There is no element
297305
bigger/equal to k. */
298-
if (lo == ptr->end())
306+
if (lo == ptr->end()) {
307+
auto last_el = ptr->begin();
308+
auto it = ptr->begin();
309+
while (it != ptr->end()) {
310+
last_el = it;
311+
it++;
312+
}
299313
UT_ASSERT(
300-
ptr->rbegin()->key() <
314+
last_el->key() <
301315
pmem::obj::string_view(
302316
k));
303-
else
317+
} else
304318
UT_ASSERT(
305319
lo->key() >=
306320
pmem::obj::string_view(
307321
k));
308322

309-
auto prev = std::prev(lo);
310-
311-
/* There is no element smaller than k.
323+
/* XXX: it's disabled due to:
324+
* https://github.com/pmem/libpmemobj-cpp/issues/1159
312325
*/
313-
if (prev == ptr->end())
314-
UT_ASSERT(
315-
ptr->begin()->key() >=
316-
pmem::obj::string_view(
317-
k));
318-
else
319-
UT_ASSERT(
320-
prev->key() <
321-
pmem::obj::string_view(
322-
k));
326+
// auto prev = std::prev(lo);
327+
328+
// /* There is no element smaller than
329+
// k.
330+
// */
331+
// if (prev == ptr->end())
332+
// UT_ASSERT(
333+
// ptr->begin()->key() >=
334+
// pmem::obj::string_view(
335+
// k));
336+
// else
337+
// UT_ASSERT(
338+
// prev->key() <
339+
// pmem::obj::string_view(
340+
// k));
323341
}
324342
},
325343
[&] {
@@ -328,31 +346,41 @@ test_erase_upper_lower_bounds_neighbours(
328346

329347
/* There is no element
330348
bigger than k. */
331-
if (ub == ptr->end())
349+
if (ub == ptr->end()) {
350+
auto last_el = ptr->begin();
351+
auto it = ptr->begin();
352+
while (it != ptr->end()) {
353+
last_el = it;
354+
it++;
355+
}
332356
UT_ASSERT(
333-
ptr->rbegin()->key() <=
357+
last_el->key() <=
334358
pmem::obj::string_view(
335359
k));
336-
else
360+
} else
337361
UT_ASSERT(
338362
ub->key() >
339363
pmem::obj::string_view(
340364
k));
341365

342-
auto prev = std::prev(ub);
343-
344-
/* There is no element smaller than k.
366+
/* XXX: it's disabled due to:
367+
* https://github.com/pmem/libpmemobj-cpp/issues/1159
345368
*/
346-
if (prev == ptr->end())
347-
UT_ASSERT(
348-
ptr->begin()->key() >
349-
pmem::obj::string_view(
350-
k));
351-
else
352-
UT_ASSERT(
353-
prev->key() <=
354-
pmem::obj::string_view(
355-
k));
369+
// auto prev = std::prev(ub);
370+
371+
// /* There is no element smaller than
372+
// k.
373+
// */
374+
// if (prev == ptr->end())
375+
// UT_ASSERT(
376+
// ptr->begin()->key() >
377+
// pmem::obj::string_view(
378+
// k));
379+
// else
380+
// UT_ASSERT(
381+
// prev->key() <=
382+
// pmem::obj::string_view(
383+
// k));
356384
}
357385
}};
358386

0 commit comments

Comments
 (0)