Thanks for the comments! I used them to revise the patch and re-submitted it.
Specifically: - static_asserts replace plain asserts - the confusing comment from latch::reset was removed along with the extraneous lock (latch::reset was also renamed to latch::unsafe_reset and a comment added to indicate why it's unsafe and what should be done to make it safe) - a snippet of the tst-pthread-barrier.so unit test results was included in the commit comment since the usleep delay interval is now very short (7 usecs) The unit test uses a randomized usleep between 1 and 7 microsecs, for 10 threads that do 4 rounds of barrier crossings. Special return value PTHREAD_BARRIER_SERIAL_THREAD == -1 [Thread 8] crossed barrier with -1 (round 1) [Thread 9] crossed barrier with -1 (round 2) [Thread 9] crossed barrier with -1 (round 3) [Thread 3] crossed barrier with -1 (round 4) The full unit test results are: $ scripts/run.py -e tests/tst-pthread-barrier.so OSv v0.24-241-g8171cd6 eth0: 192.168.122.15 Sizeof pthread_barrier_t : 32 PASS: sizeof pthread_barrier_t is 32 bytes Sizeof pthread_barrierattr_t: 4 PASS: sizeof pthread_barrierattr_t is 4 bytes PASS: pthread_barrier_init (count == -1) PASS: pthread_barrier_init (count == 0) PASS: pthread_barrier_init (count == INT_MAX) PASS: pthread_barrier_init [Thread 0] starting... [Thread 0] waiting on barrier [Thread 1] starting... [Thread 1] waiting on barrier [Thread 2] starting... [Thread 2] waiting on barrier [Thread 3] starting... [Thread 3] waiting on barrier [Thread 4] starting... [Thread 4] waiting on barrier [Thread 5] starting... [Thread 5] waiting on barrier [Thread 6] starting... [Thread 6] waiting on barrier [Thread 7] starting... [Thread 7] waiting on barrier [Thread 8] starting... [Thread 8] waiting on barrier [Thread 9] starting... [Thread 9] waiting on barrier [Thread 9] crossed barrier with 0 [Thread 0] crossed barrier with 0 [Thread 1] crossed barrier with 0 [Thread 2] crossed barrier with 0 [Thread 3] crossed barrier with 0 [Thread 4] crossed barrier with 0 [Thread 5] crossed barrier with 0 [Thread 6] crossed barrier with 0 [Thread 7] crossed barrier with 0 [Thread 8] crossed barrier with -1 [Thread 2] waiting on barrier [Thread 6] waiting on barrier [Thread 4] waiting on barrier [Thread 0] waiting on barrier [Thread 1] waiting on barrier [Thread 5] waiting on barrier [Thread 3] waiting on barrier [Thread 7] waiting on barrier [Thread 9] waiting on barrier [Thread 8] waiting on barrier [Thread 8] crossed barrier with 0 [Thread 2] crossed barrier with 0 [Thread 6] crossed barrier with 0 [Thread 4] crossed barrier with 0 [Thread 0] crossed barrier with 0 [Thread 1] crossed barrier with 0 [Thread 5] crossed barrier with 0 [Thread 3] crossed barrier with 0 [Thread 7] crossed barrier with 0 [Thread 9] crossed barrier with -1 [Thread 4] waiting on barrier [Thread 2] waiting on barrier [Thread 6] waiting on barrier [Thread 0] waiting on barrier [Thread 1] waiting on barrier [Thread 3] waiting on barrier [Thread 5] waiting on barrier [Thread 7] waiting on barrier [Thread 9] waiting on barrier [Thread 8] waiting on barrier [Thread 8] crossed barrier with 0 [Thread 4] crossed barrier with 0 [Thread 2] crossed barrier with 0 [Thread 6] crossed barrier with 0 [Thread 0] crossed barrier with 0 [Thread 1] crossed barrier with 0 [Thread 3] crossed barrier with 0 [Thread 5] crossed barrier with 0 [Thread 7] crossed barrier with 0 [Thread 9] crossed barrier with -1 [Thread 4] waiting on barrier [Thread 6] waiting on barrier [Thread 2] waiting on barrier [Thread 1] waiting on barrier [Thread 0] waiting on barrier [Thread 9] waiting on barrier [Thread 5] waiting on barrier [Thread 7] waiting on barrier [Thread 3] waiting on barrier [Thread 8] waiting on barrier [Thread 8] crossed barrier with 0 [Thread 4] crossed barrier with 0 [Thread 6] crossed barrier with 0 [Thread 2] crossed barrier with 0 [Thread 1] crossed barrier with 0 [Thread 0] crossed barrier with 0 [Thread 9] crossed barrier with 0 [Thread 5] crossed barrier with 0 [Thread 7] crossed barrier with 0 [Thread 3] crossed barrier with -1 PASS: pthread_barrier_wait (special retvals) SUMMARY: 7 tests / 0 failures thanks, Rean On Tue, Dec 20, 2016 at 1:59 AM, Nadav Har'El <[email protected]> wrote: > Thanks. > Some comments inline below. > > On Tue, Dec 20, 2016 at 11:21 AM, 'rean' via OSv Development < > [email protected]> wrote: > >> - pthread_barrier* patch clean up (address review comments) >> - remove pthread_barrier_t_int.in counter >> >> Signed-off-by: rean <[email protected]> >> --- >> include/osv/latch.hh | 11 +++++ >> libc/pthread.cc | 109 ++++++++++++++++++++++++++++++ >> ++++++++++++- >> modules/tests/Makefile | 2 +- >> tests/tst-pthread-barrier.cc | 106 ++++++++++++++++++++++++++++++ >> +++++++++++ >> 4 files changed, 226 insertions(+), 2 deletions(-) >> create mode 100644 tests/tst-pthread-barrier.cc >> >> diff --git a/include/osv/latch.hh b/include/osv/latch.hh >> index 6ff78a8..d4b0cee 100644 >> --- a/include/osv/latch.hh >> +++ b/include/osv/latch.hh >> @@ -58,6 +58,17 @@ public: >> std::unique_lock<std::mutex> l(_mutex); >> return _condvar.wait_for(l, duration, [&] () -> bool { return >> is_released(); }); >> } >> + // Useful if latches are being used as a primitive for implementing >> + // pthread_barrier_t so threads can wait on a barrier multiple times >> + // (over multiple rounds). Only one thread should call this, >> preferably >> + // with some external locking. >> + void reset(int count) >> + { >> + // Use scoped-lock for reset so that another thread could do a >> count_down >> + // concurrently but there would be some reasonable ordering. >> + std::unique_lock<std::mutex> l(_mutex); >> > > I don't understand this comment, or why this lock is helpful: > If a call to reset() races with a call to count_down(), disaster is pretty > much guaranteed, regardless of which "order" the mutex guarantees: > 1. If reset() happens before the count_down(), the count_down() call will > bring the counter to -1, and a sleeping await() will never get called. > 2. If reset() happens after the count_down(), it is possible this is not > the last count_down() (after all, the intention here is that several > threads call count_down()), and when one comes later, we'll have the > problem in 1. > > So it seems to me, the only way to use reset() safely is for the caller to > ensure that when it's called, no other thread may either be calling > count_down(), or be in an await(). The external mutex you used guarantees > that. > > > + _count.store(count, std::memory_order_relaxed); >> + } >> }; >> >> class thread_barrier >> diff --git a/libc/pthread.cc b/libc/pthread.cc >> index 148b79e..805a6f3 100644 >> --- a/libc/pthread.cc >> +++ b/libc/pthread.cc >> @@ -29,7 +29,7 @@ >> >> #include <api/time.h> >> #include <osv/rwlock.h> >> - >> +#include <osv/latch.hh> >> #include "pthread.hh" >> >> namespace pthread_private { >> @@ -1121,3 +1121,110 @@ int pthread_attr_getaffinity_np(const >> pthread_attr_t *attr, size_t cpusetsize, >> >> return 0; >> } >> + >> +// Private definitions of the internal structs backing pthread_barrier_t >> and >> +// pthread_barrierattr_t >> +typedef struct >> +{ >> + unsigned int out; >> + unsigned int count; >> + latch *ltch; >> + pthread_mutex_t *mtx; >> +} pthread_barrier_t_int; >> + >> +typedef struct >> +{ >> + unsigned pshared; >> +} pthread_barrierattr_t_int; >> + >> +int pthread_barrier_init(pthread_barrier_t *barrier_opq, >> + const pthread_barrierattr_t *attr_opq, >> + unsigned count) >> +{ >> + // Linux returns EINVAL if count == 0 or INT_MAX so we do too. >> + // In theory, we could go up to UINT_MAX since count is unsigned. >> + if (count == 0 || count >= INT_MAX) { >> + return EINVAL; >> + } >> + >> + // Always ignore attr, it has no meaning in the context of a >> unikernel. >> + // pthread_barrierattr_t has a single member variable pshared that >> can be set >> + // to PTHREAD_PROCESS_PRIVATE or PTHREAD_PROCESS_SHARED. These have >> the >> + // same effect in a unikernel - there is only a single process and all >> + // threads can manipulate the memory area associated with the >> + // pthread_barrier_t so it doesn't matter what the value of pshared >> is set to >> + pthread_barrier_t_int barrier; >> + barrier.count = count; >> + barrier.out = 0; >> + barrier.ltch = new latch(count); >> + barrier.mtx = new pthread_mutex_t; >> + pthread_mutex_init(barrier.mtx, NULL); >> + assert(sizeof(pthread_barrier_t_int) <= sizeof(pthread_barrier_t)); >> + memcpy(barrier_opq, &barrier, sizeof(*barrier_opq)); >> + return 0; >> +} >> + >> +int pthread_barrier_wait(pthread_barrier_t *barrier_opq) >> +{ >> + pthread_barrier_t_int *barrier = (pthread_barrier_t_int*) >> barrier_opq; >> > + assert(sizeof(pthread_barrier_t_int) <= sizeof(pthread_barrier_t)); >> > > This should be a static_assert (see examples in pthread.cc), to discover > this problem during compilation, not in runtime when pthread_barrier_wait > is first used. > >> + >> + if (!barrier || !barrier->ltch || !barrier->mtx) { >> + return EINVAL; >> + } >> + >> + int retval = 0; >> + pthread_mutex_t *mtx = barrier->mtx; >> + >> + pthread_mutex_lock(mtx); >> + pthread_mutex_unlock(mtx); >> + >> + latch *l = barrier->ltch; >> + l->count_down(); >> + // All threads stuck here until we get at least 'count' waiters >> + l->await(); >> + >> + // If the last thread (thread x) to wait on the barrier is >> descheduled here >> + // (immediately after being the count'th thread crossing the barrier) >> + // the barrier remains open (a new waiting thread will cross) until >> + // the barrier is reset below (when thread x is rescheduled), which >> doesn't >> + // seem technically incorrect. Only one of the crossing threads will >> get a >> + // retval of PTHREAD_BARRIER_SERIAL_THREAD, when >> + // barrier->out == barrier->count. >> + // All other crossing threads will get a retval of 0. >> + >> + pthread_mutex_lock(mtx); >> + barrier->out++; >> + // Make the last thread out responsible for resetting the barrier's >> latch. >> + // The last thread also gets the special return value >> + // PTHREAD_BARRIER_SERIAL_THREAD. Every other thread gets a retval of >> 0 >> + if (barrier->out == barrier->count) { >> + retval = PTHREAD_BARRIER_SERIAL_THREAD; >> + // Reset the latch for the next round of waiters >> + l->reset(barrier->count); >> + // Reset the 'out' counter so that the equality check above works >> across >> + // multiple rounds of threads waiting on the barrier >> + barrier->out = 0; >> + } >> + pthread_mutex_unlock(mtx); >> + return retval; >> +} >> + >> +int pthread_barrier_destroy(pthread_barrier_t *barrier_opq) >> +{ >> + pthread_barrier_t_int *barrier = (pthread_barrier_t_int*) barrier_opq; >> + assert(sizeof(pthread_barrier_t_int) <= sizeof(pthread_barrier_t)); >> > > Again,static_assert. > > >> + >> + if (!barrier || !barrier->ltch || !barrier->mtx) { >> + return EINVAL; >> + } >> + >> + delete barrier->ltch; >> + barrier->ltch = nullptr; >> + >> + pthread_mutex_destroy(barrier->mtx); >> + delete barrier->mtx; >> + barrier->mtx = nullptr; >> + >> + return 0; >> +} >> diff --git a/modules/tests/Makefile b/modules/tests/Makefile >> index 3f9cb59..feeba12 100644 >> --- a/modules/tests/Makefile >> +++ b/modules/tests/Makefile >> @@ -85,7 +85,7 @@ tests := tst-pthread.so misc-ramdisk.so tst-vblk.so >> tst-bsd-evh.so \ >> payload-merge-env.so misc-execve.so misc-execve-payload.so >> misc-mutex2.so \ >> tst-pthread-setcancelstate.so tst-syscall.so tst-pin.so >> tst-run.so \ >> tst-ifaddrs.so tst-pthread-affinity-inherit.so >> tst-sem-timed-wait.so \ >> - tst-ttyname.so >> + tst-ttyname.so tst-pthread-barrier.so >> >> # libstatic-thread-variable.so tst-static-thread-variable.so \ >> >> diff --git a/tests/tst-pthread-barrier.cc b/tests/tst-pthread-barrier.cc >> new file mode 100644 >> index 0000000..3a1b62b >> --- /dev/null >> +++ b/tests/tst-pthread-barrier.cc >> @@ -0,0 +1,106 @@ >> +#include <stdio.h> >> +#include <unistd.h> >> +#include <memory.h> >> +#include <errno.h> >> +#include <stdbool.h> >> +#include <pthread.h> >> +#include <limits.h> >> +#include <stdlib.h> >> +#include <atomic> >> + >> +unsigned int tests_total = 0, tests_failed = 0; >> + >> +void report(const char* name, bool passed) >> +{ >> + static const char* status[] = {"FAIL", "PASS"}; >> + printf("%s: %s\n", status[passed], name); >> + tests_total += 1; >> + tests_failed += !passed; >> +} >> + >> +// Opaque type 32 bytes in size >> +static pthread_barrier_t barrier; >> +// Opaque type 4 bytes in size >> +pthread_barrierattr_t attr; >> +// Number of crossings across the barrier >> +static int numCrossings; >> +// Counter to track the number of special return values to threads >> +static std::atomic<int> specialRetVals; >> + >> +static void* thread_func(void *arg) >> +{ >> + int threadNum = arg ? *((int*) arg): 0; >> + int retval = 0; >> + printf("[Thread %d] starting...\n", threadNum); >> + >> + for (int crossing = 0; crossing < numCrossings; crossing++) { >> + // Force threads to sleep for a random interval so we randomize >> + // which thread might get the special return value >> + // PTHREAD_BARRIER_SERIAL_THREAD >> + int delay = random() % 7 + 1; >> + usleep(delay); >> > > Between sleeping 7 seconds and sleeping 7 microseconds, there are several > orders of magnitude ;-) Please verify that this tiny sleep is actually > enough to randomize things. > > >> + >> + printf("[Thread %d] waiting on barrier\n", threadNum); >> + retval = pthread_barrier_wait(&barrier); >> + if (retval == PTHREAD_BARRIER_SERIAL_THREAD) { >> + printf("[Thread %d] crossed barrier with %d\n", threadNum, >> + PTHREAD_BARRIER_SERIAL_THREAD); >> + // Increment the counter of special return values >> + specialRetVals.fetch_add(1, std::memory_order_relaxed); >> + } else if (retval == 0) { >> + printf("[Thread %d] crossed barrier with %d\n", threadNum, >> retval); >> + } >> + } >> + return 0; >> +} >> + >> +int main(void) >> +{ >> + // Number of threads that must call into the barrier before they all >> unblock >> + const int numThreads = 10; >> + // Pass through the barrier k times >> + numCrossings = 4; >> + specialRetVals.store(0, std::memory_order_relaxed); >> + pthread_t threads[numThreads]; >> + int threadIds[numThreads]; >> + int retval = -1; >> + printf("Sizeof pthread_barrier_t : %ld\n", sizeof(barrier)); >> + report("sizeof pthread_barrier_t is 32 bytes\n", sizeof(barrier) == >> 32); >> + printf("Sizeof pthread_barrierattr_t: %ld\n", sizeof(attr)); >> + report("sizeof pthread_barrierattr_t is 4 bytes\n", sizeof(attr) == >> 4); >> + >> + // Try an invalid initialization (-1 or 0) >> + retval = pthread_barrier_init(&barrier, NULL, -1); >> + report("pthread_barrier_init (count == -1)", retval == EINVAL); >> + retval = pthread_barrier_init(&barrier, NULL, 0); >> + report("pthread_barrier_init (count == 0)", retval == EINVAL); >> + retval = pthread_barrier_init(&barrier, NULL, INT_MAX); >> + report("pthread_barrier_init (count == INT_MAX)", retval == EINVAL); >> + >> + // Initalize a barrier with NULL attributes. In general >> + // it doesn't really matter what we do with pthread_barrierattr_t >> + // PTHREAD_PROCESS_PRIVATE vs PTHREAD_PROCESS_SHARED have the same >> effect >> + // in a unikernel - there's only a single process and all threads can >> + // manipulate the barrier so we can just ignore pthread_barrierattr_t >> + retval = pthread_barrier_init(&barrier, NULL, numThreads); >> + report("pthread_barrier_init", retval == 0); >> + if (retval != 0) { >> + printf("Early exit, pthread_barrier_init returned %d instead of >> 0\n", >> + retval); >> + goto exit; >> + } >> + >> + for (int t = 0; t < numThreads; t++) { >> + threadIds[t] = t; >> + retval = pthread_create(&threads[t], NULL, thread_func, >> &threadIds[t]); >> + } >> + exit: >> + for (int t = 0; t < numThreads; t++) { >> + pthread_join(threads[t], NULL); >> + } >> + report("pthread_barrier_wait (special retvals)", >> + specialRetVals.load(std::memory_order_relaxed) == >> numCrossings); >> + pthread_barrier_destroy(&barrier); >> + printf("SUMMARY: %u tests / %u failures\n", tests_total, >> tests_failed); >> + return tests_failed == 0 ? 0 : 1; >> +} >> -- >> 2.7.4 >> >> -- >> You received this message because you are subscribed to the Google Groups >> "OSv Development" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected]. >> For more options, visit https://groups.google.com/d/optout. >> > > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
