Date: Mon, 11 Mar 2002 15:29:03 -0800 From: Alfred Perlstein <bright@mu.org> To: Chad David <davidc@acns.ab.ca> Cc: John Baldwin <jhb@FreeBSD.org>, smp@FreeBSD.org Subject: Re: select fix and giant pushdown patch Message-ID: <20020311232903.GB92565@elvis.mu.org> In-Reply-To: <20020311154042.A89113@colnta.acns.ab.ca> References: <20020311001131.GN26621@elvis.mu.org> <XFMail.020311120529.jhb@FreeBSD.org> <20020311222102.GA92565@elvis.mu.org> <20020311154042.A89113@colnta.acns.ab.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
* Chad David <davidc@acns.ab.ca> [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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020311232903.GB92565>