From owner-freebsd-smp Mon Mar 11 9: 6:47 2002 Delivered-To: freebsd-smp@freebsd.org Received: from mail6.speakeasy.net (mail6.speakeasy.net [216.254.0.206]) by hub.freebsd.org (Postfix) with ESMTP id 2721C37B433 for ; Mon, 11 Mar 2002 09:05:30 -0800 (PST) Received: (qmail 20968 invoked from network); 11 Mar 2002 17:05:29 -0000 Received: from unknown (HELO server.baldwin.cx) ([216.27.160.63]) (envelope-sender ) by mail6.speakeasy.net (qmail-ldap-1.03) with DES-CBC3-SHA encrypted SMTP for ; 11 Mar 2002 17:05:29 -0000 Received: from laptop.baldwin.cx (gw1.twc.weather.com [216.133.140.1]) by server.baldwin.cx (8.11.6/8.11.6) with ESMTP id g2BH5lv22275; Mon, 11 Mar 2002 12:05:47 -0500 (EST) (envelope-from jhb@FreeBSD.org) Message-ID: X-Mailer: XFMail 1.4.0 on FreeBSD X-Priority: 3 (Normal) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit MIME-Version: 1.0 In-Reply-To: <20020311001131.GN26621@elvis.mu.org> Date: Mon, 11 Mar 2002 12:05:29 -0500 (EST) From: John Baldwin To: Alfred Perlstein Subject: RE: select fix and giant pushdown patch Cc: davidc@freebsd.org, smp@freebsd.org Sender: owner-freebsd-smp@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org On 11-Mar-02 Alfred Perlstein wrote: > Index: dev/kbd/kbd.c > =================================================================== > RCS file: /home/ncvs/src/sys/dev/kbd/kbd.c,v > retrieving revision 1.27 > diff -u -r1.27 kbd.c > --- dev/kbd/kbd.c 12 Sep 2001 08:37:06 -0000 1.27 > +++ dev/kbd/kbd.c 10 Mar 2002 08:08:36 -0000 > @@ -524,7 +524,7 @@ > #endif > clist_alloc_cblocks(&sc->gkb_q, KB_QSIZE, KB_QSIZE/2); /* XXX */ > sc->gkb_rsel.si_flags = 0; > - sc->gkb_rsel.si_pid = 0; > + SEL_INIT(&sc->gkb_rsel); > splx(s); > > return 0; softc's are already zero'd so you don't need this at all. > Index: dev/usb/ums.c > =================================================================== > RCS file: /home/ncvs/src/sys/dev/usb/ums.c,v > retrieving revision 1.48 > diff -u -r1.48 ums.c > --- dev/usb/ums.c 15 Feb 2002 22:54:10 -0000 1.48 > +++ dev/usb/ums.c 10 Mar 2002 08:08:36 -0000 > @@ -345,7 +345,11 @@ > sc->status.dx = sc->status.dy = sc->status.dz = 0; > > sc->rsel.si_flags = 0; > +#ifdef __FreeBSD__ > + SEL_INIT(&sc->rsel); > +#else > sc->rsel.si_pid = 0; > +#endif > > sc->dev = make_dev(&ums_cdevsw, device_get_unit(self), > UID_ROOT, GID_OPERATOR, Same here. > Index: isa/psm.c > =================================================================== > RCS file: /home/ncvs/src/sys/isa/psm.c,v > retrieving revision 1.43 > diff -u -r1.43 psm.c > --- isa/psm.c 19 Dec 2001 13:32:21 -0000 1.43 > +++ isa/psm.c 10 Mar 2002 08:08:36 -0000 > @@ -1314,8 +1314,7 @@ > device_busy(devclass_get_device(psm_devclass, unit)); > > /* Initialize state */ > - sc->rsel.si_flags = 0; > - sc->rsel.si_pid = 0; > + SEL_INIT(&sc->rsel); > sc->mode.level = sc->dflt_mode.level; > sc->mode.protocol = sc->dflt_mode.protocol; > sc->watchdog = FALSE; And here. > Index: kern/kern_ktrace.c > =================================================================== > RCS file: /home/ncvs/src/sys/kern/kern_ktrace.c,v > retrieving revision 1.60 > diff -u -r1.60 kern_ktrace.c > --- kern/kern_ktrace.c 27 Feb 2002 19:10:50 -0000 1.60 > +++ kern/kern_ktrace.c 10 Mar 2002 08:08:36 -0000 > @@ -181,6 +181,8 @@ > > if (error) > return; > + > + mtx_lock(&Giant); > /* > * don't let p_tracep get ripped out from under us > */ > @@ -200,6 +202,7 @@ > vrele(vp); > FREE(kth, M_KTRACE); > p->p_traceflag &= ~KTRFAC_ACTIVE; > + mtx_unlock(&Giant); > } This will probably result in lots of nice lock order reversals. That is why I'm working on getting things like this out from under Giant rather than bogusly pushing Giant down into them. Note that pushing Giant down into foo_drop routines or foo_free (we should pick a consistent name for those btw) is probably a good idea to handle things like fdrop() and crfree(). > + si = TAILQ_FIRST(&td->td_selinfo); > + while (si != NULL) { > + nsi = TAILQ_NEXT(si, si_thrlist); > + si->si_thread = NULL; > + si = nsi; > + } > + TAILQ_INIT(&td->td_selinfo); > +} > + > +/* Maybe: while(!TAILQ_EMPTY(&td->td_selinfo) { si = TAILQ_FIRST(...); nsi = TAILQ_NEXT(...); ... } w/o the TAILQ_INIT(). Also, calling the field td_selinfo makes it look like you have embedded a selinfo in the thread instead of a list of them. Maybe something like td_silist or something that makes it clear it's a head of a list and not a selinfo itself? > + mtx_lock(&sellock); > retry: > + > ncoll = nselcoll; > mtx_lock_spin(&sched_lock); > td->td_flags |= TDF_SELECT; > mtx_unlock_spin(&sched_lock); > - PROC_UNLOCK(td->td_proc); > + mtx_unlock(&sellock); > + > + /* XXX Is there a better place for this? */ > + TAILQ_INIT(&td->td_selinfo); Yes, init it when the thread is created at fork() for now (Julian will move it when needed). It only needs the INIT once. The PROC_LOCK's were oringally due to TDF_SELECT being a p_flag and can definitely go away now that it is a td_flag. > @@ -838,23 +864,28 @@ > timo = ttv.tv_sec > 24 * 60 * 60 ? > 24 * 60 * 60 * hz : tvtohz(&ttv); > } > + > + /* XXX What is the point of this? */ > mtx_lock_spin(&sched_lock); > td->td_flags &= ~TDF_SELECT; > mtx_unlock_spin(&sched_lock); You have to have the lock to make sure no one else is writing to td_flags and to make sure you don't have a stale value while performing the update. Thanks for fixing select() btw. I was also thinking about how to fix this as I mentioned to you earlier. The pfind() was definitely evil. I don't like having a global lock per-se, but I can't think of a better way to do this myself. > + TAILQ_REMOVE(&td->td_selinfo, sip, si_thrlist); > + mtx_lock_spin(&sched_lock); > + if (td->td_wchan == (caddr_t)&selwait) { > + if (td->td_proc->p_stat == SSLEEP) > + setrunnable(td); > + else > + cv_waitq_remove(td); > + } else > + td->td_flags &= ~TDF_SELECT; > + mtx_unlock_spin(&sched_lock); This can probably be simplified now to something like this: mtx_lock_spin(&sched_lock); if (td->td_proc->p_stat == SSLEEP) setrunnable(td); mtx_unlock_spin(&sched_lock); I would rather you commit the select stuff by itself and do the Giant pushdown's afterwards though I might like to review just that part of the diff once the select changes are in. Also, the XXXKSE comment in sys/select.h about si_thread can go away now. -- John Baldwin <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-smp" in the body of the message