Question about cv_signal(9) (never mind)

[John Polstra]

On 14-Jun-2004 John Baldwin wrote:
> On Monday 14 June 2004 02:53 pm, John Polstra wrote:
>> On 14-Jun-2004 John Baldwin wrote:
>> > On Saturday 12 June 2004 08:12 pm, John Polstra wrote:
>> >> On 12-Jun-2004 John Polstra wrote:
>> >> > [Why does a caller to cv_signal(9) have to hold the associated mutex?]
>> >>
>> >> Never mind.  I understand now.  It allows the implementation to
>> >> avoid doing any locking internally.  That seems perfectly
>> >> reasonable, and I withdraw my question.
>> >
>> > To be honest, it's also largely there to try to keep people from writing
>> > code that can lose wakeups.  The count optimization came later.  If the
>> > optimization of dropping the lock is more important and we think that
>> > people really won't make the mistake of not using locks when they should
>> > to avoid the lost wakeups then we could drop the count optimization and
>> > allow cv_signal() to not require the lock perhaps.
>>
>> It seems to me that some sort of mutual exclusion is needed when
>> manipulating the cv structure and the associated sleep queue.  If
>> the user doesn't provide it then the implementation will have to
>> provide it internally.  So unless there is a cheaper kind of mutual
>> exclusion that can be used inside the implementation, your current
>> solution seems the best to me.
>>
>> I had never thought through the need for some kind of mutual
>> exclusion, and the papers that describe the optimization I mentioned
>> simply ignore it and its performance implications.
> 
> Well, the sleep queue is not protected by the data lock, it is protected by a 
> spin lock associated with the hash table bucket that the condition variable's 
> address hashes to.  Specifically, note that sleepq_lookup() locks the 
> associated spin lock and returns with it locked where as sleepq_release(), 
> etc. unlocks the spin lock.  The sleepq is locked inside sleepq_signal() and 
> sleepq_wakeup().  If you don't implement the count optimization, then 
> cv_signal() just calls sleepq_signal() and cv_wakeup() just calls 
> sleepq_wakeup() with no need for any locking of the cv structure itself.  See 
> rev 1.47 of kern_condvar.c and the current implementations of wakeup() and 
> wakeup_one() for example.  The other member of struct condvar is static in 
> that it is only set during initialization and doesn't need any locking.

Thanks for clarifying that.  In that case, I think it's definitely
worthwhile to remove the requirement that the caller hold the mutex
when calling cv_signal.  I remember reading about some benchmark
results once (using userland POSIX threads) in which applying the
simple wakeup optimization made a very significant improvement.  Sorry
for the lack of specifics here.  It was a long time ago.  Measurements
aside, it's clear that the current restriction practically guarantees
two context switches for every condvar wakeup.  Reducing that to one
context switch would surely be worthwhile.

I think the kinds of bugs that cause programmers to lose wakeups
usually occur in the code that's waiting rather than in the code
that's signaling.  So lifting the restriction probably wouldn't result
in folks making any mistakes that they wouldn't have made regardless.
Also, anybody who's had experience using POSIX threads is already
used to the idea of not having to hold the mutex when signaling the
condition variable.

John