Date: Mon, 11 Mar 2002 17:43:41 -0500 (EST) From: John Baldwin <jhb@FreeBSD.org> To: Alfred Perlstein <bright@mu.org> Cc: smp@freebsd.org, davidc@freebsd.org Subject: Re: select fix and giant pushdown patch Message-ID: <XFMail.20020311174341.jhb@FreeBSD.org> In-Reply-To: <20020311222102.GA92565@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11-Mar-2002 Alfred Perlstein wrote:
>> Note that pushing Giant down into foo_drop routines or foo_free (we should
>> pick
>> a consistent name for those btw) is probably a good idea to handle things
>> like
>> fdrop() and crfree().
>>
>> > + 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. :)
>
> TAILQ_FOREACH(&td->td_selq, si, si_thrlist)
> si->si_thread = NULL;
> TAILQ_INIT(&td->td_selq);
>
> The idea is that you really don't need to dequeue the selinfos, just
> null out the thread pointer and clear the thread's list of selinfos.
>
>:)
Ah, sure. Looks good then.
>> > @@ -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).
In case we didn't block we don't want to leave TDF_SELECT set? I.e. if during
the original scan one of the fd's said it was ready.
> 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);
>
> ??
This is an old race so I'm not sure we need to worry about it. If we don't
mark the fd as ready now and we are returning, then we will mark it the next
time the user calls select. We just need to not be blocked while there is data
ready or a writer ready.
> Why was there extra hysterics put into the timeout code?
Not sure a) why or b) what exactly you are talking about.
>> > + TAILQ_REMOVE(&td->td_selinfo, sip, si_thrlist);
>> > + mtx_lock_spin(&sched_lock);
>> > + if (td->td_wchan == (caddr_t)&selwait) {
>> > + if (td->td_proc->p_stat == SSLEEP)
>> > + setrunnable(td);
>> > + else
>> > + cv_waitq_remove(td);
>> > + } else
>> > + td->td_flags &= ~TDF_SELECT;
>> > + mtx_unlock_spin(&sched_lock);
>>
>> This can probably be simplified now to something like this:
>>
>> mtx_lock_spin(&sched_lock);
>> if (td->td_proc->p_stat == SSLEEP)
>> setrunnable(td);
>> mtx_unlock_spin(&sched_lock);
>
> Huh, why?
Because we don't have stale selinfo references to threads now that we clear
them all when we exit. The td_wchan compare was to handle the stale
references. I don't think we can now be on the cv waitq and be runnable
now at the same time anymore either.
>> I would rather you commit the select stuff by itself and do the Giant
>> pushdown's afterwards though I might like to review just that part of the
>> diff
>> once the select changes are in.
>
> Ok. I'll post two seperate diffs tonight.
Thanks!
--
John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve!" - http://www.FreeBSD.org/
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?XFMail.20020311174341.jhb>