On 1/23/12 7:57 AM, Alan M. Carroll wrote:
Sunday, January 22, 2012, 10:20:58 PM, you wrote:
if (frequent_accept) { // true
I read that as "checking to see if frequent_accept is true".
Right, and it used to be always true. And you are suggesting to change it to
"false", no ?
What's semi confusing is that the declaration of accept_internal() has the
frequent_accept parameter defaulted to "false", yet, as far as I can tell,
in this older version of the code, we always called it with "true".
Fwiw, I also checked v2.0.1, and it also had frequent_accept (as passed
around) default to true in every case.
MuxVC.cc, InkAPI.cc, and LogCollationAccept.cc call NetProcessor::accept() with
frequent_accept false or defaulted. So it was not true everywhere.
I see, yeah, that makes sense, I guess those auxiliary 'server' features did
not want to create multiple accept threads. I meant "always true" as in for
the main traffic_server process / HTTP server (i.e. there was no way to make
it false before).
Perhaps we should leave the default true, but change the calls to
NetProcessor::accept to set it false?
So, unless I'm totally missing something, just changing the default back to
'false' again will break accept_internal() as it was intended to work, no ?
accept_internal should be unaffected by the default. I think it was intended to
check the value and do something based on it. Otherwise there should not even
be an option to check.
The question is what the callers should do. I can try tweaking TSNetAccept to
turn off frequent_accept, which is original (2.1.6) behavior (although that
seems to make mock of the accept_threads parameter). We're still left with the
question of why my code fails regression with fast accept.
Alright, I think we are on the same page now. For some reason, I thought you
meant to turn off (set to false) the frequent_accept for the main server.
And like I said, I know we had that bug once before (before v2.1.7, when you
initially made the changes to pass the "opt" parameter, with it defaulted to
false). My concern is/was that we'd make the same mistake again.
Seeing your explanation, the frequent_accept parameter does make some sense,
since there are (apparent) use cases where you don't want multiple accept
threads, or accepts per net-thread (for e.g. the log collation server). So I
don't think we should remove it (i.e. making it always on seems like a bad
idea).
It might be that we should clean this up though, a more obvious solution
would be to only have two options:
1) One, or several, dedicated accept threads (e.g. num_accept_threads).
2) Accept on one, or all, of the net-threads
This is basically the same as it works today, but less convoluted. If I
understand the code correct, if frequent_accept is false, then you get
assigned a "random" ET_NET thread to do the non-blocking accept() on. If it
is true, you either create one or several accept threads to do blocking
accept() on, or you do non-blocking accept() on all the net-threads.
Then all uses of this would allow for this, and specially, the InkAPI accept
APIs would allow for this control as well (unless they already do, I didn't
check).
Ciao,
-- Leif
