Re: Introduce timeout capability for ConditionVariableSleep

Hello.

At Thu, 14 Mar 2019 17:26:11 -0700, Shawn Debnath <sdn(at)amazon(dot)com> wrote in <20190315002611(dot)GA1119(at)f01898859afd(dot)ant(dot)amazon(dot)com>
> 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

Sure.

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

Mmm. The two look the same to me.. Timeout means for both that
"Wait for one of these events or timeout expiration to
occur". Removal from waiting queue is just a subtask of exiting
from waiting state.

The "don't exit until timeout expires unless any expected events
occur" part is to be done in the uppermost layer so it is enough
that the lower layer does just "exit when something
happened". This is the behavior of WaitEventSetWaitBlock for
WaitEventSetWait. My proposal is giving callers capabliy to tell
WaitEventSetWait not to perform useless timeout contination.

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

Right, I understand that. I didn't mean that it is the right
design for everyone. Just meant that it is in that shape. (And I
rather like it.)

latch.h:127
#define WL_TIMEOUT (1 << 3) /* not for WaitEventSetWait() */

We can make it one of the events for WaitEventSetWait, but I
don't see such a big point on that, and also that doesn't this
patch better in any means.

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

And too complex so that infrequent usage easily get out from my
brain:(

> > > Updated v2 patch attached.

Thank you . It looks fine execpt the above point. But still I
have some questions on it. (the reason for they not being
comments is that they are about wordings..).

+ * Track the current time so that we can calculate the remaining timeout
+ * if we are woken up spuriously.

I think tha "track" means chasing a moving objects. So it might
be bettter that it is record or something?

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

Yes, we can do that after we found it needed.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center