Hi Nadav,
Thanks for the review comments. I will clean up the patch and re-submit
tonight.
I found this issue while building OSv images for Nodejs v7.0.0 and 7.2.0.
The GC in Nodejs 7.x does a semaphore wait-for operation that eventually
triggers this issue (/node-7.0.0/deps/v8/src/heap/mark-compact.cc) causing
a nodejs application to die if/when a GC starts:
bool MarkCompactCollector::Sweeper::IsSweepingCompleted() {
while (pending_sweeper_tasks_semaphore_.WaitFor(
base::TimeDelta::FromSeconds(0))) {
num_sweeping_tasks_.Increment(-1);
}
return num_sweeping_tasks_.Value() == 0;
}
More replies inline [RG]
On Wednesday, December 7, 2016, Nadav Har'El <[email protected]> wrote:
> 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.
>
[RG] Sure. This comment isn't useful, I'll remove it
>
>
+ 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 :-)
>
[RG] I will give the direct erase route a try. That would be much simpler!
If it doesn't work I'll leave the search but add a comment that the direct
erase didn't work.
> + // 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).
>
> [RG] I wasn't sure whether this if() could succeed either. Since it seems
unlikely I'll remove it.
> + // Remove our wait_record
>
This comment is redundant (a few lines above, you explained that you are
> removing our wait record from the list)
>
> [RG] I'll remove this comment
> + _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.
>
[RG] I will give that a tr
> +
>> + 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.
>
>
[RG] I'll look at wall::time_point. I just wanted to have an immediate
timeout.
> + 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
>>
>> thanks,
Rean
> --
>> 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.
