Skip to content

Commit

Permalink
gettime: use atomic operations to access t->seq
Browse files Browse the repository at this point in the history 
fio on Windows with a 16 or 32 CPUs frequently fails while running

./fio --cpuclock-test

even though "reliable_tsc: yes" is reported. Using clang's thread sanitizer via

CC=clang ./configure --extra-cflags="-fsanitize=thread"

and running the same on Linux also generates multiple warnings similar to the
following on a VM with 16 cores:

WARNING: ThreadSanitizer: data race (pid=23780)
  Atomic write of size 4 at 0x7ffecb865a3c by thread T15 (mutexes: write M169):
    #0 __tsan_atomic32_fetch_add /home/clang-3.9/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cc:591 (fio+0x000000471505)
    axboe#1 atomic32_inc_return /home/fio/gettime.c:567:13 (fio+0x0000004c56c1)
    axboe#2 clock_thread_fn /home/fio/gettime.c:607 (fio+0x0000004c56c1)

  Previous read of size 4 at 0x7ffecb865a3c by thread T4 (mutexes: write M147):
    #0 clock_thread_fn /home/fio/gettime.c:611:19 (fio+0x0000004c56e2)

  Location is stack of main thread.

  Mutex M169 (0x7d700000f6a0) created at:
    #0 pthread_mutex_init /home/clang-3.9/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:1119 (fio+0x00000043b695)
    axboe#1 fio_monotonic_clocktest /home/fio/gettime.c:694:3 (fio+0x0000004c4c12)
    axboe#2 parse_cmd_line /home/fio/init.c:2710:15 (fio+0x0000004ce8e5)
    axboe#3 parse_options /home/fio/init.c:2828:14 (fio+0x0000004cf3da)
    axboe#4 main /home/fio/fio.c:47:6 (fio+0x00000054b991)

  Mutex M147 (0x7d700000f178) created at:
    #0 pthread_mutex_init /home/clang-3.9/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:1119 (fio+0x00000043b695)
    axboe#1 fio_monotonic_clocktest /home/fio/gettime.c:694:3 (fio+0x0000004c4c12)
    axboe#2 parse_cmd_line /home/fio/init.c:2710:15 (fio+0x0000004ce8e5)
    axboe#3 parse_options /home/fio/init.c:2828:14 (fio+0x0000004cf3da)
    axboe#4 main /home/fio/fio.c:47:6 (fio+0x00000054b991)

  Thread T15 (tid=23796, running) created by main thread at:
    #0 pthread_create /home/clang-3.9/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:902 (fio+0x00000042c9a6)
    axboe#1 fio_monotonic_clocktest /home/fio/gettime.c:697:7 (fio+0x0000004c4c38)
    axboe#2 parse_cmd_line /home/fio/init.c:2710:15 (fio+0x0000004ce8e5)
    axboe#3 parse_options /home/fio/init.c:2828:14 (fio+0x0000004cf3da)
    axboe#4 main /home/fio/fio.c:47:6 (fio+0x00000054b991)

  Thread T4 (tid=23785, finished) created by main thread at:
    #0 pthread_create /home/clang-3.9/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:902 (fio+0x00000042c9a6)
    axboe#1 fio_monotonic_clocktest /home/fio/gettime.c:697:7 (fio+0x0000004c4c38)
    axboe#2 parse_cmd_line /home/fio/init.c:2710:15 (fio+0x0000004ce8e5)
    axboe#3 parse_options /home/fio/init.c:2828:14 (fio+0x0000004cf3da)
    axboe#4 main /home/fio/fio.c:47:6 (fio+0x00000054b991)

SUMMARY: ThreadSanitizer: data race /home/fio/gettime.c:567:13 in atomic32_inc_return

Fix the above by doing the following:

- Add a configure check for __sync_val_compare_and_swap and add a helper
  atomic32_cas_return that uses it.
- Add comments noting that the atomic32_* functions act as full
  barriers.
- Don't access t->seq directly when protecting a critical region and
  instead use the atomic32_* helpers to update/read it.

The above fixes the sanitizer warnings and makes the test pass on
Windows.

Fixes: axboe#479

Signed-off-by: Sitsofe Wheeler <sitsofe@yahoo.com>
  • Loading branch information
@sitsofe
sitsofe committed Oct 16, 2017
1 parent 3082c4e commit 1a7a1fd
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 7 deletions.
28 changes: 28 additions & 0 deletions configure
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,7 @@ CYGWIN*)
# Flags below are still necessary mostly for MinGW.
socklen_t="yes"
sfaa="yes"
svcas="yes"
rusage_thread="yes"
fdatasync="yes"
clock_gettime="yes" # clock_monotonic probe has dependency on this
Expand Down Expand Up @@ -706,6 +707,30 @@ if compile_prog "" "" "__sync_fetch_and_add()" ; then
fi
print_config "__sync_fetch_and_add" "$sfaa"

