From 697f8f87c609f668bc4c91460a6b1a35273a5e2b Mon Sep 17 00:00:00 2001 From: Chih-En Lin Date: Sun, 23 Apr 2023 05:58:48 +0800 Subject: [PATCH] thread-rcu: Fix incorrect grace period counting Currently, the reader_func is counting the number of grace periods based on the value of dut, which is updated by rcu_assign_pointer. However, the value of dut actually represents the time we update the value, not the number of grace periods. Also, the original method might result in incorrect counting if someone tried to update the gp_idx while others who saw the same dut value with prev_count still depend on the old gp_idx to increase the counter. To fix the problem, instead of relying on the dut value to increase the gp_idx, we manually increase gp_idx on write side. Then, we can easily determine the gp on read side. For dut value, we simply check the old count value is not greater than the newest one. Additionally, since synchronize_rcu is quite slow, readers generally will pass through the critical section during the first grace period. To generate more realistic output, we add a delay on read side before entering the critical section. Before: 100 reader(s), 5 update run(s), 6 grace period(s) [grace period #0] 100 reader(s) [grace period #1] 0 reader(s) [grace period #2] 0 reader(s) [grace period #3] 0 reader(s) [grace period #4] 0 reader(s) [grace period #5] 0 reader(s) After, we added a delay: 100 reader(s), 5 update run(s), 6 grace period(s) [grace period #0] 76 reader(s) [grace period #1] 0 reader(s) [grace period #2] 1 reader(s) [grace period #3] 0 reader(s) [grace period #4] 3 reader(s) [grace period #5] 20 reader(s) --- thread-rcu/Makefile | 2 +- thread-rcu/main.c | 44 +++++++++++++++++++++++++++----------------- thread-rcu/rcu.h | 2 +- 3 files changed, 29 insertions(+), 19 deletions(-) diff --git a/thread-rcu/Makefile b/thread-rcu/Makefile index 8df1af5..62201d3 100644 --- a/thread-rcu/Makefile +++ b/thread-rcu/Makefile @@ -1,6 +1,6 @@ CFLAGS = -Wall CFLAGS += -g -CFLAGS += -std=c11 +CFLAGS += -std=gnu11 CFLAGS += -D'N_READERS=100' CFLAGS += -D'N_UPDATE_RUN=5' CFLAGS += -fsanitize=thread diff --git a/thread-rcu/main.c b/thread-rcu/main.c index 39dfbc6..4318f9e 100644 --- a/thread-rcu/main.c +++ b/thread-rcu/main.c @@ -37,6 +37,7 @@ static inline void thread_barrier(struct barrier_struct *b, size_t n) #include #include #include +#include #include "rcu.h" @@ -55,34 +56,42 @@ static atomic_uint grace_periods[GP_IDX_MAX]; static void *reader_func(void *argv) { struct test *tmp; + unsigned int local_gp_idx; unsigned int old_prev_count; + struct timespec ts = { + .tv_sec = 0, + .tv_nsec = 30000L, + }; if (rcu_init()) abort(); thread_barrier(&test_barrier, N_READERS + 1); + nanosleep(&ts, NULL); rcu_read_lock(); tmp = rcu_dereference(dut); - if (tmp->count != atomic_load_explicit(&prev_count, memory_order_acquire)) { - old_prev_count = atomic_exchange_explicit(&prev_count, tmp->count, - memory_order_release); - if (tmp->count != old_prev_count) - atomic_fetch_add_explicit(&gp_idx, 1, memory_order_release); - if (atomic_load_explicit(&gp_idx, memory_order_acquire) > - N_UPDATE_RUN) { - fprintf(stderr, "grace period index (%u) is over bound (%u).\n", - atomic_load_explicit(&gp_idx, memory_order_acquire), - N_UPDATE_RUN); - abort(); - } + old_prev_count = atomic_load_explicit(&prev_count, memory_order_acquire); + if (old_prev_count < tmp->count) { + atomic_compare_exchange_strong(&prev_count, &old_prev_count, + tmp->count); + } else if (tmp->count < old_prev_count) { + fprintf(stderr, + "old count (%u) should not be larger than new one (%u).\n", + old_prev_count, tmp->count); + abort(); } - atomic_fetch_add_explicit( - &grace_periods[atomic_load_explicit(&gp_idx, memory_order_acquire)], 1, - memory_order_relaxed); + local_gp_idx = atomic_load_explicit(&gp_idx, memory_order_acquire); + if (local_gp_idx > N_UPDATE_RUN) { + fprintf(stderr, "grace period index (%u) is over bound (%u).\n", + local_gp_idx, N_UPDATE_RUN); + abort(); + } + atomic_fetch_add_explicit(&grace_periods[local_gp_idx], 1, + memory_order_relaxed); rcu_read_unlock(); @@ -103,6 +112,7 @@ static void *updater_func(void *argv) newval->count = i; oldp = rcu_assign_pointer(dut, newval); synchronize_rcu(); + atomic_fetch_add_explicit(&gp_idx, 1, memory_order_release); free(oldp); } @@ -140,8 +150,8 @@ int main(int argc, char *argv[]) if (total != N_READERS) fprintf(stderr, - "The Sum of records in the array of grace period(s) (%u) is " - "not the same with number of reader(s) (%u)\n", + "The sum of records in the array of grace period(s) (%u)\n" + "is not the same with number of reader(s) (%u)\n", total, N_READERS); return 0; diff --git a/thread-rcu/rcu.h b/thread-rcu/rcu.h index 55650b5..2508ad2 100644 --- a/thread-rcu/rcu.h +++ b/thread-rcu/rcu.h @@ -168,7 +168,7 @@ struct rcu_data { } while (0) #define rcu_next(np) \ ((struct rcu_node *) (READ_ONCE((np)->__next_rcu_nesting) & ~0x3)) -#define rcu_next_mask(nrn) ((struct rcu_node *) ((uintptr_t)(nrn) & ~0x3)) +#define rcu_next_mask(nrn) ((struct rcu_node *) ((uintptr_t) (nrn) & ~0x3)) static struct rcu_data rcu_data = { .nr_thread = 0,