@cryptotaxi247 / netdata-1 / commits / 1ccef511f

Fix __atomic_compare_exchange_n() atomics (#14085)

* proper use for atomic_compare_exchange() * diskspace plugin is multi-threaded but it uses single threaded dictionaries

Costa Tsaousis committed Dec 3, 2022 at 16:30 UTC 1ccef511f0cc27cbc0aa46dff2ddc71928e8e304

4 files changed +27 -29

collectors/diskspace.plugin/plugin_diskspace.c

+1 -1

@@ -320,7 +320,7 @@ static inline void do_disk_space_stats(struct mountinfo *mi, int update_every) {

320 , SIMPLE_PATTERN_EXACT

321 );

322

323 - dict_mountpoints = dictionary_create(DICT_OPTION_SINGLE_THREADED);

323 + dict_mountpoints = dictionary_create(DICT_OPTION_NONE);

324 }

325

326 struct mount_point_metadata *m = dictionary_get(dict_mountpoints, mi->mount_point);

database/rrdcontext.c

+2 -1

@@ -29,7 +29,7 @@

29 #define WORKER_JOB_PP_QUEUE_SIZE 13

32 -typedef enum {

32 +typedef enum __attribute__ ((__packed__)) {

33 RRD_FLAG_NONE = 0,

34 RRD_FLAG_DELETED = (1 << 0), // this is a deleted object (metrics, instances, contexts)

35 RRD_FLAG_COLLECTED = (1 << 1), // this object is currently being collected

@@ -115,6 +115,7 @@ typedef enum {

115 static inline void

116 rrd_flag_add_remove_atomic(RRD_FLAGS *flags, RRD_FLAGS check, RRD_FLAGS conditionally_add, RRD_FLAGS always_remove) {

117 RRD_FLAGS expected, desired;

118 +

119 do {

120 expected = *flags;

121

libnetdata/dictionary/dictionary.c

+19 -23

@@ -3,10 +3,9 @@

3 #define DICTIONARY_INTERNALS

5 #include "../libnetdata.h"

6 -#include <Judy.h>

7 // runtime flags of the dictionary - must be checked with atomics

9 -typedef enum {

8 +typedef enum __attribute__ ((__packed__)) {

9 DICT_FLAG_NONE = 0,

10 DICT_FLAG_DESTROYED = (1 << 0), // this dictionary has been destroyed

11 } DICT_FLAGS;

@@ -23,14 +22,14 @@ typedef enum {

22 #define is_view_dictionary(dict) ((dict)->master)

23 #define is_master_dictionary(dict) (!is_view_dictionary(dict))

26 -typedef enum item_options {

25 +typedef enum __attribute__ ((__packed__)) item_options {

26 ITEM_OPTION_NONE = 0,

27 ITEM_OPTION_ALLOCATED_NAME = (1 << 0), // the name pointer is a STRING

29 // IMPORTANT: This is 1-bit - to add more change ITEM_OPTIONS_BITS

30 } ITEM_OPTIONS;

33 -typedef enum item_flags {

32 +typedef enum __attribute__ ((__packed__)) item_flags {

33 ITEM_FLAG_NONE = 0,

34 ITEM_FLAG_DELETED = (1 << 0), // this item is marked deleted, so it is not available for traversal (deleted from the index too)

35 ITEM_FLAG_BEING_CREATED = (1 << 1), // this item is currently being created - this flag is removed when construction finishes

@@ -623,7 +622,7 @@ static void dictionary_execute_delete_callback(DICTIONARY *dict, DICTIONARY_ITEM

622 if(likely(!dict->hooks || !dict->hooks->del_callback))

623 return;

624

626 - // We may execute the delete callback on items deleted from a view,

625 + // We may execute delete callback on items deleted from a view,

626 // because we may have references to it, after the master is gone

627 // so, the shared structure will remain until the last reference is released.

628

@@ -782,7 +781,7 @@ static void garbage_collect_pending_deletes(DICTIONARY *dict) {

781 while(item) {

782 examined++;

783

785 - // this will cleanup

784 + // this will clean up

785 item_next = item->next;

786 int rc = item_check_and_acquire_advanced(dict, item, is_view);

787

@@ -882,7 +881,7 @@ static void item_acquire(DICTIONARY *dict, DICTIONARY_ITEM *item) {

881

882 static void item_release(DICTIONARY *dict, DICTIONARY_ITEM *item) {

883 // this function may be called without any lock on the dictionary

885 - // or even when someone else has write lock on the dictionary

884 + // or even when someone else has 'write' lock on the dictionary

885

886 bool is_deleted;

887 REFCOUNT refcount;

@@ -934,11 +933,11 @@ static int item_check_and_acquire_advanced(DICTIONARY *dict, DICTIONARY_ITEM *it

933

934 int ret = RC_ITEM_OK;

935

936 + refcount = DICTIONARY_ITEM_REFCOUNT_GET(dict, item);

937 +

938 do {

939 spins++;

940

940 - refcount = DICTIONARY_ITEM_REFCOUNT_GET(dict, item);

941 -

941 if(refcount < 0) {

942 // we can't use this item

943 ret = RC_ITEM_IS_CURRENTLY_BEING_DELETED;

@@ -953,8 +952,7 @@ static int item_check_and_acquire_advanced(DICTIONARY *dict, DICTIONARY_ITEM *it

952

953 desired = refcount + 1;

954

956 - } while(!__atomic_compare_exchange_n(&item->refcount, &refcount, desired,

957 - false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST));

955 + } while(!__atomic_compare_exchange_n(&item->refcount, &refcount, desired, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST));

956

957 // if ret == ITEM_OK, we acquired the item

958

@@ -971,7 +969,7 @@ static int item_check_and_acquire_advanced(DICTIONARY *dict, DICTIONARY_ITEM *it

969 else

970 pointer_del(dict, item);

971

974 - // mark it in our dictionary as deleted too

972 + // mark it in our dictionary as deleted too,

973 // this is safe to be done here, because we have got

974 // a reference counter on item

975 dict_item_set_deleted(dict, item);

@@ -1013,11 +1011,11 @@ static inline int item_is_not_referenced_and_can_be_removed_advanced(DICTIONARY

1011

1012 int ret = RC_ITEM_OK;

1013

1014 + refcount = DICTIONARY_ITEM_REFCOUNT_GET(dict, item);

1015 +

1016 do {

1017 spins++;

1018

1019 - refcount = DICTIONARY_ITEM_REFCOUNT_GET(dict, item);

1020 -

1019 if(refcount < 0) {

1020 // we can't use this item

1021 ret = RC_ITEM_IS_CURRENTLY_BEING_DELETED;

@@ -1035,8 +1033,7 @@ static inline int item_is_not_referenced_and_can_be_removed_advanced(DICTIONARY

1033 ret = RC_ITEM_IS_CURRENTLY_BEING_CREATED;

1034 break;

1035 }

1038 - } while(!__atomic_compare_exchange_n(&item->refcount, &refcount, desired,

1039 - false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST));

1036 + } while(!__atomic_compare_exchange_n(&item->refcount, &refcount, desired, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST));

1037

1038 #ifdef NETDATA_INTERNAL_CHECKS

1039 if(ret == RC_ITEM_OK)

@@ -1055,8 +1052,7 @@ static inline bool item_shared_release_and_check_if_it_can_be_freed(DICTIONARY *

1052 // if we can set refcount to REFCOUNT_DELETING, we can delete this item

1053

1054 REFCOUNT links = __atomic_sub_fetch(&item->shared->links, 1, __ATOMIC_SEQ_CST);

1058 - if(links == 0 && __atomic_compare_exchange_n(&item->shared->links, &links, REFCOUNT_DELETING,

1059 - false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST)) {

1055 + if(links == 0 && __atomic_compare_exchange_n(&item->shared->links, &links, REFCOUNT_DELETING, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST)) {

1056

1057 // we can delete it

1058 return true;

@@ -1430,16 +1426,16 @@ static void dict_item_shared_set_deleted(DICTIONARY *dict, DICTIONARY_ITEM *item

1426 static bool dict_item_set_deleted(DICTIONARY *dict, DICTIONARY_ITEM *item) {

1427 ITEM_FLAGS expected, desired;

1428

1429 + expected = __atomic_load_n(&item->flags, __ATOMIC_SEQ_CST);

1430 +

1431 do {

1434 - expected = __atomic_load_n(&item->flags, __ATOMIC_SEQ_CST);

1432

1433 if (expected & ITEM_FLAG_DELETED)

1434 return false;

1435

1436 desired = expected | ITEM_FLAG_DELETED;

1437

1441 - } while(!__atomic_compare_exchange_n(&item->flags, &expected, desired,

1442 - false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST));

1438 + } while(!__atomic_compare_exchange_n(&item->flags, &expected, desired, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST));

1439

1440 DICTIONARY_ENTRIES_MINUS1(dict);

1441 return true;

@@ -1474,7 +1470,7 @@ static inline void dict_item_free_or_mark_deleted(DICTIONARY *dict, DICTIONARY_I

1470 }

1471

1472 // this is used by traversal functions to remove the current item

1477 -// if it is deleted and it has zero references. This will eliminate

1473 +// if it is deleted, and it has zero references. This will eliminate

1474 // the need for the garbage collector to kick-in later.

1475 // Most deletions happen during traversal, so this is a nice hack

1476 // to speed up everything!

@@ -1601,7 +1597,7 @@ static DICTIONARY_ITEM *dict_item_add_or_reset_value_and_acquire(DICTIONARY *dic

1597 hashtable_inserted_item_unsafe(dict, item);

1598

1599 // unlock the index lock, before we add it to the linked list

1604 - // DONT DO IT THE OTHER WAY AROUND - DO NOT CROSS THE LOCKS!

1600 + // DON'T DO IT THE OTHER WAY AROUND - DO NOT CROSS THE LOCKS!

1601 dictionary_index_wrlock_unlock(dict);

1602

1603 item_linked_list_add(dict, item);

libnetdata/string/string.c

+5 -4

@@ -71,11 +71,12 @@ void string_statistics(size_t *inserts, size_t *deletes, size_t *searches, size_

72 static inline bool string_entry_check_and_acquire(STRING *se) {

73 REFCOUNT expected, desired, count = 0;

74 +

75 + expected = __atomic_load_n(&se->refcount, __ATOMIC_SEQ_CST);

76 +

77 do {

78 count++;

77 - expected = __atomic_load_n(&se->refcount, __ATOMIC_SEQ_CST);

78 -

80 if(expected <= 0) {

81 // We cannot use this.

82 // The reference counter reached value zero,

@@ -85,8 +86,8 @@ static inline bool string_entry_check_and_acquire(STRING *se) {

86 }

88 desired = expected + 1;

88 - }

89 - while(!__atomic_compare_exchange_n(&se->refcount, &expected, desired, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST));

89 +

90 + } while(!__atomic_compare_exchange_n(&se->refcount, &expected, desired, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST));

92 string_internal_stats_add(spins, count - 1);