svn commit: r308424 - head/sys/arm/broadcom/bcm2835

[John Baldwin]

On Monday, November 07, 2016 08:32:17 PM Hans Petter Selasky wrote:
> On 11/07/16 20:23, Oleksandr Tymoshenko wrote:
> >
> >> On Nov 7, 2016, at 10:27 AM, Hans Petter Selasky <hps at selasky.org> wrote:
> >>
> >> On 11/07/16 18:38, Oleksandr Tymoshenko wrote:
> >>> +		bcm2835_audio_unlock(sc);
> >>> +		cv_signal(&sc->worker_cv);
> >>
> >>
> >> Shouldn't cv_signal() be done locked, so that you don't loose any transactions? CV's only wakeup the treads that are sleeping right there and then.
> >
> > Hi Hans,
> >
> > In this case it doesn’t matter. bcm2835_audio_xxx lock functions are used to keep channel state consistent. The actual audio hw reprogramming happens in worker thread which only picks up latest state of the virtual channel, there is no need to run every transaction in sequence.
> >
> 
> Hi,
> 
> It is not about running in sequence, but that if the worker thread is 
> not sleeping, but on the way to sleep, it will never get woken up unless 
> you use proper locks here!

You do not have to hold locks across cv_signal/broadcast or wakeup.  You do
have to hold them across the check determining whether you should sleep.
Take this simple example:

	lock(&m);
	while (should_sleep)
		cv_wait(&cv, &m);
	unlock(&m);

	...


	lock(&m);
	should_sleep = true;
	unlock(&m);
	cv_signal(&cv);

A thread that locks 'm' after the 'unlock' but before the cv_signal will see
'should_sleep' as false and will not call cv_wait().  The cv_signal of course
might then wakeup a second thread prematurely, but that's why you should
always use a while loop with cv wait operations (same is true with pthread
condvars btw).  On the other hand, doing the wakeup outside of the lock
avoids preempting during the wakeup only to immediately block on the lock and
switch back to the thread that did the wakeup.

-- 
John Baldwin