Thanks, good catch (I have egg on my face for inserting this oversight in the first place...). I'm curious, how did you notice this bug? What kind of visible problem did it fix? Did it break an actual application?
I have some comments and requests below. Thanks! Nadav. -- Nadav Har'El [email protected] On Wed, Dec 7, 2016 at 8:51 AM, 'rean' via OSv Development < [email protected]> wrote: > Signed-off-by: rean <[email protected]> > --- > core/semaphore.cc | 23 +++++++++++++++++++++ > modules/tests/Makefile | 2 +- > tests/tst-sem-timed-wait.cc | 50 ++++++++++++++++++++++++++++++ > +++++++++++++++ > 3 files changed, 74 insertions(+), 1 deletion(-) > create mode 100644 tests/tst-sem-timed-wait.cc > > diff --git a/core/semaphore.cc b/core/semaphore.cc > index 73732b8..6081a87 100644 > --- a/core/semaphore.cc > +++ b/core/semaphore.cc > @@ -46,6 +46,29 @@ bool semaphore::wait(unsigned units, sched::timer* tmr) > sched::thread::wait_until(_mtx, > [&] { return (tmr && tmr->expired()) || !wr.owner; }); > > + // If wr.owner is not nullptr then there was a timeout (post() did not > + // wake us). In that case, just remove the wait_record (local > variable) > + // that we just pushed onto _waiters (via push_back) > An important comment here is that we're holding the lock at this point, so can work on the wait list without race with post(). Strangely, I did already write that comment (below)! But there was no code after it! I guess I was distracted while submitting that patch :-( So your change should probably go after that existing comment. > + if (wr.owner) { > + auto i = _waiters.begin(); > + // _val can be 0 right now, so we can't use the same condition that > + // post_unlocked uses "_val > 0 && i != _waiters.end()" > I don't think this comment is relevant - clearly post_unlocked() does something different (it needs to loop over all waiters it can wake) while all we need to do here is to search for a specific record, so we just use an ordinary loop over the waiters until we find the one we need. > + while (i != _waiters.end()) { > + auto wait_rec = i++; > + if (wait_rec->owner == sched::thread::current()) { > Well, I guess you can indeed compare the owner like you did, and nothing will be wrong, but I think a more dire test would be to simply search for wr itself! I.e., something like if (wait_rec == wr) or maybe if (&*wait_rec == wr) But even more importantly, since we already *have* wr, do we really need to search for it in list just to remove it from the list? I don't remember the details right now (please look into it), but I seem to remember that the boost-intrusive list we use is a doubly linked list, which means that given an item that we know must be in the list (wr), we can remove it from the list immediately, without firs finding it inside the list. If you can do that, most of the code in your patch becomes redundant, and all we need to add is one line of code, erasing wr from the list if wr.owner :-) > + // We found our wait record so remove it from the list. > + // There is no need to wake ourselves or set wr.owner = > nullptr > + // we want to remember that we timed out (if post happened > then > + // wr.owner would have been set to nullptr) > + if (wait_rec->units <= _val) { > + _val -= wait_rec->units; > + } > I don't think that this if() can ever succeed: consider that every time we added units, in post(), we checked all the waiters - including this one, if their units <= val. So I don't see how we can wake up without a post() and discover val is big enough. If I'm correct here, this entire if() - and three of the comment lines above it - should be removed? Note that even if you can explain why this if() case *can* happen, you will need to return "true" if you succeeded to take the units, since that is what the return boolean means. One way to cause the code the code below (which has return !wr.owner) to return "true" is to set wr.owner = nullptr here (and of course, if you do that the comment above becomes wrong). + // Remove our wait_record This comment is redundant (a few lines above, you explained that you are removing our wait record from the list) + _waiters.erase(wait_rec); > + } > + } > + } > + > // if wr.owner, it's a timeout - post() didn't wake us and didn't > decrease > // the semaphore's value for us. Note we are holding the mutex, so > there > // can be no race with post(). > diff --git a/modules/tests/Makefile b/modules/tests/Makefile > index fb8d001..6e3aea7 100644 > --- a/modules/tests/Makefile > +++ b/modules/tests/Makefile > @@ -84,7 +84,7 @@ tests := tst-pthread.so misc-ramdisk.so tst-vblk.so > tst-bsd-evh.so \ > tst-namespace.so tst-without-namespace.so payload-env.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-ifaddrs.so tst-pthread-affinity-inherit.so > tst-sem-timed-wait.so > > # libstatic-thread-variable.so tst-static-thread-variable.so \ > > diff --git a/tests/tst-sem-timed-wait.cc b/tests/tst-sem-timed-wait.cc > new file mode 100644 > index 0000000..f401ed0 > --- /dev/null > +++ b/tests/tst-sem-timed-wait.cc > @@ -0,0 +1,50 @@ > +#include <osv/semaphore.hh> > +#include <stdio.h> > +#include <stdbool.h> > +#include <unistd.h> > +#include <errno.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; > +} > + > +int main(void) > +{ > + printf("Starting sem_timed_wait test\n"); > + > + // Basic flow for test > + // 1) Create a semaphore (initialized to 0) > + // 2) Do a timed-wait on it > + // 3) We're never signaled/woken so our stack-allocated wait_record > remains > + // on the semaphore's waiters list > + // > + // In the failure case the end result is a stacktrace that looks like: > + // Assertion failed: !hook.is_linked() > + //(/usr/include/boost/intrusive/detail/generic_hook.hpp: > destructor_impl: 47) > + > + //[backtrace] > + //0x0000000000225a48 <__assert_fail+24> > + //0x00000000003c40e9 <???+3948777> > + //0x00000000003c4242 <semaphore::wait(unsigned int, sched::timer*)+98> > + //0x0000100000c01057 <???+12587095> > + > + semaphore sem(0); > + timespec ts; > + ts.tv_sec = 0; > + ts.tv_nsec = 0; > You didn't need to create this "timespec" variable... You can just use use std::chrono::seconds(0) below and that's it - you don't even need to add 0 nanoseconds to it, as it obviously adds nothing. + > + sched::timer tmr(*sched::thread::current()); > + osv::clock::wall::time_point time(std::chrono::seconds(ts.tv_sec) + > + std::chrono::nanoseconds(ts. > tv_nsec)); > Nitpick: I'm not sure you meant to use wall::time_point here, I am guessing you actually meant a wall::duration here - so you'll get a timeout in 0 seconds - not a timeout in the absolute date 0 (which means 1970). But what you've done here isn't wrong either - it just means you want a timeout in the long past (so it will timeout immediately). It's just strange :-) Anyway, feel free not to change. > + tmr.set(time); > + bool ret_val = sem.wait(1, &tmr); > + report("sem_timedwait\0", ret_val == false); > + 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.
