Re: commloops revisited

On Wed, May 24, 2006, Henrik Nordstrom wrote:

> > Here's my first cut. They compile and run; I haven't done much
> > testing on them. There's plenty more tidying up which can be done
> > after this is in - mostly involving tidying up commSetSelect() and
> > making the commDeferFD/commResumeFD calls part of the net API rather
> > than just for epoll.
>
> We also should get the comm pending stuff sorted out.

In what way?

> > I'd like to get this into squid-2.6 now so I or someone else can bring
> > across kqueue/solaris epoll before 2.6 is released.
>
>
> Looked almost fine to me, except that it appears you forgot to rip out
> the poll code from comm_select.c....

Fixed.

> Before commit this needs to be refined it a bit first. Do you have a
> branch created for this in your monotone or devel.squid-cache?

Nope; its just sitting in a checked out tree. I can stuff it in a
branch on devel if needs be.

> - poll code still in comm_select. Either rip it, or drop comm_poll.c.

Done.

> - lots of debug statement cleanups

Hm?

> - need to take a broader view on the comm loops in general. A bit too
> much code duplication which guarantees that some will bitrot more than
> it already has..

I do plan on twiddling this stuff. Hm, maybe I should just create a branch.

> but honestly this is starting to go a bit outside the intended scope of
> 2.6. Isn't bad, but a bit worried how to get all this done and verified
> in the few days left..

This is why I wanted to keep the comm code reasonably untouched and leave
this as a structural change.

> > I also discovered some recent change to the select() code stopped it
> > compiling; I've fixed it with what a cursory glance shows should be
> > right. I'd appreciate someone verifying this.
>
> You mean this:
>
> - if (FD_ISSET(fd, &readfds) &&
> fd_table[fd].flags.read_pending) {
> + if (FD_ISSET(fd, &readfds) && fd_table[fd].read_pending
> & COMM_PENDING_NORMAL) {
>
>
> unfortunately not correct.. see the poll loop. This btw got bitrotted by
> the ssl update.

Ah, hm. Thats going to be a bit interesting.

> As you are not moving the select() code we do this: I fix up the pending
> bits in the select loop, and you think about how these would fit into
> the epoll loop...

Thats going to require some thought; and I can't dedicate that much thought
on it at the moment (hence why these were mostly structural changes.)

What one really wants to avoid doing is scanning each FD each time the comm
select/poll/epoll routine is called; which I believe your code was intended
to do. Instead perhaps you may wish to modify the SSL code to reschedule
for another read/write if it detects something is needed. Maybe a hack is
doable for the epoll() stuff but it'd be just a hack.

Reading this code makes me believe the SSL support code should really be
abstracted out a little further: ie, the SSL support code handles scheduling
IO as well as handling read/write. I don't have the time to do this until
mid-june, which means missing your release date somewhat.

A nice advantage of sorting out the backoff/backon and the above SSL stuff
is that it'd result in much, much nicer poll and select code; one wouldn't
need to walk the FD list a couple of times to build poll/select lists
each time we went through the comm loop.

Adrian
Received on Wed May 24 2006 - 18:45:59 MDT