nd_poll() fairness (netdata#19298)

* nd_poll() fairness * ensure we dont return events the user may have deleted * fixed warnings * fixed test compilation * minor fixes * statsd fix * track writer tid in rw spinlock * log replication counters on sender disconnect
stelfrag · Dec 30, 2024 · d3b09d8 · d3b09d8
1 parent 62297e4
commit d3b09d8
Show file tree

Hide file tree

Showing 10 changed files with 245 additions and 131 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -562,6 +562,7 @@ void my_function() { ; }
 " HAVE_FUNC_ATTRIBUTE_NOINLINE)
 
 check_c_source_compiles("
+#include <stdlib.h>
 void my_exit_function() __attribute__((noreturn));
 int main() {
         my_exit_function(); // Call the noreturn function

diff --git a/src/collectors/debugfs.plugin/module-libsensors.c b/src/collectors/debugfs.plugin/module-libsensors.c
@@ -664,7 +664,7 @@ static inline void check_value_greater_than_zero(SENSOR *s, SENSOR_SUBFEATURE_TY
             s->state = state;
 
             string_freez(s->log_msg);
-            char buf[100];
+            char buf[1024];
             snprintf(buf, sizeof(buf), "%s == %f (kernel driver generated)",
                      SENSOR_SUBFEATURE_TYPE_2str(*config), status);
             s->log_msg = string_strdupz(buf);
@@ -674,7 +674,7 @@ static inline void check_value_greater_than_zero(SENSOR *s, SENSOR_SUBFEATURE_TY
 
 static void userspace_evaluation_log_msg(SENSOR *s, const char *reading_txt, const char *condition, const char *threshold_txt, double reading, double threshold) {
     string_freez(s->log_msg);
-    char buf[200];
+    char buf[1024];
     snprintf(buf, sizeof(buf), "%s %f %s %s %f (userspace evaluation using kernel provided thresholds)",
              reading_txt, reading, condition, threshold_txt, threshold);
     s->log_msg = string_strdupz(buf);
@@ -1210,7 +1210,7 @@ static int sensors_collect_data(void) {
 static bool libsensors_running = false;
 static int libsensors_update_every = 1;
 
-void *libsensors_thread(void *ptr) {
+void *libsensors_thread(void *ptr __maybe_unused) {
     int update_every = libsensors_update_every;
 
     // first try the default directory for libsensors

diff --git a/src/libnetdata/locks/rw-spinlock.c b/src/libnetdata/locks/rw-spinlock.c
@@ -2,10 +2,13 @@
 
 #include "libnetdata/libnetdata.h"
 
+#define WRITER_LOCKED (-65536)
+
 // ----------------------------------------------------------------------------
 // rw_spinlock implementation
 
 void rw_spinlock_init_with_trace(RW_SPINLOCK *rw_spinlock, const char *func __maybe_unused) {
+    rw_spinlock->writer = 0;
     rw_spinlock->counter = 0;
 }
 
@@ -14,10 +17,13 @@ bool rw_spinlock_tryread_lock_with_trace(RW_SPINLOCK *rw_spinlock, const char *f
 
     REFCOUNT expected = rw_spinlock->counter;
     while (true) {
-        if(expected < 0)
+        if(expected == WRITER_LOCKED)
             // writer is active
             return false;
 
+        if(expected < 0)
+            fatal("RW_SPINLOCK: refcount found negative, on %s(), called from %s()", __FUNCTION__, func);
+
         // increment reader count
         if (__atomic_compare_exchange_n(
                 &rw_spinlock->counter,
@@ -43,9 +49,12 @@ void rw_spinlock_read_lock_with_trace(RW_SPINLOCK *rw_spinlock, const char *func
     REFCOUNT expected = rw_spinlock->counter;
 
     // we should not increase it if it is negative (a writer holds the lock)
-    if(expected < 0) expected = 0;
+    if(expected == WRITER_LOCKED) expected = 0;
 
     while (true) {
+        if(expected < 0)
+            fatal("RW_SPINLOCK: refcount found negative, on %s(), called from %s()", __FUNCTION__, func);
+
         // Attempt to increment reader count
         if (__atomic_compare_exchange_n(
                 &rw_spinlock->counter,
@@ -59,7 +68,7 @@ void rw_spinlock_read_lock_with_trace(RW_SPINLOCK *rw_spinlock, const char *func
 
         spins++;
 
-        if (expected < 0) {
+        if (expected == WRITER_LOCKED) {
             // writer is active
 
             // we should not increase it if it is negative (a writer holds the lock)
@@ -76,13 +85,9 @@ void rw_spinlock_read_lock_with_trace(RW_SPINLOCK *rw_spinlock, const char *func
 }
 
 void rw_spinlock_read_unlock_with_trace(RW_SPINLOCK *rw_spinlock, const char *func __maybe_unused) {
-#ifndef NETDATA_INTERNAL_CHECKS
-    __atomic_sub_fetch(&rw_spinlock->counter, 1, __ATOMIC_RELEASE);
-#else
     REFCOUNT x = __atomic_sub_fetch(&rw_spinlock->counter, 1, __ATOMIC_RELEASE);
     if (x < 0)
-        fatal("RW_SPINLOCK: readers is negative %d", x);
-#endif
+        fatal("RW_SPINLOCK: readers is negative %d, on %s called from %s()", x, __FUNCTION__, func);
 
     nd_thread_rwspinlock_read_unlocked();
 }
@@ -94,14 +99,15 @@ bool rw_spinlock_trywrite_lock_with_trace(RW_SPINLOCK *rw_spinlock, const char *
     if (!__atomic_compare_exchange_n(
             &rw_spinlock->counter,
             &expected,
-            -1,
+            WRITER_LOCKED,
             false,              // Strong CAS
             __ATOMIC_ACQUIRE,   // Success memory order
             __ATOMIC_RELAXED    // Failure memory order
             )) {
         return false;
     }
 
+    __atomic_store_n(&rw_spinlock->writer, gettid_cached(), __ATOMIC_RELAXED);
     worker_spinlock_contention(func, 0);
     nd_thread_rwspinlock_write_locked();
     return true;
@@ -117,7 +123,7 @@ void rw_spinlock_write_lock_with_trace(RW_SPINLOCK *rw_spinlock, const char *fun
         if (__atomic_compare_exchange_n(
                 &rw_spinlock->counter,
                 &expected,
-                -1,
+                WRITER_LOCKED,
                 false,              // Strong CAS
                 __ATOMIC_ACQUIRE,   // Success memory order
                 __ATOMIC_RELAXED    // Failure memory order
@@ -129,17 +135,19 @@ void rw_spinlock_write_lock_with_trace(RW_SPINLOCK *rw_spinlock, const char *fun
         tinysleep();
     }
 
+    __atomic_store_n(&rw_spinlock->writer, gettid_cached(), __ATOMIC_RELAXED);
     worker_spinlock_contention(func, spins);
     nd_thread_rwspinlock_write_locked();
 }
 
 void rw_spinlock_write_unlock_with_trace(RW_SPINLOCK *rw_spinlock, const char *func __maybe_unused) {
 #ifdef NETDATA_INTERNAL_CHECKS
     int32_t x = __atomic_load_n(&rw_spinlock->counter, __ATOMIC_RELAXED);
-    if (x != -1)
-        fatal("RW_SPINLOCK: writer unlock encountered unexpected state: %d", x);
+    if (x != WRITER_LOCKED)
+        fatal("RW_SPINLOCK: writer unlock encountered unexpected state: %d, on %s() called from %s()", x, __FUNCTION__, func);
 #endif
 
+    __atomic_store_n(&rw_spinlock->writer, 0, __ATOMIC_RELAXED);
     __atomic_store_n(&rw_spinlock->counter, 0, __ATOMIC_RELEASE); // Release writer lock
     nd_thread_rwspinlock_write_unlocked();
 }
diff --git a/src/libnetdata/locks/rw-spinlock.h b/src/libnetdata/locks/rw-spinlock.h
@@ -7,10 +7,11 @@
 #include "spinlock.h"
 
 typedef struct netdata_rw_spinlock {
+    pid_t writer;
     REFCOUNT counter; // positive is readers, negative is a writer
 } RW_SPINLOCK;
 
-#define RW_SPINLOCK_INITIALIZER { .counter = 0, }
+#define RW_SPINLOCK_INITIALIZER { .counter = 0, .writer = 0, }
 
 void rw_spinlock_init_with_trace(RW_SPINLOCK *rw_spinlock, const char *func);
 void rw_spinlock_read_lock_with_trace(RW_SPINLOCK *rw_spinlock, const char *func);