Re: select fix and giant pushdown patch

* Chad David <[e-mail]> [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 [e-mail]
with "unsubscribe freebsd-smp" in the body of the message