Date: Mon, 11 Mar 2002 12:05:29 -0500 (EST) From: John Baldwin <jhb@FreeBSD.org> To: Alfred Perlstein <bright@mu.org> Cc: davidc@freebsd.org, smp@freebsd.org Subject: RE: select fix and giant pushdown patch Message-ID: <XFMail.020311120529.jhb@FreeBSD.org> In-Reply-To: <20020311001131.GN26621@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <jhb@FreeBSD.org> <>< 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?XFMail.020311120529.jhb>