Re: Introduce timeout capability for ConditionVariableSleep

Thank you reviewing. Comments inline.

On Wed, Mar 13, 2019 at 05:24:15PM +0900, Kyotaro HORIGUCHI wrote:
> > 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.

After thinking about this more, I see WaitEventSetWait()'s contract as
wait for an event or timeout if no events are received in that time
frame. Although ConditionVariableTimedSleep() is also using the same
word, I believe the semantics are different. The timeout period in
ConditionVariableTimedSleep() is intended to limit the time we wait
until removal from the wait queue. Whereas, in the case of
WaitEventSetWait, the timeout period is intended to limit the time the
caller waits till the first event.

Hence, I believe the code is correct as is and we shouldn't change the
contract for WaitEventSetWait.

> > 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.

Hmm, I don't agree with that completely. One could argue that the
backend is waiting for any event in order to be woken up, including a
timeout event.

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

Good to know! :-) Luckily git is smart enough to still match it to the
correct commit.

> > 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.

Agree. Changed to:

* Wait for the given condition variable to be signaled or till timeout.
*
* Returns -1 when timeout expires, otherwise returns 0.
*
* See ConditionVariableSleep() for general usage.

> +ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
>
> Counldn't the two-state return value be a boolean?

I wanted to leave the option open to use the positive integers for other
purposes but you are correct, bool suffices for now. If needed, we can
change it in the future.

> + 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()
> ...
> }

Agree. The timeout did complicate the logic for a single variable to
track the exit condition. Adopted the approach above.

> + Assert(ret == 0);
>
> I don't see a point in the assertion so much.

Being overly verbose. Removed.

--
Shawn Debnath
Amazon Web Services (AWS)