I think Leif has it right about the use cases and meaning of
frequent_accept. Given how much has changed kernel-wise since that code
was originally written, it isn't clear how much we save by limiting some
accepts to a single thread non-blocking (i.e. epoll() makes the marginal
cost of another fd small, and modern OSes handle many more threads
efficiently than they used to).
The key question is "why is the test failing", and AFAICT it looks like
NetAccept::init_accept() isn't doing:
PollDescriptor *pd = get_PollDescriptor(t);
if (a->ep.start(pd, a, EVENTIO_READ) < 0)
link in init_accept_per_thread(). So it isn't clear to me why it would get
called ever.
It just looks like this code needs to be simplified and cleaned up a bit.
There is a lot of commented out code and convoluted paths and the code is
split between NetAccept and UnixNetProcessor which further complicates
things. There are too many different paths which do largely the same thing
under different circumstances, and yes, we might loose a few cycles by
factoring the code better, but it would be much more maintainable, stable
and (hopefully) bug free.
The way the code is now, we tend to have the main entry points for a
processor in the XXXXProcessor.cc file and then some code for each point in
another XXXFoo file. This tends to split up the code which is confusing.
Perhaps for main entry point Foo we should just have all the code in
XXXFoo, in this case UnixAccept.cc for example.
I'll see if I can break away some time.
john
On Mon, Jan 23, 2012 at 7:50 AM, Leif Hedstrom <[email protected]> wrote:
> 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
>
>
