Date: Mon, 11 Mar 2002 15:40:42 -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: <20020311154042.A89113@colnta.acns.ab.ca> In-Reply-To: <20020311222102.GA92565@elvis.mu.org>; from bright@mu.org on Mon, Mar 11, 2002 at 02:21:02PM -0800 References: <20020311001131.GN26621@elvis.mu.org> <XFMail.020311120529.jhb@FreeBSD.org> <20020311222102.GA92565@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Mar 11, 2002 at 02:21:02PM -0800, Alfred Perlstein wrote:
> * John Baldwin <jhb@FreeBSD.org> [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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020311154042.A89113>