Date: Tue, 14 Oct 2003 12:00:31 -0700 (PDT) From: Bruce Evans <bde@zeta.org.au> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/57945: [patch] Add locking to kqueue to make it MP-safe Message-ID: <200310141900.h9EJ0V4e065059@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/57945; it has been noted by GNATS. From: Bruce Evans <bde@zeta.org.au> To: Stefan Farfeleder <stefan@fafoe.narf.at> Cc: freebsd-gnats-submit@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200310141900.h9EJ0V4e065059>