Re: Introduce timeout capability for ConditionVariableSleep

Hello.

At Tue, 12 Mar 2019 17:53:43 -0700, Shawn Debnath <sdn(at)amazon(dot)com> wrote in <20190313005342(dot)GA8301(at)f01898859afd(dot)ant(dot)amazon(dot)com>
> Hi Thomas,
>
> Thanks for reviewing!
>
> On Wed, Mar 13, 2019 at 12:40:57PM +1300, Thomas Munro wrote:
> > Can we just refer to the other function's documentation for this? I
> > don't want two copies of this blurb (and this copy-paste already
> > failed to include "Timed" in the example function name).
>
> Hah - point well taken. Fixed.
>
> > One difference compared to pthread_cond_timedwait() is that pthread
> > uses an absolute time and here you use a relative time (as we do in
> > WaitEventSet API). The first question is which makes a better API,
> > and the second is what the semantics of a relative timeout should be:
> > start again every time we get a spurious wake-up, or track time
> > already waited? Let's see...
> >
> > - (void) WaitEventSetWait(cv_wait_event_set, -1, &event, 1,
> > + ret = WaitEventSetWait(cv_wait_event_set, timeout, &event, 1,
> > wait_event_info);
> >
> > Here you're restarting the timeout clock every time through the loop
> > without adjustment, and I think that's not a good choice: spurious
> > wake-ups cause bonus waiting.
>
> Agree. In my testing WaitEventSetWait did the work for calculating the
> right timeout remaining. It's a bummer that we have to repeat the same
> pattern at the ConditionVariableTimedSleep() but I guess anyone who
> loops in such cases will have to adjust their values accordingly.

I think so, too. And actually the duplicate timeout calculation
doesn't seem good. We could eliminate the duplicate by allowing
WaitEventSetWait to exit by unwanted events, something like the
attached.

> BTW, I am curious why Andres in 98a64d0bd71 didn't just create an
> artificial event with WL_TIMEOUT and return that from
> WaitEventSetWait(). Would have made it cleaner than checking checking
> return values for -1.

Maybe because it is not a kind of WaitEvent, so it naturally
cannot be a part of occurred_events.

# By the way, you can obtain a short hash of a commit by git
# rev-parse --short <full hash>.

> Updated v2 patch attached.

I'd like to comment on the patch.

+ * In the event of a timeout, we simply return and the caller
+ * calls ConditionVariableCancelSleep to remove themselves from the
+ * wait queue. See ConditionVariableSleep() for notes on how to correctly check
+ * for the exit condition.
+ *
+ * Returns 0, or -1 if timed out.

Maybe this could be more simpler, that like:

* ConditionVariableTimedSleep - allows us to specify timeout
*
* If timeout = =1, block until the condition is satisfied.
*
* Returns -1 when timeout expires, otherwise returns 0.
*
* See ConditionVariableSleep() for general behavior and usage.

+int
+ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,

Counldn't the two-state return value be a boolean?

+ int ret = 0;

As a general coding convention, we are not to give such a generic
name for a variable with such a long life if avoidable. In the
case the 'ret' could be 'timeout_fired' or something and it would
be far verbose.

+ if (rc == 0 && timeout >= 0)

WaitEventSetWait returns 0 only in the case of timeout
expiration, so the second term is useless. Just setting ret to
-1 and break seems to me almost the same with "goto". The reason
why the existing ConditionVariableSleep uses do {} while(done) is
that it is straightforwad. Timeout added everal exit point in the
loop so it's make the loop rather complex by going around with
the variable. Whole the loop could be in the following more flat
shape.

while (true)
{
CHECK_FOR_INTERRUPTS();
rc = WaitEventSetWait();
ResetLatch();

/* timeout expired, return */
if (rc == 0) return -1;
SpinLockAcquire();
if (!proclist...)
{
done = true;
}
SpinLockRelease();

/* condition satisfied, return */
if (done) return 0;

/* if we're here, we should wait for the remaining time */
INSTR_TIME_SET_CURRENT()
...
}

+ Assert(ret == 0);

I don't see a point in the assertion so much.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center