Date: Mon, 11 Mar 2002 16:48:40 -0700 From: Chad David <davidc@acns.ab.ca> To: Alfred Perlstein <bright@mu.org> Cc: John Baldwin <jhb@FreeBSD.org>, smp@FreeBSD.org Subject: Re: select fix and giant pushdown patch Message-ID: <20020311164840.A89277@colnta.acns.ab.ca> In-Reply-To: <20020311232903.GB92565@elvis.mu.org>; from bright@mu.org on Mon, Mar 11, 2002 at 03:29:03PM -0800 References: <20020311001131.GN26621@elvis.mu.org> <XFMail.020311120529.jhb@FreeBSD.org> <20020311222102.GA92565@elvis.mu.org> <20020311154042.A89113@colnta.acns.ab.ca> <20020311232903.GB92565@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Mar 11, 2002 at 03:29:03PM -0800, Alfred Perlstein wrote:
> * 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. :)
I know, I know. I wrote some of this at 4am, and I'm still feeling
stupid about all of the other dump things I did, so don't rub it in :(.
>
> > > 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?
I think you need the extra call to selscan() in the timedout case so that
you will return with the most up to date bits set. You could put a check
right after we lock sellock and remove the timeout check, but if we are
timedout you probably do not want to loop there, but instead just update
the bits and return?
I would remove the td->td_flags &= ~TDF_SELECT part all together after the
timeout check as is gets cleared right after done: anyway.
--
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020311164840.A89277>