Re: condition variables

On Fri, Oct 28, 2016 at 9:38 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Oct 4, 2016 at 3:12 PM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>> On Mon, Aug 15, 2016 at 5:58 PM, Thomas Munro
>> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>>> Please find attached a -v2 of your patch which includes suggestions
>>> 1-3 above.
>>
>> Here's a rebased patch. ConditionVariableSleep now takes
>> wait_event_info. Anyone using this in patches for core probably needs
>> to add enumerators to the WaitEventXXX enums in pgstat.h to describe
>> their wait points.
>
> So, in my original patch, it was intended that cvSleeping was the
> definitive source of truth as to whether a particular backend is
> committed to sleeping or not. That had some bugs, I guess, but in
> your version, there are now two sources of truth. On the one hand,
> there's still cvSleeping. On the other hand, there's now also whether
> proclist_contains(&cv->wakeup, MyProc->pgprocno, cvWaitLink) returns
> true.
>
> I'm not sure whether that causes any serious problem. It seems
> possible, for example, that ConditionVariableSignal() could clear the
> cvSleeping flag for a backend that's now waiting for some OTHER
> condition variable, because once it pops the victim off the list and
> releases the spinlock, the other backend could meanwhile
> ConditionVariableCancelSleep() and then do anything it likes,
> including sleep on some other condition variable. Now I think that
> the worst thing that will happen is that the other backend will
> receive a spurious wakeup, but I'm not quite sure. The whole point of
> having cvSleeping in the first place is so that we don't get spurious
> wakeups just because somebody happens to set your process latch, so it
> seems a bit unfortunate that it doesn't actually prevent that from
> happening.
>
> I wonder if we shouldn't try to create the invariant that when the CV
> mutex is not help, the state of cvSleeping has to be true if we're in
> the proclist and false if we're not. So ConditionVariableSignal()
> would clear the flag before releasing the spinlock, for example. Then
> I think we wouldn't need proclist_contains().

Yeah, that'd work. ConditionVariableSleep would need to hold the
spinlock while checking cvSleeping (otherwise there is race case where
another process sets it to false but this process doesn't see that yet
and then waits for a latch-set which never arrives). It's not the end
of the world but it seems unfortunate to have to acquire and release
the spinlock in ConditionVariablePrepareToSleep and then immediately
again in ConditionVariableSleep.

I was going to code that up but I read this and had another idea:

http://stackoverflow.com/questions/8594591/why-does-pthread-cond-wait-have-spurious-wakeups

I realise that you didn't actually say you wanted to guarantee no
spurious wakeups. But since the client code already has to check its
own exit condition, why bother adding a another layer of looping?
Sprurious latch sets must be super rare, but even if they aren't, what
do you save by filtering them here? In this situation you already got
woken from a deep slumbler and scheduled back onto the CPU, so it
hardly matters whether you go around again in that loop or the
client's loop. We could make things really simple instead: get rid of
cvSleeping, have ConditionVariablePrepareToSleep reset the latch, then
have ConditionVariableSleep wait for the latch to be set just once (no
loop). Then we'd document that spurious wakeups are possible so the
caller must write a robust predicate loop, exactly as you already
showed in your first message. We'd need to keep that
proclist_contains stuff to avoid corrupting the list.
proclist_contains would be the one source of truth for whether you're
in the waitlist, and the client code's predicate loop would contain
the one source of truth for whether the condition it is waiting for
has been reached.

Thoughts?

--
Thomas Munro
http://www.enterprisedb.com