kern/57945: [patch] Add locking to kqueue to make it MP-safe
Bruce Evans
bde at zeta.org.au
Tue Oct 14 12:00:33 PDT 2003
The following reply was made to PR kern/57945; it has been noted by GNATS.
From: Bruce Evans <bde at zeta.org.au>
To: Stefan Farfeleder <stefan at fafoe.narf.at>
Cc: freebsd-gnats-submit at freebsd.org
Subject: Re: kern/57945: [patch] Add locking to kqueue to make it MP-safe
Date: Wed, 15 Oct 2003 04:50:03 +1000 (EST)
On Mon, 13 Oct 2003, Stefan Farfeleder wrote:
> On Tue, Oct 14, 2003 at 03:56:19AM +1000, Bruce Evans wrote:
> > > - error = tsleep(kq, PSOCK | PCATCH, "kqread", timeout);
> > > + error = msleep(kq, &kq->kq_mtx, PSOCK | PCATCH | PDROP,
> > > + "kqread", timeout);
> > > }
> > > - splx(s);
> >
> > Not copying the spl locking seems to give a bug here. msleep() returns with
> > the mutex held.
>
> The PDROP flag should prevent msleep() from reacquiring the lock.
Oops.
> > > + kq->kq_count--;
> > > + mtx_unlock(&kq->kq_mtx);
> > > if (kn->kn_status & KN_DISABLED) {
> > > kn->kn_status &= ~KN_QUEUED;
> > > - kq->kq_count--;
> > > + mtx_lock(&kq->kq_mtx);
> > > continue;
> > > }
> >
> > I don't see any reason to unlock/lock here. There were no splx()/splhigh()'s.
> > splhigh() is very global but kq_mtx is very local so keeping the
> > critical sections short should be much less needed than before.
>
> Ah, right. So it should look like this?
>
> + kq->kq_count--;
> if (kn->kn_status & KN_DISABLED) {
> kn->kn_status &= ~KN_QUEUED;
> - kq->kq_count--;
> continue;
> }
> + mtx_unlock(&kq->kq_mtx);
OK.
> > BTW, what locks kn?
>
> Nothing at the moment, I hoped it wouldn't be necessary. Do you have a
> specific race in mind?
Nothing specific. It must be locked by something. Giant I guess.
Bruce
More information about the freebsd-bugs
mailing list