On Linux, FreeBSD and Solaris (at least), even with the most recent nonblocking kernel facilities, it is still faster to accept new TCP connections in a blocking fashion via thread pool. FWIW.
-- Theo Schlossnagle (mobile) http://omniti.com/is/theo-schlossnagle On Jan 23, 2012, at 1:19 PM, John Plevyak <jplev...@acm.org> wrote: > 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 <zw...@apache.org> 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 >> >>
