cc dev list ---------- Forwarded message ---------- From: Rean Griffith <[email protected]> Date: Mon, Dec 19, 2016 at 8:08 AM Subject: Re: [PATCH] pthread_barrier*: Add OSv-specific pthread_barrier_init, wait, destroy + test To: Nadav Har'El <[email protected]>
Thanks for the review comments! I will make another pass. More replies inline...[RG] On Mon, Dec 19, 2016 at 1:18 AM, Nadav Har'El <[email protected]> wrote: > 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. > [RG] Thanks for the explanation! I will look into adopting the same strategy after cleaning up other parts of the patch. >> >> 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. > [RG] Yes the latch reset isn't safe so it requires external locking. I'll see if there's an alternative. Modulo not having a reset() the OSv latch did most of what I was looking for but I can think about it some more. }; >> >> 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? > [RG] I'll fix the <= 0. Re: the INT_MAX limit: testing on linux returned EINVAL for INT_MAX, but there's no reason we couldn't differ and allow a wider range up to UINT_MAX + } >> + >> + // 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. > [RG] I'll look into this. > > >> + 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); > [RG] I initially thought that tracking the incoming threads would be useful, but so far it's not needed. I might end up removing it. > >> + >> + 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). > [RG] I couldn't find anything in the documentation about whether this is an issue either so I assumed that it's not wrong (but who knows). > > >> 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? > [RG] No specific reason for using %, barrier->out == barrier->count should also work. + 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. > [RG] Right! I will add the call to pthread_mutex_destroy before the delete. > + 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... > [RG] Sure. + >> + 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. > [RG] Sure, I'll revisit. > >> + } 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. > [RG] Just passing in invalid data to see what happened. I don't remember seeing a compiler warning about this. thanks, Rean > > >> + 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.
