From owner-freebsd-smp Mon Mar 11 14:40:57 2002 Delivered-To: freebsd-smp@freebsd.org Received: from mail.acns.ab.ca (mail.acns.ab.ca [142.179.151.95]) by hub.freebsd.org (Postfix) with ESMTP id 892FC37B416; Mon, 11 Mar 2002 14:40:48 -0800 (PST) Received: from colnta.acns.ab.ca (colnta.acns.ab.ca [192.168.1.2]) by mail.acns.ab.ca (8.11.6/8.11.3) with ESMTP id g2BMehu25385; Mon, 11 Mar 2002 15:40:43 -0700 (MST) (envelope-from davidc@colnta.acns.ab.ca) Received: (from davidc@localhost) by colnta.acns.ab.ca (8.11.6/8.11.3) id g2BMegC89163; Mon, 11 Mar 2002 15:40:42 -0700 (MST) (envelope-from davidc) Date: Mon, 11 Mar 2002 15:40:42 -0700 From: Chad David To: Alfred Perlstein Cc: John Baldwin , smp@FreeBSD.org Subject: Re: select fix and giant pushdown patch Message-ID: <20020311154042.A89113@colnta.acns.ab.ca> References: <20020311001131.GN26621@elvis.mu.org> <20020311222102.GA92565@elvis.mu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <20020311222102.GA92565@elvis.mu.org>; from bright@mu.org on Mon, Mar 11, 2002 at 02:21:02PM -0800 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 Mon, Mar 11, 2002 at 02:21:02PM -0800, Alfred Perlstein wrote: > * John Baldwin [020311 09:05] wrote: > > > > > + 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(). > > No, actually this code needs to be fixed up. :) *slaps chad* > It should be a TAILQ_FOREACH followed by a TAILQ_INIT. :) man 9 queue Note: Faster TailQ Deletion. If you cannot trust the man page what can you trust ;-). > > > 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? > > Can do. I was waiting for Alfred to tell me to fix this but he never did, and then I forgot. > > > > > > + 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. > > I think we need an assertion that a cv can only be used with a single > mutex. And that the mutex is held? > > > > > > @@ -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. > > This is Chad's XXX, what he's talking about is why do we need to clear > the TDF_SELECT flag at all, not that it's done while holding sched lock > (which makes sense). > > I think there's actually a race here, we seem to only recheck the > TDF_SELECT flag if there was a timeout specified in the select call. > So if there wasn't a timeout it looks like we could loose a race > where an fd being waited on but we don't recheck if a selwakeup occured > on it. Am I missing something? Basically, I think this code should be > something like: > > mtx_lock_spin(&sched_lock); > if ((td->td_flags & TDF_SELECT) == 0) { > mtx_unlock_spin(&sched_lock); > goto restart; > } > td->td_flags &= ~TDF_SELECT; > mtx_unlock_spin(&sched_lock); I think you also need to check nselcoll. > > ?? > > Why was there extra hysterics put into the timeout code? > -- Chad David davidc@acns.ab.ca www.FreeBSD.org davidc@freebsd.org ACNS Inc. Calgary, Alberta Canada Fourthly, The constant breeders, beside the gain of eight shillings sterling per annum by the sale of their children, will be rid of the charge of maintaining them after the first year. - Johnathan Swift To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-smp" in the body of the message