Re: (almost working) patch to make opencrypto use mutex/condvar

[Andrew Doran <ad%netbsd.org@localhost>]

On Sat, Feb 02, 2008 at 03:10:17PM +0000, Andrew Doran wrote:

> On Sat, Feb 02, 2008 at 07:26:43AM +0100, Pawel Jakub Dawidek wrote:
> 
> > On Fri, Feb 01, 2008 at 04:45:13PM -0500, Thor Lancelot Simon wrote:
> > > @@ -621,10 +635,13 @@
> > >  
> > >   error = crypto_kdispatch(krp);
> > >   if (error == 0)
> > > -         error = tsleep(krp, PSOCK, "crydev", 0);
> > > - if (error)
> > > +         mutex_spin_enter(&crypto_mtx);
> > > +         cv_wait(&krp->krp_cv, &crypto_mtx);     /* XXX cv_wait_sig? */
> > > +         mutex_spin_exit(&crypto_mtx);
> > > +#if 0
> > > + if (error)                      /* see XXX above -- wait intr? */
> > >           goto fail;
> > > -
> > > +#endif
> > 
> > You changed:
> > 
> >     if (error == 0)
> >             error = tsleep(krp, PSOCK, "crydev", 0);
> > 
> > to this:
> > 
> >     if (error == 0)
> >             mutex_spin_enter(&crypto_mtx);
> >             cv_wait(&krp->krp_cv, &crypto_mtx);     /* XXX cv_wait_sig? */
> >             mutex_spin_exit(&crypto_mtx);
> > 
> > and it should be of course:
> > 
> >     if (error == 0) {
> >             mutex_spin_enter(&crypto_mtx);
> >             cv_wait(&krp->krp_cv, &crypto_mtx);     /* XXX cv_wait_sig? */
> >             mutex_spin_exit(&crypto_mtx);
> >     }
> 
> That looks suspicious. What if the event being waited for occurs before
> mutex_spin_enter(&crypto_mtx)? Without the mutex being used to interlock
> it's just there to placate cv_wait().

Also, in order to handle spurious wakeups cv_wait() should always be in a
loop, or used in a restartable sequence. The same is true for tsleep().
So something like:

        mutex_spin_enter(&crypto_mtx);
        while (!condition) {
                cv_wait(&krp->krp_cv, &crypto_mtx);
        }
        mutex_spin_exit(&crypto_mtx);

Andrew