Signed-off-by: Emilio G. Cota <cota@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;
 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);
+}
 
 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);
     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);
     synchronize_rcu();
-    while (n_nodes_removed > n_reclaims) {
+    while (count_read(&n_nodes_removed) > count_read(&n_reclaims)) {
         g_usleep(100);
         synchronize_rcu();
     }
     if (g_test_in_charge) {
-        g_assert_cmpint(n_nodes_removed, ==, n_reclaims);
+        g_assert_cmpint(count_read(&n_nodes_removed), ==,
+                        count_read(&n_reclaims));
     } else {
         printf("%s: %d readers; 1 updater; nodes read: "  \
                "%lld, nodes removed: %lld; nodes reclaimed: %lld\n",
-               test, nthreadsrunning - 1, n_reads, n_nodes_removed, n_reclaims);
+               test, nthreadsrunning - 1, n_reads,
+               count_read(&n_nodes_removed), count_read(&n_reclaims));
         exit(0);
     }
 }
-- 
2.17.1

Hi, Emilio.

On Mon, Sep 03, 2018 at 01:18:27PM -0400, Emilio G. Cota wrote:
> Signed-off-by: Emilio G. Cota <cota@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?

>  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?

>
>  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?

>      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?

>      synchronize_rcu();
> -    while (n_nodes_removed > n_reclaims) {
> +    while (count_read(&n_nodes_removed) > count_read(&n_reclaims)) {
>          g_usleep(100);
>          synchronize_rcu();
>      }
>      if (g_test_in_charge) {
> -        g_assert_cmpint(n_nodes_removed, ==, n_reclaims);
> +        g_assert_cmpint(count_read(&n_nodes_removed), ==,
> +                        count_read(&n_reclaims));
>      } else {
>          printf("%s: %d readers; 1 updater; nodes read: "  \
>                 "%lld, nodes removed: %lld; nodes reclaimed: %lld\n",
> -               test, nthreadsrunning - 1, n_reads, n_nodes_removed, n_reclaims);
> +               test, nthreadsrunning - 1, n_reads,
> +               count_read(&n_nodes_removed), count_read(&n_reclaims));
>          exit(0);
>      }
>  }
> --
> 2.17.1
>
>

--
Murilo

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 <cota@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

Hi, Emilio.

On Tue, Sep 04, 2018 at 03:35:38PM -0400, Emilio G. Cota wrote:
> 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 <cota@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
> 

The explanations you provided made a lot of difference on understanding the why
of your patch.  Thank you!

Is it possible to enhance commit message and add the explanations?

-- 
Murilo

On Tue, Sep 04, 2018 at 17:56:31 -0300, Murilo Opsfelder Araujo wrote:
> The explanations you provided made a lot of difference on understanding the why
> of your patch.  Thank you!

Glad that helped!

> Is it possible to enhance commit message and add the explanations?

If I end up submitting a v2 of this series due to code-related
changes, then I'll expand this patch's commit message.

Thanks,

		E.

On 04/09/2018 22:56, Murilo Opsfelder Araujo wrote:
>>> +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.

Like for patch 1, I'm not sure I like introducing the data races...
While reads inside the seqlock do not need atomics, I think the write
should be atomic in both cases.

Paolo