From owner-freebsd-smp Mon Mar 11 15:29: 7 2002 Delivered-To: freebsd-smp@freebsd.org Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by hub.freebsd.org (Postfix) with ESMTP id D247437B405; Mon, 11 Mar 2002 15:29:03 -0800 (PST) Received: by elvis.mu.org (Postfix, from userid 1192) id 8F39FAE1FE; Mon, 11 Mar 2002 15:29:03 -0800 (PST) Date: Mon, 11 Mar 2002 15:29:03 -0800 From: Alfred Perlstein To: Chad David Cc: John Baldwin , smp@FreeBSD.org Subject: Re: select fix and giant pushdown patch Message-ID: <20020311232903.GB92565@elvis.mu.org> References: <20020311001131.GN26621@elvis.mu.org> <20020311222102.GA92565@elvis.mu.org> <20020311154042.A89113@colnta.acns.ab.ca> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20020311154042.A89113@colnta.acns.ab.ca> User-Agent: Mutt/1.3.27i 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 * Chad David [020311 14:40] wrote: > On Mon, Mar 11, 2002 at 02:21:02PM -0800, Alfred Perlstein wrote: > > > > 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 ;-). That only applies when removing the nodes from the list. :) UTSL sys/queue.h to see what TAILQ_FOREACH expands to. :) > > > 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. I've got it, it's td_selq. > > I think we need an assertion that a cv can only be used with a single > > mutex. > > And that the mutex is held? Yes. :) John is all about infrastructure, and we need such assertions in the code. :) > > 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. Good call. Are you sure about this? It's just odd that there's so much hysterics when a timeout is involved... should we just nuke that code and add the 'goto restart' here if the flag is unset OR we've had more collisions? -Alfred To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-smp" in the body of the message