On Tue, Sep 04, 2018 at 14:37:34 -0300, Murilo Opsfelder Araujo wrote: > Hi, Emilio. > > On Mon, Sep 03, 2018 at 01:18:27PM -0400, Emilio G. Cota wrote: > > Signed-off-by: Emilio G. Cota <c...@braap.org> > > --- > > tests/test-rcu-list.c | 67 +++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 59 insertions(+), 8 deletions(-) > > > > diff --git a/tests/test-rcu-list.c b/tests/test-rcu-list.c > > index 192bfbf02e..2606b7c19d 100644 > > --- a/tests/test-rcu-list.c > > +++ b/tests/test-rcu-list.c > > @@ -25,6 +25,23 @@ > > #include "qemu/rcu.h" > > #include "qemu/thread.h" > > #include "qemu/rcu_queue.h" > > +#include "qemu/seqlock.h" > > + > > +/* > > + * Abstraction to avoid torn accesses when there is a single thread > > updating > > + * the count. > > + * > > + * If CONFIG_ATOMIC64 is defined, we simply use atomic accesses. > > Otherwise, we > > + * use a seqlock without a lock, since only one thread can update the > > count. > > + */ > > +struct Count { > > + long long val; > > +#ifndef CONFIG_ATOMIC64 > > + QemuSeqLock sequence; > > +#endif > > +}; > > + > > +typedef struct Count Count; > > > > /* > > * Test variables. > > @@ -33,8 +50,8 @@ > > static QemuMutex counts_mutex; > > static long long n_reads = 0LL; > > static long long n_updates = 0LL; > > -static long long n_reclaims = 0LL; > > -static long long n_nodes_removed = 0LL; > > +static Count n_reclaims; > > +static Count n_nodes_removed; > > Don't we need to init the seqlocks? > > seqlock_init(&n_reclaims.sequence); > seqlock_init(&n_nodes_removed.sequence); > > Don't we need to init ->val with 0LL as well?
These are all zeroed out due to being static. We could add seqlock_init calls just to be more clear, but seqlock_init would not have any actual effect (it just sets the sequence to 0). > > static long long n_nodes = 0LL; > > static int g_test_in_charge = 0; > > > > @@ -60,6 +77,38 @@ static int select_random_el(int max) > > return (rand() % max); > > } > > > > +static inline long long count_read(Count *c) > > +{ > > +#ifdef CONFIG_ATOMIC64 > > + /* use __nocheck because sizeof(void *) might be < sizeof(long long) */ > > + return atomic_read__nocheck(&c->val); > > +#else > > + unsigned int version; > > + long long val; > > + > > + do { > > + version = seqlock_read_begin(&c->sequence); > > + val = c->val; > > + } while (seqlock_read_retry(&c->sequence, version)); > > + return val; > > +#endif > > +} > > + > > +static inline void count_add(Count *c, long long val) > > +{ > > +#ifdef CONFIG_ATOMIC64 > > + atomic_set__nocheck(&c->val, c->val + val); > > +#else > > + seqlock_write_begin(&c->sequence); > > + c->val += val; > > + seqlock_write_end(&c->sequence); > > +#endif > > +} > > + > > +static inline void count_inc(Count *c) > > +{ > > + count_add(c, 1); > > +} > > Are these `#ifdef CONFIG_ATOMIC64` required? > > The bodies of > > seqlock_read_begin() > seqlock_read_retry() > seqlock_write_begin() > seqlock_write_end() > > in include/qemu/seqlock.h make me think that they already use atomic > operations. > What am I missing? atomic_read/set(*foo), as defined in qemu/atomic.h, are as wide as foo. The sequence number in a seqlock is an "unsigned", so those atomics won't be larger than 32 bits. The counts we're dealing with here are 64-bits, so with #ifdef CONFIG_ATOMIC64 we ensure that the host can actually perform those 64-bit atomic accesses. > > > > static void create_thread(void *(*func)(void *)) > > { > > @@ -104,7 +153,7 @@ static void reclaim_list_el(struct rcu_head *prcu) > > struct list_element *el = container_of(prcu, struct list_element, rcu); > > g_free(el); > > /* Accessed only from call_rcu thread. */ > > - n_reclaims++; > > + count_inc(&n_reclaims); > > } > > > > #if TEST_LIST_TYPE == 1 > > @@ -232,7 +281,7 @@ static void *rcu_q_updater(void *arg) > > qemu_mutex_lock(&counts_mutex); > > n_nodes += n_nodes_local; > > n_updates += n_updates_local; > > - n_nodes_removed += n_removed_local; > > + count_add(&n_nodes_removed, n_removed_local); > > I'm just curious why n_nodes and n_updates don't need seqlocks. Are > n_nodes_removed and n_reclaims some kind of special that seqlocks are > required? n_nodes and n_updates are serialized by counts_mutex. n_nodes_removed and n_reclaims don't really need the lock (even though some accesses to it are protected by it; more on this below), since they're updated by a single thread at a time. This is why just using atomic_set is enough for these, and why we use seqlocks if the host cannot do 64-bit atomic accesses. Note that these "atomics" are not "read-modify-write"; they are atomic in the sense that they prevent torn accesses, but otherwise imply no memory fences or synchronization. > > qemu_mutex_unlock(&counts_mutex); > > return NULL; > > } > > @@ -286,19 +335,21 @@ static void rcu_qtest(const char *test, int duration, > > int nreaders) > > n_removed_local++; > > } > > qemu_mutex_lock(&counts_mutex); > > - n_nodes_removed += n_removed_local; > > + count_add(&n_nodes_removed, n_removed_local); > > qemu_mutex_unlock(&counts_mutex); > > Does this count_add() need to be guarded by a mutex? No. In fact, accesses to n_nodes_removed don't need the mutex at all, because only one thread writes to it at a time (first the updater thread, then the main thread joins the updater thread, and then the main thread updates it). Performance-wise, this change wouldn't change much though, which is why I decided not to include it. The main purpose of this patch is to avoid undefined behaviour when different threads access the same variable with regular accesses without always holding the same lock, as is the case with the two variables converted to Count. Cheers, Emilio