fix dictionaries unittest (#14042)
dictionary unittest was giving errors because of a wrong flags; added pointer registry to dictionary to ensure that pointers are unique (internal checks only); added locks to dictionary destroy
Costa Tsaousis committed
Nov 24, 2022 at 19:35 UTC
2327d2ba35ee76a5631fd67751ba4745945e5ec0
1 file changed
+122
-13
libnetdata/dictionary/dictionary.c
+122
-13
@@ -80,6 +80,10 @@ typedef struct dictionary_item_shared {
80
struct dictionary_item {
81
#ifdef NETDATA_INTERNAL_CHECKS
82
DICTIONARY *dict;
83
+ pid_t creator_pid;
84
+ pid_t deleter_pid;
85
+ pid_t ll_adder_pid;
86
+ pid_t ll_remover_pid;
87
#endif
88
89
DICTIONARY_ITEM_SHARED *shared;
@@ -169,6 +173,11 @@ struct dictionary {
173
long int entries; // how many items are currently in the index (the linked list may have more)
174
long int referenced_items; // how many items of the dictionary are currently being used by 3rd parties
175
long int pending_deletion_items; // how many items of the dictionary have been deleted, but have not been removed yet
176
+
177
+#ifdef NETDATA_INTERNAL_CHECKS
178
+ netdata_mutex_t global_pointer_registry_mutex;
179
+ Pvoid_t global_pointer_registry;
180
+#endif
181
};
182
183
// forward definitions of functions used in reverse order in the code
@@ -190,6 +199,60 @@ static int item_check_and_acquire_advanced(DICTIONARY *dict, DICTIONARY_ITEM *it
199
#define item_is_not_referenced_and_can_be_removed(dict, item) (item_is_not_referenced_and_can_be_removed_advanced(dict, item) == RC_ITEM_OK)
200
static inline int item_is_not_referenced_and_can_be_removed_advanced(DICTIONARY *dict, DICTIONARY_ITEM *item);
201
202
+static inline void pointer_index_init(DICTIONARY *dict) {
203
+#ifdef NETDATA_INTERNAL_CHECKS
204
+ netdata_mutex_init(&dict->global_pointer_registry_mutex);
205
+#else
206
+ ;
207
+#endif
208
+}
209
+
210
+static inline void pointer_destroy_index(DICTIONARY *dict) {
211
+#ifdef NETDATA_INTERNAL_CHECKS
212
+ netdata_mutex_lock(&dict->global_pointer_registry_mutex);
213
+ JudyHSFreeArray(&dict->global_pointer_registry, PJE0);
214
+ netdata_mutex_unlock(&dict->global_pointer_registry_mutex);
215
+#else
216
+ ;
217
+#endif
218
+}
219
+static inline void pointer_add(DICTIONARY *dict __maybe_unused, DICTIONARY_ITEM *item) {
220
+#ifdef NETDATA_INTERNAL_CHECKS
221
+ netdata_mutex_lock(&dict->global_pointer_registry_mutex);
222
+ Pvoid_t *PValue = JudyHSIns(&dict->global_pointer_registry, &item, sizeof(void *), PJE0);
223
+ if(*PValue != NULL)
224
+ fatal("pointer already exists in registry");
225
+ *PValue = item;
226
+ netdata_mutex_unlock(&dict->global_pointer_registry_mutex);
227
+#else
228
+ ;
229
+#endif
230
+}
231
+
232
+static inline void pointer_check(DICTIONARY *dict __maybe_unused, DICTIONARY_ITEM *item) {
233
+#ifdef NETDATA_INTERNAL_CHECKS
234
+ netdata_mutex_lock(&dict->global_pointer_registry_mutex);
235
+ Pvoid_t *PValue = JudyHSGet(dict->global_pointer_registry, &item, sizeof(void *));
236
+ if(PValue == NULL)
237
+ fatal("pointer is not found in registry");
238
+ netdata_mutex_unlock(&dict->global_pointer_registry_mutex);
239
+#else
240
+ ;
241
+#endif
242
+}
243
+
244
+static inline void pointer_del(DICTIONARY *dict __maybe_unused, DICTIONARY_ITEM *item) {
245
+#ifdef NETDATA_INTERNAL_CHECKS
246
+ netdata_mutex_lock(&dict->global_pointer_registry_mutex);
247
+ int ret = JudyHSDel(&dict->global_pointer_registry, &item, sizeof(void *), PJE0);
248
+ if(!ret)
249
+ fatal("pointer to be deleted does not exist in registry");
250
+ netdata_mutex_unlock(&dict->global_pointer_registry_mutex);
251
+#else
252
+ ;
253
+#endif
254
+}
255
+
256
// ----------------------------------------------------------------------------
257
// memory statistics
258
@@ -666,13 +729,21 @@ static inline void dictionary_index_lock_rdlock(DICTIONARY *dict) {
729
730
netdata_rwlock_rdlock(&dict->index.rwlock);
731
}
732
+
733
+static inline void dictionary_index_rdlock_unlock(DICTIONARY *dict) {
734
+ if(unlikely(is_dictionary_single_threaded(dict)))
735
+ return;
736
+
737
+ netdata_rwlock_unlock(&dict->index.rwlock);
738
+}
739
+
740
static inline void dictionary_index_lock_wrlock(DICTIONARY *dict) {
741
if(unlikely(is_dictionary_single_threaded(dict)))
742
return;
743
744
netdata_rwlock_wrlock(&dict->index.rwlock);
745
}
675
-static inline void dictionary_index_lock_unlock(DICTIONARY *dict) {
746
+static inline void dictionary_index_wrlock_unlock(DICTIONARY *dict) {
747
if(unlikely(is_dictionary_single_threaded(dict)))
748
return;
749
@@ -735,7 +806,7 @@ static void garbage_collect_pending_deletes(DICTIONARY *dict) {
806
}
807
808
if(is_view)
738
- dictionary_index_lock_unlock(dict);
809
+ dictionary_index_wrlock_unlock(dict);
810
811
ll_recursive_unlock(dict, DICTIONARY_LOCK_WRITE);
812
@@ -892,7 +963,10 @@ static int item_check_and_acquire_advanced(DICTIONARY *dict, DICTIONARY_ITEM *it
963
964
if (having_index_lock) {
965
// delete it from the hashtable
895
- hashtable_delete_unsafe(dict, item_get_name(item), item->key_len, item);
966
+ if(hashtable_delete_unsafe(dict, item_get_name(item), item->key_len, item) == 0)
967
+ error("DICTIONARY: INTERNAL ERROR VIEW: tried to delete item with name '%s', name_len %u that is not in the index", item_get_name(item), (KEY_LEN_TYPE)(item->key_len - 1));
968
+ else
969
+ pointer_del(dict, item);
970
971
// mark it in our dictionary as deleted too
972
// this is safe to be done here, because we have got
@@ -961,6 +1035,11 @@ static inline int item_is_not_referenced_and_can_be_removed_advanced(DICTIONARY
1035
} while(!__atomic_compare_exchange_n(&item->refcount, &refcount, desired,
1036
false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST));
1037
1038
+#ifdef NETDATA_INTERNAL_CHECKS
1039
+ if(ret == RC_ITEM_OK)
1040
+ item->deleter_pid = gettid();
1041
+#endif
1042
+
1043
if(unlikely(spins > 1 && dict->stats))
1044
DICTIONARY_STATS_DELETE_SPINS_PLUS(dict, spins - 1);
1045
@@ -996,6 +1075,8 @@ static size_t hashtable_init_unsafe(DICTIONARY *dict) {
1075
static size_t hashtable_destroy_unsafe(DICTIONARY *dict) {
1076
if(unlikely(!dict->index.JudyHSArray)) return 0;
1077
1078
+ pointer_destroy_index(dict);
1079
+
1080
JError_t J_Error;
1081
Word_t ret = JudyHSFreeArray(&dict->index.JudyHSArray, &J_Error);
1082
if(unlikely(ret == (Word_t) JERR)) {
@@ -1061,6 +1142,7 @@ static inline DICTIONARY_ITEM *hashtable_get_unsafe(DICTIONARY *dict, const char
1142
Rc = JudyHSGet(dict->index.JudyHSArray, (void *)name, name_len);
1143
if(likely(Rc)) {
1144
// found in the hash table
1145
+ pointer_check(dict, (DICTIONARY_ITEM *)*Rc);
1146
return (DICTIONARY_ITEM *)*Rc;
1147
}
1148
else {
@@ -1090,6 +1172,10 @@ static inline void item_linked_list_add(DICTIONARY *dict, DICTIONARY_ITEM *item)
1172
else
1173
DOUBLE_LINKED_LIST_APPEND_UNSAFE(dict->items.list, item, prev, next);
1174
1175
+#ifdef NETDATA_INTERNAL_CHECKS
1176
+ item->ll_adder_pid = gettid();
1177
+#endif
1178
+
1179
// clear the BEING created flag,
1180
// after it has been inserted into the linked list
1181
item_flag_clear(item, ITEM_FLAG_BEING_CREATED);
@@ -1103,6 +1189,10 @@ static inline void item_linked_list_remove(DICTIONARY *dict, DICTIONARY_ITEM *it
1189
1190
DOUBLE_LINKED_LIST_REMOVE_UNSAFE(dict->items.list, item, prev, next);
1191
1192
+#ifdef NETDATA_INTERNAL_CHECKS
1193
+ item->ll_remover_pid = gettid();
1194
+#endif
1195
+
1196
garbage_collect_pending_deletes(dict);
1197
ll_recursive_unlock(dict, DICTIONARY_LOCK_WRITE);
1198
}
@@ -1150,8 +1240,14 @@ static DICTIONARY_ITEM *dict_item_create(DICTIONARY *dict __maybe_unused, size_t
1240
1241
size_t size = sizeof(DICTIONARY_ITEM);
1242
item = callocz(1, size);
1243
+
1244
+#ifdef NETDATA_INTERNAL_CHECKS
1245
+ item->creator_pid = gettid();
1246
+#endif
1247
+
1248
item->refcount = 1;
1249
item->flags = ITEM_FLAG_BEING_CREATED;
1250
+
1251
*allocated_bytes += size;
1252
1253
if(master_item) {
@@ -1431,14 +1527,16 @@ static bool dict_item_del(DICTIONARY *dict, const char *name, ssize_t name_len)
1527
int ret;
1528
DICTIONARY_ITEM *item = hashtable_get_unsafe(dict, name, name_len);
1529
if(unlikely(!item)) {
1434
- dictionary_index_lock_unlock(dict);
1530
+ dictionary_index_wrlock_unlock(dict);
1531
ret = false;
1532
}
1533
else {
1534
if(hashtable_delete_unsafe(dict, name, name_len, item) == 0)
1439
- error("DICTIONARY: INTERNAL ERROR: tried to delete item with name '%s' that is not in the index", name);
1535
+ error("DICTIONARY: INTERNAL ERROR: tried to delete item with name '%s', name_len %zd that is not in the index", name, name_len - 1);
1536
+ else
1537
+ pointer_del(dict, item);
1538
1441
- dictionary_index_lock_unlock(dict);
1539
+ dictionary_index_wrlock_unlock(dict);
1540
1541
dict_item_free_or_mark_deleted(dict, item);
1542
ret = true;
@@ -1487,25 +1585,29 @@ static DICTIONARY_ITEM *dict_item_add_or_reset_value_and_acquire(DICTIONARY *dic
1585
DICTIONARY_ITEM *item = NULL;
1586
do {
1587
DICTIONARY_ITEM **item_pptr = (DICTIONARY_ITEM **)hashtable_insert_unsafe(dict, name, name_len);
1490
- if (likely(*item_pptr == 0)) {
1588
+ if (likely(*item_pptr == NULL)) {
1589
// a new item added to the index
1590
1591
// create the dictionary item
1592
item = *item_pptr =
1593
dict_item_create_with_hooks(dict, name, name_len, value, value_len, constructor_data, master_item);
1594
1595
+ pointer_add(dict, item);
1596
+
1597
// call the hashtable react
1598
hashtable_inserted_item_unsafe(dict, item);
1599
1600
// unlock the index lock, before we add it to the linked list
1601
// DONT DO IT THE OTHER WAY AROUND - DO NOT CROSS THE LOCKS!
1502
- dictionary_index_lock_unlock(dict);
1602
+ dictionary_index_wrlock_unlock(dict);
1603
1604
item_linked_list_add(dict, item);
1605
1606
added_or_updated = true;
1607
}
1608
else {
1609
+ pointer_check(dict, *item_pptr);
1610
+
1611
if(item_check_and_acquire_advanced(dict, *item_pptr, true) != RC_ITEM_OK) {
1612
spins++;
1613
continue;
@@ -1548,7 +1650,7 @@ static DICTIONARY_ITEM *dict_item_add_or_reset_value_and_acquire(DICTIONARY *dic
1650
}
1651
}
1652
1551
- dictionary_index_lock_unlock(dict);
1653
+ dictionary_index_wrlock_unlock(dict);
1654
}
1655
} while(!item);
1656
@@ -1592,7 +1694,7 @@ static DICTIONARY_ITEM *dict_item_find_and_acquire(DICTIONARY *dict, const char
1694
DICTIONARY_STATS_SEARCH_IGNORES_PLUS1(dict);
1695
}
1696
1595
- dictionary_index_lock_unlock(dict);
1697
+ dictionary_index_rdlock_unlock(dict);
1698
1699
return item;
1700
}
@@ -1622,7 +1724,7 @@ static bool dictionary_free_all_resources(DICTIONARY *dict, size_t *mem, bool fo
1724
// destroy the index
1725
dictionary_index_lock_wrlock(dict);
1726
index_size += hashtable_destroy_unsafe(dict);
1625
- dictionary_index_lock_unlock(dict);
1727
+ dictionary_index_wrlock_unlock(dict);
1728
1729
ll_recursive_lock(dict, DICTIONARY_LOCK_WRITE);
1730
DICTIONARY_ITEM *item = dict->items.list;
@@ -1846,6 +1948,8 @@ static DICTIONARY *dictionary_create_internal(DICT_OPTIONS options, struct dicti
1948
dict_size += reference_counter_init(dict);
1949
dict_size += hashtable_init_unsafe(dict);
1950
1951
+ pointer_index_init(dict);
1952
+
1953
DICTIONARY_STATS_PLUS_MEMORY(dict, 0, dict_size, 0);
1954
1955
return dict;
@@ -1914,6 +2018,8 @@ size_t dictionary_destroy(DICTIONARY *dict) {
2018
2019
if(!dict) return 0;
2020
2021
+ ll_recursive_lock(dict, DICTIONARY_LOCK_WRITE);
2022
+
2023
dict_flag_set(dict, DICT_FLAG_DESTROYED);
2024
DICTIONARY_STATS_DICT_DESTRUCTIONS_PLUS1(dict);
2025
@@ -1931,9 +2037,12 @@ size_t dictionary_destroy(DICTIONARY *dict) {
2037
dict->referenced_items,
2038
dict->entries);
2039
2040
+ ll_recursive_unlock(dict, DICTIONARY_LOCK_WRITE);
2041
return 0;
2042
}
2043
2044
+ ll_recursive_unlock(dict, DICTIONARY_LOCK_WRITE);
2045
+
2046
size_t freed;
2047
dictionary_free_all_resources(dict, &freed, true);
2048
@@ -2391,7 +2500,7 @@ static char **dictionary_unittest_generate_names(size_t entries) {
2500
char **names = mallocz(sizeof(char *) * entries);
2501
for(size_t i = 0; i < entries ;i++) {
2502
char buf[25 + 1] = "";
2394
- snprintfz(buf, 25, "name.%zu.0123456789.%zu \t !@#$%%^&*(),./[]{}\\|~`", i, entries / 2 + i);
2503
+ snprintfz(buf, 25, "name.%zu.0123456789.%zu!@#$%%^&*(),./[]{}\\|~`", i, entries / 2 + i);
2504
names[i] = strdupz(buf);
2505
}
2506
return names;
@@ -3030,7 +3139,7 @@ static int dictionary_unittest_threads() {
3139
};
3140
3141
// threads testing of dictionary
3033
- tu.dict = dictionary_create(DICT_OPTION_NAME_LINK_DONT_CLONE | DICT_OPTION_DONT_OVERWRITE_VALUE);
3142
+ tu.dict = dictionary_create(DICT_OPTION_DONT_OVERWRITE_VALUE);
3143
time_t seconds_to_run = 5;
3144
int threads_to_create = 2;
3145
fprintf(