##########################################
# __sync_val_compare_and_swap test
if test "$svcas" != "yes" ; then
svcas="no"
fi
cat > $TMPC << EOF
#include <inttypes.h>
static int svcas(uint32_t *ptr)
{
return __sync_val_compare_and_swap(ptr, 0, 0);
}
int main(int argc, char **argv)
{
uint32_t val = 42;
svcas(&val);
return val;
}
EOF
if compile_prog "" "" "__sync_val_compare_and_swap()" ; then
svcas="yes"
fi
print_config "__sync_val_compare_and_swap" "$svcas"

##########################################
# libverbs probe
if test "$libverbs" != "yes" ; then
Expand Down Expand Up @@ -2108,6 +2133,9 @@ fi
if test "$sfaa" = "yes" ; then
output_sym "CONFIG_SFAA"
fi
if test "$svcas" = "yes" ; then
output_sym "CONFIG_SVCAS"
fi
if test "$libverbs" = "yes" -a "$rdmacm" = "yes" ; then
output_sym "CONFIG_RDMA"
fi
Expand Down
26 changes: 19 additions & 7 deletions gettime.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -547,8 +547,8 @@ uint64_t time_since_now(const struct timespec *s)
return mtime_since_now(s) / 1000;
}

#if defined(FIO_HAVE_CPU_AFFINITY) && defined(ARCH_HAVE_CPU_CLOCK) && \
defined(CONFIG_SFAA)
#if defined(FIO_HAVE_CPU_AFFINITY) && defined(ARCH_HAVE_CPU_CLOCK) && \
defined(CONFIG_SFAA) && defined(CONFIG_SVCAS)

#define CLOCK_ENTRIES_DEBUG 100000
#define CLOCK_ENTRIES_TEST 1000
Expand All @@ -570,11 +570,19 @@ struct clock_thread {
struct clock_entry *entries;
};

/* Note: the following acts as a full barrier */
static inline uint32_t atomic32_inc_return(uint32_t *seq)
{
return 1 + __sync_fetch_and_add(seq, 1);
}

/* Note: the following acts as a full barrier */
static inline uint32_t atomic32_cas_return(uint32_t *ptr, uint32_t old,
uint32_t new)
{
return __sync_val_compare_and_swap(ptr, old, new);
}

static void *clock_thread_fn(void *data)
{
struct clock_thread *t = data;
Expand Down Expand Up @@ -607,16 +615,18 @@ static void *clock_thread_fn(void *data)
last_seq = 0;
c = &t->entries[0];
for (i = 0; i < t->nr_entries; i++, c++) {
uint32_t seq;
uint32_t seq, end_seq;
uint64_t tsc;

c->cpu = t->cpu;
do {
seq = atomic32_inc_return(t->seq);
tsc = get_cpu_clock();
end_seq = atomic32_cas_return(t->seq, seq, seq);
if (seq < last_seq)
break;
tsc = get_cpu_clock();
} while (seq != *t->seq);
last_seq = end_seq;
} while (seq != end_seq);

This comment has been minimized.

Sign in to view

Copy link
@sitsofe

sitsofe Oct 17, 2017

Author Owner

@axboe I've got to know - where did my attempt go wrong (you can see the full branch over on https://github.com/sitsofe/fio/tree/cycletest-broken)? Because the operations wrapping tsc were atomic I presumed their results would be visible on all CPUs simultaneously and that they would themselves act as full barriers. As such they wouldn't they protect the critical region correctly?

This comment has been minimized.

Sign in to view

Copy link
@axboe

axboe Oct 17, 2017

I don't know off the top of my head. I just sat down and tried to work out what the most efficient, yet bulletproof, method would be. I don't see any immediate holes in yours, but you basically just have to work out the various patterns the inc/get_cpu_clock/cas can work across CPUs. It's not a barrier thing, both the inc and the cas are fine wrt that.

Hmm, I wonder if it does need a synchronize after the inc_return and the get_cpu_clock() - it's using fetch_and_add instead of add_and fetch.

In any case, we can get by with just the cas and avoid the other atomic/locked instruction.

c->seq = seq;
c->tsc = tsc;
Expand Down Expand Up @@ -778,7 +788,8 @@ int fio_monotonic_clocktest(int debug)
return !!failed;
}

#else /* defined(FIO_HAVE_CPU_AFFINITY) && defined(ARCH_HAVE_CPU_CLOCK) */
#else /* defined(FIO_HAVE_CPU_AFFINITY) && defined(ARCH_HAVE_CPU_CLOCK) &&
* defined(CONFIG_SFAA) && defined(CONFIG_SVCAS)*/

int fio_monotonic_clocktest(int debug)
{
Expand All @@ -787,4 +798,5 @@ int fio_monotonic_clocktest(int debug)
return 1;
}

#endif
#endif /* defined(FIO_HAVE_CPU_AFFINITY) && defined(ARCH_HAVE_CPU_CLOCK) &&
* defined(CONFIG_SFAA) && defined(CONFIG_SVCAS)*/

0 comments on commit 1a7a1fd

Please sign in to comment.