On Tue, Mar 6, 2018 at 3:46 PM, Waldemar Kozaczuk <jwkozac...@gmail.com>
wrote:

> This patch adds logic to make sure that waitqueue object actually
> exists in queues map before it tries to dereference it.
>
> The content of this patch was authored by Benoit Canet.
>

Hi, I already reviewed this patch with Benoit, but unfortunately it was off
the mailing list.
I never understood, and still don't understand, what this patch fixes... :-(
Did it solve any problems in Go or any of your other workloads?

Your description, saying that you need to make sure the queue exists before
it is dereferenced, looks like
a wrong explanation to me: The original code held waitqueues in the map,
and the instruction "queues[uaddr]"
not only looks for a waitqueue with that key - if it is missing, it creates
a new one! So this did not need fixing
at all....

What this code does change is to change the direct "waitqueue" object to a
pointer std::shared_ptr<waitqueue>,
and since a shared_ptr's default constructor creates a null pointer (not a
pointer to the new object), in particular
Benoit had to stop using the operator[] and started using find().

The question is why this change was necessary.

In my review back then (a year ago), I tried to guess, but without success:

So you're doing the same except for one differences - you're using a shared
pointer instead of an object.
There can be two difference:
1. a shared_pointer means the object does not need to be "moved" - do we
have a problem with moving a waitqueue? (I didn't think so)
2. A shared pointer could be used to ensure the waitqueue lives longer.

I don't find any evidence of 2: Yes, while you're holding on "q" the queue
cannot disappear, but the only thing which can (I think) cause a queue to
disappear is a FUTEX_WAKE, and since the code here and in FUTEX_WAKE both
use the same mutex, they cannot happen in parallel anyway.

So I wonder what I'm missing....

Benoit, Waldek, any ideas?



>
> Signed-off-by: Waldemar Kozaczuk <jwkozac...@gmail.com>
> ---
>  linux.cc | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/linux.cc b/linux.cc
> index 25d28ee..0256c31 100644
> --- a/linux.cc
> +++ b/linux.cc
> @@ -54,7 +54,7 @@ extern "C" long gettid()
>  // The __cxa_guard_* functions only call futex in the rare case of
> contention,
>  // in fact so rarely that OSv existed for a year before anyone noticed
> futex
>  // was missing. So the performance of this implementation is not critical.
> -static std::unordered_map<void*, waitqueue> queues;
> +static std::unordered_map<void*, std::shared_ptr<waitqueue>> queues;
>  static mutex queues_mutex;
>  enum {
>      FUTEX_WAIT           = 0,
> @@ -71,12 +71,16 @@ int futex(int *uaddr, int op, int val, const struct
> timespec *timeout,
>      case FUTEX_WAIT:
>          WITH_LOCK(queues_mutex) {
>              if (*uaddr == val) {
> -                waitqueue &q = queues[uaddr];
> +                auto i = queues.find(uaddr);
> +                if (i == queues.end()) {
> +                    queues[uaddr] = std::make_shared<waitqueue>();
> +                }
> +                std::shared_ptr<waitqueue> q = queues[uaddr];
>

Nitpick: This code is a bit wierd, could have used emplace()  and *i,
instead of looking up the same address 3 times.

                 if (timeout) {
>                      sched::timer tmr(*sched::thread::current());
>                      tmr.set(std::chrono::seconds(timeout->tv_sec) +
>                              std::chrono::nanoseconds(timeout->tv_nsec));
> -                    sched::thread::wait_for(queues_mutex, tmr, q);
> +                    sched::thread::wait_for(queues_mutex, tmr, *q);
>                      // FIXME: testing if tmr was expired isn't quite
> right -
>                      // we could have had both a wakeup and timer
> expiration
>                      // racing. It would be more correct to check if we
> were
> @@ -86,7 +90,7 @@ int futex(int *uaddr, int op, int val, const struct
> timespec *timeout,
>                          return -1;
>                      }
>                  } else {
> -                    q.wait(queues_mutex);
> +                    q->wait(queues_mutex);
>                  }
>                  return 0;
>              } else {
> @@ -104,11 +108,11 @@ int futex(int *uaddr, int op, int val, const struct
> timespec *timeout,
>              auto i = queues.find(uaddr);
>              if (i != queues.end()) {
>                  int waken = 0;
> -                while( (val > waken) && !(i->second.empty()) ) {
> -                    i->second.wake_one(queues_mutex);
> +                while( (val > waken) && !(i->second->empty()) ) {
> +                    i->second->wake_one(queues_mutex);
>                      waken++;
>                  }
> -                if(i->second.empty()) {
> +                if(i->second->empty()) {
>                      queues.erase(i);
>                  }
>                  return waken;
> --
> 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 osv-dev+unsubscr...@googlegroups.com.
> 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 osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to