Hi, thanks for the patch! Some comments and requests below.
On Mon, Dec 19, 2016 at 7:23 AM, 'rean' via OSv Development < [email protected]> wrote: > Signed-off-by: rean <[email protected]> > --- > include/api/x64/bits/alltypes.h.sh | 4 +- > include/osv/latch.hh | 7 +++ > libc/pthread.cc | 80 +++++++++++++++++++++++++++++- > modules/tests/Makefile | 2 +- > tests/tst-pthread-barrier.c | 99 ++++++++++++++++++++++++++++++ > ++++++++ > 5 files changed, 188 insertions(+), 4 deletions(-) > create mode 100644 tests/tst-pthread-barrier.c > > diff --git a/include/api/x64/bits/alltypes.h.sh b/include/api/x64/bits/ > alltypes.h.sh > index 5f19458..30d9f33 100755 > --- a/include/api/x64/bits/alltypes.h.sh > +++ b/include/api/x64/bits/alltypes.h.sh > @@ -90,13 +90,13 @@ TYPEDEF int pthread_spinlock_t; > TYPEDEF struct { union { int __i[14]; size_t __s[7]; } __u; } > pthread_attr_t; > TYPEDEF unsigned pthread_mutexattr_t; > TYPEDEF unsigned pthread_condattr_t; > -TYPEDEF unsigned pthread_barrierattr_t; > +TYPEDEF struct { unsigned pshared; } pthread_barrierattr_t; > This change (and the similar change for pthread_barrier_t below) is ok, and properly ABI-compatible (namely, the same size structure as on Linux), and I guess I can leave with it. But before we decide, please let me explain why these structures look the way they look now, and let you consider if you want to leave them as they were: The idea is that all these pthread_*_t are "opaque" - their users shouldn't access individual fields, or know what they mean. So all of them, including the ones we did implement - like pthread_mutex_t, have this opaque array of ints (4 bytes each) or pointers (8 bytes each) to control their size, and alignment (when the union has a pointer member, it gets 8-byte alignment), to be identical to what Linux has. Then, the actual implementation of the pthread function (in pthread.cc) casts the pointer to the opaque type, to a pointer to the actual type, which has the same (or smaller) size. You could do the same for these two types too, without changing this header file. > > TYPEDEF struct { unsigned __attr[2]; } pthread_rwlockattr_t; > > TYPEDEF struct { union { int __i[10]; void *__p[5]; } __u; } > pthread_mutex_t; > TYPEDEF struct { union { int __i[12]; void *__p[6]; } __u; } > pthread_cond_t; > TYPEDEF struct { union { int __i[14]; void *__p[7]; } __u; } > pthread_rwlock_t; > -TYPEDEF struct { union { int __i[8]; void *__p[4]; } __u; } > pthread_barrier_t; > +TYPEDEF struct { unsigned int in; unsigned int out; unsigned int count; > void *latch; void *mtx; } pthread_barrier_t; > > TYPEDEF long off_t; > TYPEDEF long __off_t; > diff --git a/include/osv/latch.hh b/include/osv/latch.hh > index 6ff78a8..09430ea 100644 > --- a/include/osv/latch.hh > +++ b/include/osv/latch.hh > @@ -58,6 +58,13 @@ 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) > + void reset(int count) > + { > + _count.fetch_add(count, std::memory_order_release); > + } It's not clear to me when or how this reset() can actually be used correctly without a lot of addition machinery (like you added in barrier below). Imagine that N threads participate in the latch, all of them await() the count to be zero. Now, when the count does reach zero, all of them are woken up, and "someone" wants to call this reset() and everyone calls await() again. But who calls this reset()? If it is one of the threads that were woken up, then another thread who succeeded in calling await() for a second time *before* the one thread calls reset(), will not wait. Looking at the code below, I see you actually used reset() correctly by having yet another mutex, and counter, outside the latch. But if we can't use this reset() without these tricks, I think at least, reset should not pretend to be safer than it is - and use assignment (not fetch_add and memory_order_release), and a comment says it is not safe to use it without protecting it from a race against count_down(). There are perhaps other ways to avoid the reset() race besides the additional mutex you used, perhaps use another "round" counter to tell the thread being woken up whether it was actually woken up, without relying on the counter, or rely on our (but not pthread's!) condvar guarantee, that it does not have spurious wakeups, so if the condvar was woken, we know we were woken, and do not need to loop. I don't know which is best. Perhaps even what you did. But please don't be locked into the idea of reusing this existing "latch" structure (I don't even remember why we have it) - you can copy it (it's short) and do something slightly different, which unlike latch would allow several rounds. > }; > > class thread_barrier > diff --git a/libc/pthread.cc b/libc/pthread.cc > index 148b79e..251c669 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,81 @@ int pthread_attr_getaffinity_np(const > pthread_attr_t *attr, size_t cpusetsize, > > return 0; > } > + > +int pthread_barrier_init(pthread_barrier_t *barrier, > + const pthread_barrierattr_t *attr, > + unsigned count) > +{ > + if (count <= 0 || count >= INT_MAX) { > + return EINVAL; > Since count in and unsigned int, there isn't much point in checking <= 0, you can check for ==0. Why the >= INT_MAX check? Why not allow the full range of unsigned int? > + } > + > + // 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 > + barrier->count = count; > + barrier->in = 0; > + barrier->out = 0; > + barrier->latch = (void*) (new latch(count)); > barrier->latch could have the correct type, so you won't have to do this cast. To avoid an #include hell, you can do forward declaration, i.e., something like struct pthread_barrier_t { ... struct latch *latch; // use "struct latch", not "latch" so we don't need to include latch.hh Also, if your header file definitions are opaque, and you can use C++ in the real implementation structure, you can use std::unique_ptr<mutex> in the structure, and the new and delete will be automatic - you'll just need to use placement new and operator delete in the init() and destroy() functions. > + barrier->mtx = (void*) (new pthread_mutex_t); > Ditto. You can also use mutex directly instead of pthread_mutex_t. > + pthread_mutex_init((pthread_mutex_t*) barrier->mtx, NULL); > + return 0; > +} > + > +int pthread_barrier_wait(pthread_barrier_t *barrier) > +{ > + if (!barrier || !barrier->latch || !barrier->mtx) { > + return EINVAL; > + } > + > + int retval = 0; > + pthread_mutex_t *mtx = (pthread_mutex_t*) barrier->mtx; > + // Critical section to increment the number of incoming threads/waiters > + pthread_mutex_lock(mtx); > + barrier->in++; > + pthread_mutex_unlock(mtx); > Hmm, why do you need this "in" counter? The mutex_lock() you *do* need here (to wait until the previous round is over), but seems to me it could be an empty locked section: pthread_mutex_lock(mtx); pthread_mutex_unlock(mtx); > + > + latch *l = (latch*) barrier->latch; > + 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 > seems > + // technically correct. I'm not sure what you're saying here... Are you suggesting that someone creates a barrier with a counter of N, but then N+1 threads actually call thread_wait()? It seems that with pthread_barrier_wait(), the N+1'th thread will wait, as if it started the next round, but in your implementation it may not wait for the next round. I wouldn't call this "technically correct", but I'm also not worried about this because I think this seems to me a broken usage of the barrier (although I'm not actually sure, I don't see any mention of this in the documentation). > Only one of the crossing threads will get a > + // retval of PTHREAD_BARRIER_SERIAL_THREAD, when barrier->out % count > == 0. > + // 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 == 0) { > Why the "%" and not barrier->out == barrier->count, and then set it to zero? + retval = PTHREAD_BARRIER_SERIAL_THREAD; > + // Reset the latch for the next round of waiters > + l->reset(barrier->count); > + } > + pthread_mutex_unlock(mtx); > + return retval; > +} > + > +int pthread_barrier_destroy(pthread_barrier_t *barrier) > +{ > + if (!barrier || !barrier->latch || !barrier->mtx) { > + return EINVAL; > + } > + > + delete ((latch*) barrier->latch); > + barrier->latch = nullptr; > + > + delete ((pthread_mutex_t*) barrier->mtx); > If you use pthread_mutex_t (not mutex), delete() is not enough, you also need to use pthread_mutex_destroy. + 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.c b/tests/tst-pthread-barrier.c > new file mode 100644 > index 0000000..1d22e19 > --- /dev/null > +++ b/tests/tst-pthread-barrier.c > @@ -0,0 +1,99 @@ > +#include <stdio.h> > +#include <unistd.h> > +#include <memory.h> > +#include <errno.h> > +#include <stdbool.h> > +#include <pthread.h> > +#include <limits.h> > +#include <stdlib.h> > + > +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; > + > +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; > + sleep(delay); > Please use usleep() to have a much shorter test - no need to have this sleep for a whopping 7 seconds... > + > + 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); > + report("pthread_barrier_wait (special)", > + retval == PTHREAD_BARRIER_SERIAL_THREAD); > This doesn't test anything.... It just says that if retval as a particular value, you check if it has this particular value ;-) A better test would be to count (using an atomic variable, or whatever) the number of threads that got this special return value, and then check that it was exactly one thread for each crossing. Or something like that. > + } 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; > + numCrossings = 4; // Pass through the barrier k times > + 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); > Since barrier_init takes and unsigned count, what's the point of passing it -1? I'm surprised the compiler doesn't warn about this. > + 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); > + } > + 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.
