On Wed, Nov 16, 2011 at 2:23 PM, Jeff Trawick <[email protected]> wrote: > On Wed, Nov 16, 2011 at 11:51 AM, Jeff Trawick <[email protected]> wrote: >> On Wed, Nov 16, 2011 at 11:20 AM, Paul Querna <[email protected]> wrote: >>> On Wed, Nov 16, 2011 at 2:44 AM, Rainer Jung <[email protected]> >>> wrote: >>>> On 15.11.2011 20:57, Jeff Trawick wrote: >>>>> >>>>> On Tue, Nov 15, 2011 at 2:32 PM, William A. Rowe Jr. >>>>> <[email protected]> wrote: >>>>>> >>>>>> On 11/15/2011 12:33 PM, Stefan Fritsch wrote: >>>>>>> >>>>>>> On Tuesday 15 November 2011, Paul Querna wrote: >>>>>>>> >>>>>>>> On Tue, Nov 15, 2011 at 9:17 AM, Stefan Fritsch<[email protected]> >>>>>>> >>>>>>> wrote: >>>>>>>>> >>>>>>>>> On Tue, 15 Nov 2011, [email protected] wrote: >>>>>>>>>> >>>>>>>>>> Author: pquerna >>>>>>>>>> Date: Tue Nov 15 15:49:19 2011 >>>>>>>>>> New Revision: 1202255 >>>>>>>>>> >>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1202255&view=rev >>>>>>>>>> Log: >>>>>>>>>> disable mod_reqtimeout if not configured >>>>>>>>> >>>>>>>>> Why that? We have just changed the default to be enabled in >>>>>>>>> r1199447 and several developers at the hackathon agreed to this >>>>>>>>> change. >>>>>>>> >>>>>>>> Didn't know it was discussed in depth at the hackathon, and there >>>>>>>> wasn't any discussion on the list.... >>>>>>>> >>>>>>>> It showed up quite quickly in my profiling of the Event MPM, >>>>>>>> because every pull/push on the filters would cause a >>>>>>>> apr_time_now() call. >>>>>>>> >>>>>>>> I don't really like that just by loading the module, it changes the >>>>>>>> behavior and performance of the server so drastically. >>>>>>> >>>>>>> It only acts on reads from the client. Normal non-POST requests arrive >>>>>>> in one or two packets, which would mean approx. 3 additional >>>>>>> apr_time_now calls per request. I haven't done benchmarks, but I can't >>>>>>> imagine that this has a drastic impact on performance. And if it costs >>>>>>> 1-2%, then that's a small cost compared to the impact of slowloris >>>>>>> type attacks which eat lots of memory. >>>>>>> >>>>>>> The general intention of the recent changes in default configs and >>>>>>> module selection/loading was to make it easier to only load those >>>>>>> modules that are really needed, have a reasonable default config, and >>>>>>> have the compiled-in default values be the same as those in the >>>>>>> example config files. >>>>>> >>>>>> Which means, build by default, disable by default. I think that keeps >>>>>> everyone happy. When abuse arrives, it's trivial to load. >>>>> >>>>> Timeout 60 isn't nearly as bad as the old Timeout 300 that is probably >>>>> still in wide use, but mod_reqtimeout can provide a much more >>>>> reasonable out of the box configuration. I think we should keep it in >>>>> place by default. >>>> >>>> +1 >>>> >>> >>> What I am really opposed to is that the LoadModule causes such a >>> degradation in performance. >>> >>> I am 100% +1 to adding conf commands to the default configuration in >>> the httpd.conf, but what I do not like is that having just a >>> LoadModule with nothing else causes reqtimeout to do work. It is too >>> trivial for people to have accidental load modules in many distros. >> >> How many distinct concerns are there? (Cut through the detail that >> the implementation of request phase-specific timeouts is in a module, >> and hence conflicts with thoughts for what it means to have code >> loaded by LoadModule but not explicitly configured.) I see at least >> two raised. >> >> 1) performance degradation out of the box with no explicit configuration* >> 2) any processing at all out of the box with no explicit configuration >> 3) relatively complex-to-understand timeout mechanism out of the box >> with no explicit configuration (independent of whether it is core or >> mod_) >> 4) disagreement that the mod_reqtimeout defaults are "generally >> better" across arbitrary situations than Timeout <somenumber> >> 5) ??? >> >> (I assume that the performance issue related to the apr_time_now() >> calls in mod_reqtimeout is addressable, and probably in a way that >> helps other modules that don't need to know the time with high >> precision.) > > See attached patch for one possible fix (completely untested :) ). It > should be determined (even if by changing code) that event's listener > thread can update the timestamp in the child at least every 200ms. > ap_time_around_now(), ap_roughly_time_now(), etc. might be better API > names. Over the long term it might be helpful to have a couple of > versions with different requirements, as some MPMs will simply punt to > apr_time_now() if asked to provide something within 200ms.
oops, I forgot to 0 out roughly_now during special situations (maybe just exiting?); its just a concept patch ;)
