Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Thu, 28 Aug 2003 02:07:40 -0400 (EDT), Cliff Woolley wrote: >On Thu, 28 Aug 2003 [EMAIL PROTECTED] wrote: > >> jwoolley2003/08/27 22:54:44 >> >> Modified:.CHANGES >>server/mpm/beos beos.c >>server/mpm/experimental/leader leader.c >>server/mpm/experimental/threadpool threadpool.c >>server/mpm/mpmt_os2 mpmt_os2_child.c >>server/mpm/netware mpm_netware.c >>server/mpm/worker worker.c >> Log: >> Updated the various MPM's to use the new bucket_alloc_create_ex API >> when necessary. Which is to say that it's necessary in all cases except >> for prefork, where the change to apr-util to have it use the allocator >> from the pool passed in is already sufficient. > > >I've tested worker, leader, and threadpool to compile. Jean-Jacques tells >me that the netware mpm seems to be working fine. > >I have no idea how to make either perchild or the winnt MPM honor >MaxMemFree in any way at all. The pool-recycling-and-passing-around logic >got a bit too complicated for me to follow easily. If somebody could jump >in and give me a hint on those, I'd appreciate it. > >Please double-check that your MPM's still compile and work, that they >honor the MaxMemFree setting, do not leak memory upon restart, do not >segfault on exit, etc. :) Seems ok using mpmt_os2 though I had to add #define AP_MPM_WANT_SET_MAX_MEM_FREE to mpmt_os2/mpm.h or it bombs with symbol 'ap_max_mem_free' undefined. -- __ | Brian Havard | "He is not the messiah! | | [EMAIL PROTECTED] | He's a very naughty boy!" - Life of Brian | --
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Thu, 28 Aug 2003 [EMAIL PROTECTED] wrote: > Index: threadpool.c > === > RCS file: /home/cvs/httpd-2.0/server/mpm/experimental/threadpool/threadpool.c,v > retrieving revision 1.17 > retrieving revision 1.18 > diff -u -d -u -r1.17 -r1.18 > --- threadpool.c27 Aug 2003 22:33:11 - 1.17 > +++ threadpool.c28 Aug 2003 05:54:44 - 1.18 > @@ -981,12 +981,10 @@ > >apr_allocator_create(&allocator); >apr_allocator_max_free_set(allocator, ap_max_mem_free); > +/* XXX: why is ptrans's parent not tpool? --jcw 08/2003 */ >apr_pool_create_ex(&ptrans, NULL, NULL, allocator); >apr_allocator_owner_set(allocator, ptrans); > - > -/* XXX: What happens if this is allocated from the > - * single-thread-optimized ptrans pool? -aaron */ > -bucket_alloc = apr_bucket_alloc_create(tpool); > +bucket_alloc = apr_bucket_alloc_create_ex(allocator); > Side note: to answer this question of yours, Aaron, if bucket_alloc had been allocated from ptrans, then when ptrans got cleared, the bucket allocator would get destroyed. Note that this actually /is/ what I'm now doing in the worker MPM... I create a new bucket_alloc per transaction and let it get destroyed when ptrans gets cleared. Since apr_bucket_alloc_create() is no longer creating its own allocator but rather using the one from the parent pool, I figure this is okay, since no actual malloc() calls ought to happen. Can anybody answer the question I inserted? Why would it not be apr_pool_create_ex(&ptrans, pthrd, NULL, allocator); rather than apr_pool_create_ex(&ptrans, NULL, NULL, allocator); Doesn't the latter leak memory if you destroy a thread and replace it with another thread? I guess if you never do that it's no big deal. --Cliff
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Thu, 28 Aug 2003 [EMAIL PROTECTED] wrote: > jwoolley2003/08/27 22:54:44 > > Modified:.CHANGES >server/mpm/beos beos.c >server/mpm/experimental/leader leader.c >server/mpm/experimental/threadpool threadpool.c >server/mpm/mpmt_os2 mpmt_os2_child.c >server/mpm/netware mpm_netware.c >server/mpm/worker worker.c > Log: > Updated the various MPM's to use the new bucket_alloc_create_ex API > when necessary. Which is to say that it's necessary in all cases except > for prefork, where the change to apr-util to have it use the allocator > from the pool passed in is already sufficient. I've tested worker, leader, and threadpool to compile. Jean-Jacques tells me that the netware mpm seems to be working fine. I have no idea how to make either perchild or the winnt MPM honor MaxMemFree in any way at all. The pool-recycling-and-passing-around logic got a bit too complicated for me to follow easily. If somebody could jump in and give me a hint on those, I'd appreciate it. Please double-check that your MPM's still compile and work, that they honor the MaxMemFree setting, do not leak memory upon restart, do not segfault on exit, etc. :) Thanks! --Cliff PS: Sorry for sending to so many people directly... just wanted to make sure I caught everyone's attention. :)
APR versioning (was: cvs commit: httpd-2.0/server/mpm/worker worker.c)
On Wed, May 29, 2002 at 08:40:55PM -0400, Cliff Woolley wrote: > On 30 May 2002 [EMAIL PROTECTED] wrote: > > striker 02/05/29 17:21:27 > > > > Modified:server/mpm/beos beos.c > >server/mpm/experimental/leader leader.c > >server/mpm/experimental/threadpool threadpool.c > >server/mpm/netware mpm_netware.c > >server/mpm/prefork prefork.c > >server/mpm/worker worker.c > > Log: > > Catch up with the apr_allocator_set_owner -> apr_allocator_owner_set renames > > in APR. > > This requires an MMN bump (which is fine with me, since we've already done > one in 2.0.37-dev, and I'm starting to see little point in going back > now). If there are other renames of this sort, we should get them in all > at once. > > Is the APR versioning system in place yet? The header and the code is in there, but we also need to integrate it into the build process so that we generate versioned .so files (e.g. libapr.so.1) The mechanism has been added to apr, but not apr-util (yet). But the versioning doc states that we are not (strictly) bound to retain API compatibility as long as we have not reached version 1.0. Currently, APR is labelled as 0.9.0. So APR(UTIL) is somewhere on the line of "free to change the API at will" and "please don't mess up httpd releases". If we made a formal 1.0 release of APR(UTIL), then we could start applying the versioning rules to it quite strictly. A separate decision is whether to carry the versioning scheme into httpd. At this point, I'd say "no". Until we show that it is working well for APR and company, we can/should just let httpd stick to its current versioning model. As a true action item, I might suggest making an official 0.9 release of APR and APRUTIL, to get the release procedures set up and oiled. I'd suggest that we do the release with the caveat of API changes until 1.0 is released (just as a fallback; I doubt we'd really make any, but if we *do*, we don't have to worry about all the rules), then we apply the thumbscrews to its API. Note that I'd also suggest calling it 0.9 so we don't give false impressions of a "real" 1.0 release. We need more field testing before then (specifically with things like: where do the includes go? do we have vsn numbers on the .so files? etc) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Renames pending, WAS: RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
* Sander Striker ([EMAIL PROTECTED]) wrote : > > From: Cliff Woolley [mailto:[EMAIL PROTECTED]] > > Sent: 30 May 2002 02:41 > > [...] > > > Log: > > > Catch up with the apr_allocator_set_owner -> apr_allocator_owner_set renames > > > in APR. > > > > This requires an MMN bump (which is fine with me, since we've already done > > one in 2.0.37-dev, and I'm starting to see little point in going back > > now). If there are other renames of this sort, we should get them in all > > at once. > > What about these: > > apr_pool_set_abort > apr_pool_get_abort > apr_pool_get_parent > > And the ones left in renames_pending? Also, Thom May posted a long list > of suggestions a while back. Noone seems to have responded to that. > Attached. -- Thom May -> [EMAIL PROTECTED] "pants" is like "fuck". It can be good or bad. Index: renames_pending === RCS file: /home/cvspublic/apr/renames_pending,v retrieving revision 1.9 diff -u -u -r1.9 renames_pending --- renames_pending 22 Apr 2002 10:24:40 - 1.9 +++ renames_pending 30 May 2002 09:46:38 - @@ -4,3 +4,51 @@ apr_user_id_get from apr_get_userid apr_user_name_getfrom apr_get_username +-- +apr_time_exp_gmt_get from apr_implode_gmt +apr_time_interval_t from apr_interval_time_t +apr_time_interval_short_tfrom apr_short_interval_time_t +apr_file_info_t from apr_finfo_t +apr_file_statfrom apr_stat +apr_file_lstat from apr_lstat +apr_file_path_root from apr_filepath_root +apr_file_path_merge from apr_file_path_merge +apr_file_path_setfrom apr_filepath_set +apr_file_path_getfrom apr_filepath_get +apr_file_attrs_t from apr_fileattrs_t +apr_file_seek_where_tfrom apr_seek_where_t + +apr_fnmatch_test from apr_is_fnmatch + +apr_lock_scope_e from apr_lockscope_e +apr_lock_type_e from apr_locktype_e +apr_lock_mech_e from apr_lockmech_e +apr_lock_readerwriter_e from apr_readerwriter_e + +apr_socket_shutdown from apr_shutdown +apr_socket_bind from apr_bind +apr_socket_listenfrom apr_listen +apr_socket_acceptfrom apr_accept +apr_socket_connect from apr_connect +apr_sockaddr_name_info_get from apr_getnameinfo +apr_port_addr_parse from apr_parse_addr_port +apr_hostname_get from apr_gethostname +apr_socket_send from apr_send +apr_socket_sendv from apr_sendv +apr_socket_sendtofrom apr_sendto +apr_socket_recv_from from apr_recvfrom +apr_socket_file_send from apr_sendfile +apr_socket_recv from apr_recv +apr_socket_opt_set from apr_setsocketopt +apr_socket_opt_get from apr_getsocketopt +apr_socket_file_create from apr_socket_from_file +apr_service_byname_get from apr_getservbyname +apr_socket_filter_accept from apr_socket_accept_filter +apr_socket_inherit_set from apr_socket_set_inherit +apr_socket_inherit_unset from apr_socket_unset_inherit + +apr_user_id_current from apr_current_userid +apr_user_compare from apr_compare_users +apr_group_id_get from apr_get_groupid +apr_group_comparefrom apr_compare_groups +
Renames pending, WAS: RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
> From: Cliff Woolley [mailto:[EMAIL PROTECTED]] > Sent: 30 May 2002 02:41 [...] > > Log: > > Catch up with the apr_allocator_set_owner -> apr_allocator_owner_set renames > > in APR. > > This requires an MMN bump (which is fine with me, since we've already done > one in 2.0.37-dev, and I'm starting to see little point in going back > now). If there are other renames of this sort, we should get them in all > at once. What about these: apr_pool_set_abort apr_pool_get_abort apr_pool_get_parent And the ones left in renames_pending? Also, Thom May posted a long list of suggestions a while back. Noone seems to have responded to that. Sander
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On 30 May 2002 [EMAIL PROTECTED] wrote: > striker 02/05/29 17:21:27 > > Modified:server/mpm/beos beos.c >server/mpm/experimental/leader leader.c >server/mpm/experimental/threadpool threadpool.c >server/mpm/netware mpm_netware.c >server/mpm/prefork prefork.c >server/mpm/worker worker.c > Log: > Catch up with the apr_allocator_set_owner -> apr_allocator_owner_set renames > in APR. This requires an MMN bump (which is fine with me, since we've already done one in 2.0.37-dev, and I'm starting to see little point in going back now). If there are other renames of this sort, we should get them in all at once. Is the APR versioning system in place yet? --Cliff
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Wed, 1 May 2002, Aaron Bannert wrote: > - Does this work on ... (linux)? Yes, it seems to. Something else is screwy, but as a whole, it kind of works at least, and this piece in particular seems to be doing fine. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
> Because we have to keep the old API working, and because duplicating code > everywhere is a bad thing. How is it duplicated? This is new code. > And while we are on the topic, anything that is posted to the mailing > list is open for others to commit to the code base. That is how we work. > People here are expected to be part-time volunteers, so if one person does > 60% of the work and posts it, others should feel free to do the other 40% > and commit the sucker while the originator is sleeping. The only necessary > part is that it be appropriately attributed in Submitted By. If you go back and read my original post, you'll realize that the reason I posted it was so that I could get some feedback. If I thought it only needed one +1 I wouldn't have posted it. I don't think 2 days (partly over a weekend) is enough time for everyone to review a patch with such ugly side-effects. Furthermore, this patch still has outstanding issues which I think warrant discussion: - Will the log message confuse people? Greg suggests we drop the level down during forcefull restart/shutdown. - Does this work on all platforms (linux)? - Have we beaten this enough to call it release quality? > In this case, there is no excuse for sitting on a bug fix just because > there are stylistic issues about a patch. The appropriate thing to do > is remove the style changes and commit the fix. I don't think anyone was sitting on it for stylistic issues, where did you get that from? Besides, the commit message is inaccurate and partly incorrect. -aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
> > safe_free()-like stuff does, but at least it is a compromise. OTOH, > > as Cliff brought up, we'll get a SEGV if apr_palloc() returns NULL. > > Whoa, slow down. I said that was an argument someone had once posed, but > I also said I thought it was ridiculous. We just don't need to worry > about palloc returning NULL. None of the rest of Apache does. Fair enough. [I didn't mean to imply that you thought it was a concern, just that you remembered someone bringing it up.] -aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Wed, 1 May 2002, Aaron Bannert wrote: > safe_free()-like stuff does, but at least it is a compromise. OTOH, > as Cliff brought up, we'll get a SEGV if apr_palloc() returns NULL. Whoa, slow down. I said that was an argument someone had once posed, but I also said I thought it was ridiculous. We just don't need to worry about palloc returning NULL. None of the rest of Apache does. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Wednesday, May 1, 2002, at 01:49 PM, Aaron Bannert wrote: >> And, consider my position on your calloc change in this patch as a >> veto. If you want to remove calloc, then you should do so throughout >> the code rather than in sporadic places that may make maintaining the >> code a nightmare if we were to fix calloc. But, that is an issue >> that is now open in APR's STATUS. > > What exactly are you vetoing? My use of apr_palloc()+memset() is > technically superior to apr_pcalloc(). What is your technical reason > for forcing me to use apr_pcalloc()? Umm, no it isn't. The reason is that it makes the code harder to understand. >> If the end result of the calloc vote is that we should remove calloc, >> then feel free to do a search and replace across the entire tree. >> Until then, let's please remain consistent to our API. -- justin > > I'm really not sure about the macro yet. On the one hand it's too > late to remove apr_pcalloc(). The macro stinks the same way #define > safe_free()-like stuff does, but at least it is a compromise. OTOH, > as Cliff brought up, we'll get a SEGV if apr_palloc() returns NULL. I > don't see how: > > foo = apr_palloc(p, sizeof(*foo)); > memset(foo, 0, sizeof(*foo)); > > is any less desirable than: > > foo = apr_pcalloc(p, sizeof(*foo)); > > It is quite obvious how the memory is being initialized in both cases, > and for the reasons discussed earlier it is obvious why the first would > be optimal. Because we have to keep the old API working, and because duplicating code everywhere is a bad thing. The arguments have already been made. I don't even understand why people are voting on the macro -- just commit it. Let's save the arguments for things where actual disagreement is useful. And while we are on the topic, anything that is posted to the mailing list is open for others to commit to the code base. That is how we work. People here are expected to be part-time volunteers, so if one person does 60% of the work and posts it, others should feel free to do the other 40% and commit the sucker while the originator is sleeping. The only necessary part is that it be appropriately attributed in Submitted By. In this case, there is no excuse for sitting on a bug fix just because there are stylistic issues about a patch. The appropriate thing to do is remove the style changes and commit the fix. Roy
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
> And, consider my position on your calloc change in this patch as a > veto. If you want to remove calloc, then you should do so throughout > the code rather than in sporadic places that may make maintaining the > code a nightmare if we were to fix calloc. But, that is an issue > that is now open in APR's STATUS. What exactly are you vetoing? My use of apr_palloc()+memset() is technically superior to apr_pcalloc(). What is your technical reason for forcing me to use apr_pcalloc()? > If the end result of the calloc vote is that we should remove calloc, > then feel free to do a search and replace across the entire tree. > Until then, let's please remain consistent to our API. -- justin I'm really not sure about the macro yet. On the one hand it's too late to remove apr_pcalloc(). The macro stinks the same way #define safe_free()-like stuff does, but at least it is a compromise. OTOH, as Cliff brought up, we'll get a SEGV if apr_palloc() returns NULL. I don't see how: foo = apr_palloc(p, sizeof(*foo)); memset(foo, 0, sizeof(*foo)); is any less desirable than: foo = apr_pcalloc(p, sizeof(*foo)); It is quite obvious how the memory is being initialized in both cases, and for the reasons discussed earlier it is obvious why the first would be optimal. -aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Wed, May 01, 2002 at 12:22:13PM -0700, Aaron Bannert wrote: > I did not post this so that you could commit it before I thought it had > been properly reviewed. On top of that you modified my original patch > in a way that you knew I would disagree with. Please do not do that again. Sorry. My sincere apologies. My thought was that you were intending to commit it once it had proper review (we had three +1s for it). What we had before segfaulted, so I felt that it was best to stop segfaulting. Again, I was wrong - sorry... And, consider my position on your calloc change in this patch as a veto. If you want to remove calloc, then you should do so throughout the code rather than in sporadic places that may make maintaining the code a nightmare if we were to fix calloc. But, that is an issue that is now open in APR's STATUS. If the end result of the calloc vote is that we should remove calloc, then feel free to do a search and replace across the entire tree. Until then, let's please remain consistent to our API. -- justin
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Wed, May 01, 2002 at 07:15:40AM -, Justin Erenkrantz wrote: > jerenkrantz02/05/01 00:15:40 > > Modified:.CHANGES >server/mpm/worker worker.c > Log: > Close sockets on worker MPM when doing a graceless restart. This should > resolve some segfaults see when doing such restarts. > > (Justin tweaked the palloc/memset in favor of calloc.) > > Submitted by: Aaron Bannert I did not post this so that you could commit it before I thought it had been properly reviewed. On top of that you modified my original patch in a way that you knew I would disagree with. Please do not do that again. -aaron
Re: Roll of 2.0.36, WAS: RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
Sander Striker wrote: > I was going to roll 2.0.36, but I want to wait for this last > worker change. Unfortunately I don't have the time to pursue > the issue now, so if someone does, please feel free to take > care of this annoying beast. BTW: Is there any problem with the CVS version of mod_cache? Accroding to the latest "nightly build log" of Chuck: > Making in httpd-2.0-nightly > In file included from mod_cache.c:61: > mod_cache.h:213: syntax error before `apr_atomic_t' > In file included from cache_storage.c:61: > mod_cache.h:213: syntax error before `apr_atomic_t' > In file included from cache_util.c:61: > mod_cache.h:213: syntax error before `apr_atomic_t' -- Eli Marmor [EMAIL PROTECTED] CTO, Founder Netmask (El-Mar) Internet Technologies Ltd. __ Tel.: +972-9-766-1020 8 Yad-Harutzim St. Fax.: +972-9-766-1314 P.O.B. 7004 Mobile: +972-50-23-7338 Kfar-Saba 44641, Israel
Re: Roll of 2.0.36, WAS: RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
"Sander Striker" <[EMAIL PROTECTED]> writes: > Hi, > > I was going to roll 2.0.36, but I want to wait for this last > worker change. Unfortunately I don't have the time to pursue > the issue now, so if someone does, please feel free to take > care of this annoying beast. I'll start working on it now. It shouldn't be hard to add a join for graceless termination. -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
Roll of 2.0.36, WAS: RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
Hi, I was going to roll 2.0.36, but I want to wait for this last worker change. Unfortunately I don't have the time to pursue the issue now, so if someone does, please feel free to take care of this annoying beast. Sander > From: Sander Striker [mailto:[EMAIL PROTECTED]] > Sent: 01 May 2002 15:11 >> From: [EMAIL PROTECTED] >> [mailto:[EMAIL PROTECTED]]On Behalf Of Jeff Trawick >> Sent: 01 May 2002 14:53 [...] >> (My apologies for not keeping up with the discussions over the past >> days.) >> >> I don't see (yet) which segfault this would fix. Maybe I was hoping >> for a fix to the wrong problem... >> >> If this was to be combined with a change to do a thread-join on the >> workers even for graceless termination, then I can see how the >> segfault caused by cleaning up a pool that the worker threads depend >> on would be avoided. (But of course a thread-join on a worker could >> hang for a while, depending on what a 3rd-party module is doing.) >> >> This change alone doesn't do much for the race between the main thread >> cleaning up pchild and the worker threads getting dispatched and >> realizing that they can exit and finally finishing their use of data >> in subpools of pchild and doing the pthread_exit(). > > Grmpf. Ofcourse, you are correct. We need the thread_join as a patch > job. After 2.0.36 I am afraid we have to implement apr_thread_cancel... > > Sander >
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
> From: [EMAIL PROTECTED] > [mailto:[EMAIL PROTECTED]]On Behalf Of Jeff Trawick > Sent: 01 May 2002 14:53 > [EMAIL PROTECTED] writes: > > > jerenkrantz02/05/01 00:15:40 > > > > Modified:.CHANGES > >server/mpm/worker worker.c > > Log: > > Close sockets on worker MPM when doing a graceless restart. This should > > resolve some segfaults see when doing such restarts. > > (My apologies for not keeping up with the discussions over the past > days.) > > I don't see (yet) which segfault this would fix. Maybe I was hoping > for a fix to the wrong problem... > > If this was to be combined with a change to do a thread-join on the > workers even for graceless termination, then I can see how the > segfault caused by cleaning up a pool that the worker threads depend > on would be avoided. (But of course a thread-join on a worker could > hang for a while, depending on what a 3rd-party module is doing.) > > This change alone doesn't do much for the race between the main thread > cleaning up pchild and the worker threads getting dispatched and > realizing that they can exit and finally finishing their use of data > in subpools of pchild and doing the pthread_exit(). Grmpf. Ofcourse, you are correct. We need the thread_join as a patch job. After 2.0.36 I am afraid we have to implement apr_thread_cancel... Sander
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
[EMAIL PROTECTED] writes: > jerenkrantz02/05/01 00:15:40 > > Modified:.CHANGES >server/mpm/worker worker.c > Log: > Close sockets on worker MPM when doing a graceless restart. This should > resolve some segfaults see when doing such restarts. (My apologies for not keeping up with the discussions over the past days.) I don't see (yet) which segfault this would fix. Maybe I was hoping for a fix to the wrong problem... If this was to be combined with a change to do a thread-join on the workers even for graceless termination, then I can see how the segfault caused by cleaning up a pool that the worker threads depend on would be avoided. (But of course a thread-join on a worker could hang for a while, depending on what a 3rd-party module is doing.) This change alone doesn't do much for the race between the main thread cleaning up pchild and the worker threads getting dispatched and realizing that they can exit and finally finishing their use of data in subpools of pchild and doing the pthread_exit(). -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.cfdqueue.h
Bill Stoddard wrote:
>
>>On Sat, Apr 27, 2002 at 07:30:51PM -0700, Justin Erenkrantz wrote:
>>
+qi = apr_palloc(pool, sizeof(*qi));
+memset(qi, 0, sizeof(*qi));
>>>As we said, if you are concerned about the performance aspect
>>>of apr_pcalloc, then we should fix apr_pcalloc NOT attempt to
>>>work around its inefficiencies by pointedly not using it.
>>>
>>>If/when Cliff (or someone else?) commits the change to apr_pcalloc,
>>>this chunk would be magically changed along with everything else if
>>>you simply called apr_pcalloc in the first place.
>>>
>>We don't have a consensus on this, and I'm ambivalent about making the
>>p_calloc macro. If we do come up with a consensus than it can change.
>>Until then this is more correct than using p_calloc.
>>
>
>I have no problem with using apr_palloc()/memset() in place of apr_pcalloc().
>
I'm +1 on turning pcalloc into a macro,
-0 for using pcalloc in ap_queue_info_init(),
-0.5 (non-veto) for using palloc/memset in ap_queue_info_init()
Rationale:
* Changing pcalloc to a macro will give us a free optimization
throughout the entire code base.
* In ap_queue_info_init(), we create a struct with 7 fields and
immediately set 4 of them to nonzero values. Using pcalloc, or
any other form of block zero-fill, isn't a big win.
* The whole point of the palloc/memset idiom is that
it's a more awkward syntax than pcalloc, but justified
for the sake of performance optimization. But there's
no point in optimizing ap_queue_info_init(). The function
gets called once per child proc, at startup, and no matter
what technique you use to zero-fill the newly created struct,
it's going to account for far too little of the total run
time to matter at all. There's plenty of code within
fdqueue.c that could benefit from performance optimization,
but none of it is in this function.
+rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
+if (rv != APR_SUCCESS) {
+return rv;
+}
+return APR_SUCCESS;
+}
>>>As I said before, simply "return rv;" works here.
>>>
>>Yeah, but this is much more readable.
>>
>
>I disagree. I would rather see "return rv".
>
I'd rather see "return rv" or "return apr_thread_mutex_unlock".
Both are more readable (and faster) than the current code with
the if-statement.
--Brian
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.cfdqueue.h
On Sun, 28 Apr 2002, Bill Stoddard wrote:
> I have no problem with using apr_palloc()/memset() in place of apr_pcalloc().
Do you have a problem with making apr_pcalloc() a macro?
> > > > +rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> > > > +if (rv != APR_SUCCESS) {
> > > > +return rv;
> > > > +}
> > > > +return APR_SUCCESS;
> > > > +}
> > >
> > > As I said before, simply "return rv;" works here.
> > Yeah, but this is much more readable.
> I disagree. I would rather see "return rv".
FWIW, I also prefer return rv. Or even return apr_thread_mutex_unlock();.
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.c fdqueue.h
> On Sat, Apr 27, 2002 at 07:30:51PM -0700, Justin Erenkrantz wrote:
> > > +qi = apr_palloc(pool, sizeof(*qi));
> > > +memset(qi, 0, sizeof(*qi));
> >
> > As we said, if you are concerned about the performance aspect
> > of apr_pcalloc, then we should fix apr_pcalloc NOT attempt to
> > work around its inefficiencies by pointedly not using it.
> >
> > If/when Cliff (or someone else?) commits the change to apr_pcalloc,
> > this chunk would be magically changed along with everything else if
> > you simply called apr_pcalloc in the first place.
>
> We don't have a consensus on this, and I'm ambivalent about making the
> p_calloc macro. If we do come up with a consensus than it can change.
> Until then this is more correct than using p_calloc.
I have no problem with using apr_palloc()/memset() in place of apr_pcalloc().
>
> > > +rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> > > +if (rv != APR_SUCCESS) {
> > > +return rv;
> > > +}
> > > +return APR_SUCCESS;
> > > +}
> >
> > As I said before, simply "return rv;" works here.
>
> Yeah, but this is much more readable.
I disagree. I would rather see "return rv".
My .02.
Bill
Re: More worker ideas Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.c fdqueue.h
On Sat, Apr 27, 2002 at 07:39:24PM -0700, Brian Pane wrote: > I was going to complain about the addition of yet another mutex > to the critical path of request processing, but it got me thinking > about an approach to making worker faster: shorten the time spent > in the mutex-protected region. Yes! The other neat thing about this is we get to balance the critical path between two mutex regions, rather than one_big_mutex. > By shortening the code path between the lock and the unlock, we > can reduce the probability that two or more threads will be > contending for the mutex at once. And uncontended mutexes have > a lot less overhead. > > I'm going to experiment with streamlining the code within the > mutex-protected blocks of the fdqueue functions to see how much > it helps the multiprocessor performance. At first glance, the > candidates for optimization are: > * In the case where we can't recycle a pool in ap_queue_pop(), > move the destruction of the pool *outside* the mutex-protected > block. > * Maybe change the queue size to a power of two so that we can > replace the modulo operations with a bit mask. ++1 on both accounts. I was also thinking that we could make the queue elements simple pointers, rather than 2 elements in a structure. That way the element lookups can happen outside of the mutex region, and inside we can have a simple struct* assignment. -aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.c fdqueue.h
On Sat, Apr 27, 2002 at 07:30:51PM -0700, Justin Erenkrantz wrote:
> > +qi = apr_palloc(pool, sizeof(*qi));
> > +memset(qi, 0, sizeof(*qi));
>
> As we said, if you are concerned about the performance aspect
> of apr_pcalloc, then we should fix apr_pcalloc NOT attempt to
> work around its inefficiencies by pointedly not using it.
>
> If/when Cliff (or someone else?) commits the change to apr_pcalloc,
> this chunk would be magically changed along with everything else if
> you simply called apr_pcalloc in the first place.
We don't have a consensus on this, and I'm ambivalent about making the
p_calloc macro. If we do come up with a consensus than it can change.
Until then this is more correct than using p_calloc.
> > +rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> > +if (rv != APR_SUCCESS) {
> > +return rv;
> > +}
> > +return APR_SUCCESS;
> > +}
>
> As I said before, simply "return rv;" works here.
Yeah, but this is much more readable.
> > +if (rv != APR_SUCCESS) {
> > +rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> > +if (rv != APR_SUCCESS) {
> > +return rv;
> > +}
> > +return rv;
> > +}
>
> Ditto.
This is legit, I'll fix it.
-aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.c fdqueue.h
Comments inline.
On Sun, Apr 28, 2002 at 01:45:00AM -, [EMAIL PROTECTED] wrote:
> 1.16 +108 -0httpd-2.0/server/mpm/worker/fdqueue.c
>
> Index: fdqueue.c
> ===
> RCS file: /home/cvs/httpd-2.0/server/mpm/worker/fdqueue.c,v
> retrieving revision 1.15
> retrieving revision 1.16
> diff -u -r1.15 -r1.16
> --- fdqueue.c 26 Apr 2002 17:13:51 - 1.15
> +++ fdqueue.c 28 Apr 2002 01:45:00 - 1.16
> @@ -58,6 +58,114 @@
>
>#include "fdqueue.h"
>
> +struct fd_queue_info_t {
> +int idlers;
> +apr_thread_mutex_t *idlers_mutex;
> +apr_thread_cond_t *wait_for_idler;
> +int terminated;
> +};
> +
> +static apr_status_t queue_info_cleanup(void *data_)
> +{
> +fd_queue_info_t *qi = data_;
> +apr_thread_cond_destroy(qi->wait_for_idler);
> +apr_thread_mutex_destroy(qi->idlers_mutex);
> +return APR_SUCCESS;
> +}
> +
> +apr_status_t ap_queue_info_create(fd_queue_info_t **queue_info,
> + apr_pool_t *pool)
> +{
> +apr_status_t rv;
> +fd_queue_info_t *qi;
> +
> +qi = apr_palloc(pool, sizeof(*qi));
> +memset(qi, 0, sizeof(*qi));
As we said, if you are concerned about the performance aspect
of apr_pcalloc, then we should fix apr_pcalloc NOT attempt to
work around its inefficiencies by pointedly not using it.
If/when Cliff (or someone else?) commits the change to apr_pcalloc,
this chunk would be magically changed along with everything else if
you simply called apr_pcalloc in the first place.
> +
> +rv = apr_thread_mutex_create(&qi->idlers_mutex, APR_THREAD_MUTEX_DEFAULT,
> + pool);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +rv = apr_thread_cond_create(&qi->wait_for_idler, pool);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +apr_pool_cleanup_register(pool, qi, queue_info_cleanup,
> + apr_pool_cleanup_null);
> +
> +*queue_info = qi;
> +
> +return APR_SUCCESS;
> +}
> +
> +apr_status_t ap_queue_info_set_idle(fd_queue_info_t *queue_info)
> +{
> +apr_status_t rv;
> +rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +AP_DEBUG_ASSERT(queue_info->idlers >= 0);
> +if (queue_info->idlers++ == 0) {
> +/* Only signal if we had no idlers before. */
> +apr_thread_cond_signal(queue_info->wait_for_idler);
> +}
> +rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +return APR_SUCCESS;
> +}
As I said before, simply "return rv;" works here.
> +apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t *queue_info)
> +{
> +apr_status_t rv;
> +rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +AP_DEBUG_ASSERT(queue_info->idlers >= 0);
> +while ((queue_info->idlers == 0) && (!queue_info->terminated)) {
> +rv = apr_thread_cond_wait(queue_info->wait_for_idler,
> + queue_info->idlers_mutex);
> +if (rv != APR_SUCCESS) {
> +rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +return rv;
> +}
Ditto.
> +}
> +queue_info->idlers--; /* Oh, and idler? Let's take 'em! */
> +rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +else if (queue_info->terminated) {
> +return APR_EOF;
> +}
> +else {
> +return APR_SUCCESS;
> +}
> +}
> +
> +apr_status_t ap_queue_info_term(fd_queue_info_t *queue_info)
> +{
> +apr_status_t rv;
> +rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +queue_info->terminated = 1;
> +apr_thread_cond_broadcast(queue_info->wait_for_idler);
> +rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +return APR_SUCCESS;
> +}
Ditto. -- justin
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
My point was you can't simply copy integers across processes, they won't map to the same vnode. You must use a mechanism for passing them between these processes, and this comes at a price. If you're looking for a cross-platform shared-memory implementation, APR's got it: apr_shm.h If you want to do memory management in that segment, feed it to apr_rmm.h -aaron On Thu, Apr 18, 2002 at 08:47:28AM -0500, Rose, Billy wrote: > Hummm. What are your thoughts on shmget() allocating a global segment owned > by the master process that each child can get at? Is there a Nix out there > that doesn't have shmget()?
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
Okay thanks :) I've printed out most of the 2.0.35 source and am annotating and it to go into 3 ring binders for quick reference. The 2.x code base is such a huge milestone over 1.3, which was already super awesome itself. Billy Rose [EMAIL PROTECTED] > -Original Message- > From: Sander Striker [mailto:[EMAIL PROTECTED]] > Sent: Thursday, April 18, 2002 9:00 AM > To: [EMAIL PROTECTED] > Cc: Billy Rose > Subject: RE: cvs commit: httpd-2.0/server/mpm/worker worker.c > > > > From: Rose, Billy [mailto:[EMAIL PROTECTED]] > > Sent: 18 April 2002 15:47 > > > Hummm. What are your thoughts on shmget() allocating a > global segment owned > > by the master process that each child can get at? Is there > a Nix out there > > that doesn't have shmget()? > > I suggest you take a look at APR, specifically > apr/include/apr_shm.h > > Sander > > > > Billy Rose > > [EMAIL PROTECTED] > > > > > -Original Message- > > > From: Aaron Bannert [mailto:[EMAIL PROTECTED]] > > > Sent: Wednesday, April 17, 2002 3:19 PM > > > To: [EMAIL PROTECTED] > > > Subject: Re: cvs commit: httpd-2.0/server/mpm/worker worker.c > > > > > > > > > On Wed, Apr 17, 2002 at 02:58:03PM -0500, Rose, Billy wrote: > > > > If I could receive feedback on the following email made on > > > the 11th, I'd be > > > > willing to burn some hours to make the following MPM > for testing: > > > > > > I think one part that is missing from this design is how > you translate > > > these "connection objects" between processes. This is not > unsolvable, > > > just an obstacle that may eat into performance. Some of the > > > methods that > > > you might use for this are: > > > > > > - unix domain sockets (named and anonymous both work here) > > > - doors (* my prefered method, only available on solaris) > > > > > > > > > I've also been working on an addition to APR that will do this in > > > a cross-platform manner, called "spipe" (aka Stream > Pipes). See some > > > sample file-descriptor-passing code in the perchild MPM. > > > > > > -aaron >
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
> From: Rose, Billy [mailto:[EMAIL PROTECTED]] > Sent: 18 April 2002 15:47 > Hummm. What are your thoughts on shmget() allocating a global segment owned > by the master process that each child can get at? Is there a Nix out there > that doesn't have shmget()? I suggest you take a look at APR, specifically apr/include/apr_shm.h Sander > Billy Rose > [EMAIL PROTECTED] > > > -Original Message- > > From: Aaron Bannert [mailto:[EMAIL PROTECTED]] > > Sent: Wednesday, April 17, 2002 3:19 PM > > To: [EMAIL PROTECTED] > > Subject: Re: cvs commit: httpd-2.0/server/mpm/worker worker.c > > > > > > On Wed, Apr 17, 2002 at 02:58:03PM -0500, Rose, Billy wrote: > > > If I could receive feedback on the following email made on > > the 11th, I'd be > > > willing to burn some hours to make the following MPM for testing: > > > > I think one part that is missing from this design is how you translate > > these "connection objects" between processes. This is not unsolvable, > > just an obstacle that may eat into performance. Some of the > > methods that > > you might use for this are: > > > > - unix domain sockets (named and anonymous both work here) > > - doors (* my prefered method, only available on solaris) > > > > > > I've also been working on an addition to APR that will do this in > > a cross-platform manner, called "spipe" (aka Stream Pipes). See some > > sample file-descriptor-passing code in the perchild MPM. > > > > -aaron
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
Hummm. What are your thoughts on shmget() allocating a global segment owned by the master process that each child can get at? Is there a Nix out there that doesn't have shmget()? Billy Rose [EMAIL PROTECTED] > -Original Message- > From: Aaron Bannert [mailto:[EMAIL PROTECTED]] > Sent: Wednesday, April 17, 2002 3:19 PM > To: [EMAIL PROTECTED] > Subject: Re: cvs commit: httpd-2.0/server/mpm/worker worker.c > > > On Wed, Apr 17, 2002 at 02:58:03PM -0500, Rose, Billy wrote: > > If I could receive feedback on the following email made on > the 11th, I'd be > > willing to burn some hours to make the following MPM for testing: > > I think one part that is missing from this design is how you translate > these "connection objects" between processes. This is not unsolvable, > just an obstacle that may eat into performance. Some of the > methods that > you might use for this are: > > - unix domain sockets (named and anonymous both work here) > - doors (* my prefered method, only available on solaris) > > > I've also been working on an addition to APR that will do this in > a cross-platform manner, called "spipe" (aka Stream Pipes). See some > sample file-descriptor-passing code in the perchild MPM. > > -aaron >
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
> -Original Message- > From: Brian Pane [mailto:[EMAIL PROTECTED]] > Sent: Wednesday, April 17, 2002 7:48 PM > To: [EMAIL PROTECTED] > Subject: Re: cvs commit: httpd-2.0/server/mpm/worker worker.c > > > Rose, Billy wrote: > > >If I could receive feedback on the following email made on > the 11th, I'd be > >willing to burn some hours to make the following MPM for testing: > > > >>I hope my emails are not annoying you guys. To give a more > complete picture > >>of this (pulled from methods I used in a client server app): > >> > >>The initial process creates a shared memory area for the > queue and then a > >>new thread, or child process, whose sole purpose is to > dispatch connections > >>to workers. The shared memory area is a FIFO for queuing up > connections. > >>The dispatcher process then goes on to set up the other > children each of which > >>has access to the shared memory so they may get at the > connection FIFO. The > >>FIFO is a linked list containing connection objects that > get filled in by > >>the listener thread/process. The dispatcher maintains a > list of all children > >>workers that it created, and sits in a loop sleeping with > some set timeout. > >>When the listener accepts a connection and puts it in the > queue, it wakes > >>the dispatcher if need be. Once awakened, the dispatcher > looks to see if > >>connections are queued, and if so, it looks in its worker > list for the next > >>available worker and awakens it. > >> > > I can think of a few factors that will complicate the implementation > of this design: > > * The role of the dispatcher in this design adds some extra context > switching to the critical path for accepting a connection. By my > count, three or four separate threads--distributed among two or > three separate processes--will have to get involved to accept each > new connection. Based on the relative performance of the other > MPM designs, I'd say that MPMs that involve fewer threads per > connection generally are fastest (that's part of why prefork > has outperformed the original worker design.) > The extra overhead of communications between contexts will be offset by the shear volume of connections handled. I would not use this prospected MPM on a light duty web server, but rather one that gets thousands of requests per minute. The machinery employed here would dictate that until some number N of connections, the context switching will cause added overhead. Beyond N, and the response mechanisms will begin to flatten out at the dictated rate of the hardware as very little switching occurs until a thread is finished with a request. I suppose this MPM is much like a tricked out V8 engine. It won't perform until enough RPM's have been gained. One extra context switch between accepting a connection and servicing it is a low price to pay for an MPM that could handle instant spikes into the thousands of requests. Not all people would benefit from this, but I'm sure some out there would. Here's an idea: how about a hybrid MPM that starts out in prefork, and then switches to something like this when N is reached. > * When the dispatcher looks for an available worker, is it looking for > a worker process, or for a worker thread within one of the worker > processes? > Thread. The dispatcher is responsible for managing the creation of all processes, and all threads therein. I keeps track of which thread in what process is handling some connection via the "task list" (for lack of a better term). Each process would have an entry point into a thread creation function that the dispatcher would call up via IPC. > If the dispatcher wakes up a worker process, then that process will > need to figure out which of its threads will handle the connection. > To do this, it will need to either: 1) let its idle threads > take turns > handling connections (meaning that it looks a lot like > leader/follower) > or 2) have one dedicated thread that delegates the connection to an > idle worker (meaning that it looks a lot like the threadpool MPM or > the original worker MPM). > > Alternately, if the dispatcher wakes up a worker thread directly, it > needs a means to signal a specific thread within another process. > It may be very difficult to do that portably. I suppose each idle > worker thread could do a blocking read on a separate socket or pipe, > and the dispatcher could write a byte to the one that it wants to > wake up. But that would slow down connection processing and use up > lots of file descriptors. > shmget(), or perhaps msg
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Rose, Billy wrote: >If I could receive feedback on the following email made on the 11th, I'd be >willing to burn some hours to make the following MPM for testing: > >>I hope my emails are not annoying you guys. To give a more complete picture >>of this (pulled from methods I used in a client server app): >> >>The initial process creates a shared memory area for the queue and then a >>new thread, or child process, whose sole purpose is to dispatch connections >>to workers. The shared memory area is a FIFO for queuing up connections. >>The dispatcher process then goes on to set up the other children each of which >>has access to the shared memory so they may get at the connection FIFO. The >>FIFO is a linked list containing connection objects that get filled in by >>the listener thread/process. The dispatcher maintains a list of all children >>workers that it created, and sits in a loop sleeping with some set timeout. >>When the listener accepts a connection and puts it in the queue, it wakes >>the dispatcher if need be. Once awakened, the dispatcher looks to see if >>connections are queued, and if so, it looks in its worker list for the next >>available worker and awakens it. >> I can think of a few factors that will complicate the implementation of this design: * The role of the dispatcher in this design adds some extra context switching to the critical path for accepting a connection. By my count, three or four separate threads--distributed among two or three separate processes--will have to get involved to accept each new connection. Based on the relative performance of the other MPM designs, I'd say that MPMs that involve fewer threads per connection generally are fastest (that's part of why prefork has outperformed the original worker design.) * When the dispatcher looks for an available worker, is it looking for a worker process, or for a worker thread within one of the worker processes? If the dispatcher wakes up a worker process, then that process will need to figure out which of its threads will handle the connection. To do this, it will need to either: 1) let its idle threads take turns handling connections (meaning that it looks a lot like leader/follower) or 2) have one dedicated thread that delegates the connection to an idle worker (meaning that it looks a lot like the threadpool MPM or the original worker MPM). Alternately, if the dispatcher wakes up a worker thread directly, it needs a means to signal a specific thread within another process. It may be very difficult to do that portably. I suppose each idle worker thread could do a blocking read on a separate socket or pipe, and the dispatcher could write a byte to the one that it wants to wake up. But that would slow down connection processing and use up lots of file descriptors. >> The worker then locks the FIFO head pointer >>and grabs the connection, moves the pointer to the next FIFO node, and then >>unlocks the FIFO head pointer. If no workers are available, the dispatcher >>creates more workers if not at a set maximum in order to take the >>connection. If at the maximum, it sleeps again with a shorter timeout, which >>when woke up again, checks for an available worker again. This repeats >>until a worker is available. The listener in all of this is totally independent of >the workers, and only knows that it accepts connections and puts them in a >>FIFO, and finally notifies the dispatcher of the connection. The connection >>queue could span thousands of queued connections if desired. The dispatcher >>is responsible for coordinating the workers, and the queue resides in one >>place only, that being the shared memory segment. >> Having a single queue definitely is the right solution. The catch is that managing that single queue within the httpd is tricky (based on the issues noted above). The alternate way to get a single-queue solution is to let the OS manage the queue for us, which is what the prefork, leader/follower, and threadpool MPMs do. >>Is this too drastic an alteration to the current worker mpm, i.e. would it >>be a separate mpm if it came to fruition? >> Separate MPM --Brian
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Wed, Apr 17, 2002 at 02:58:03PM -0500, Rose, Billy wrote: > If I could receive feedback on the following email made on the 11th, I'd be > willing to burn some hours to make the following MPM for testing: I think one part that is missing from this design is how you translate these "connection objects" between processes. This is not unsolvable, just an obstacle that may eat into performance. Some of the methods that you might use for this are: - unix domain sockets (named and anonymous both work here) - doors (* my prefered method, only available on solaris) I've also been working on an addition to APR that will do this in a cross-platform manner, called "spipe" (aka Stream Pipes). See some sample file-descriptor-passing code in the perchild MPM. -aaron
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
If I could receive feedback on the following email made on the 11th, I'd be willing to burn some hours to make the following MPM for testing: >I hope my emails are not annoying you guys. To give a more complete picture >of this (pulled from methods I used in a client server app): > >The initial process creates a shared memory area for the queue and then a >new thread, or child process, whose sole purpose is to dispatch connections >to workers. The shared memory area is a FIFO for queuing up connections. The >dispatcher process then goes on to set up the other children each of which >has access to the shared memory so they may get at the connection FIFO. The >FIFO is a linked list containing connection objects that get filled in by >the listener thread/process. The dispatcher maintains a list of all children >workers that it created, and sits in a loop sleeping with some set timeout. >When the listener accepts a connection and puts it in the queue, it wakes >the dispatcher if need be. Once awakened, the dispatcher looks to see if >connections are queued, and if so, it looks in its worker list for the next >available worker and awakens it. The worker then locks the FIFO head pointer >and grabs the connection, moves the pointer to the next FIFO node, and then >unlocks the FIFO head pointer. If no workers are available, the dispatcher >creates more workers if not at a set maximum in order to take the >connection. If at the maximum, it sleeps again with a shorter timeout, which >when woke up again, checks for an available worker again. This repeats until >a worker is available. The listener in all of this is totally independent of >the workers, and only knows that it accepts connections and puts them in a >FIFO, and finally notifies the dispatcher of the connection. The connection >queue could span thousands of queued connections if desired. The dispatcher >is responsible for coordinating the workers, and the queue resides in one >place only, that being the shared memory segment. > >Is this too drastic an alteration to the current worker mpm, i.e. would it >be a separate mpm if it came to fruition? > >Billy Rose >[EMAIL PROTECTED] > > -Original Message- > From: Brian Pane [mailto:[EMAIL PROTECTED]] > Sent: Thursday, April 11, 2002 2:49 PM > To: [EMAIL PROTECTED] > Subject: Re: [PATCH] convert worker MPM to leader/followers design > > > Rose, Billy wrote: > > >Would the solution in my last email do what you are looking for? > > > > My one concern with your solution is that it puts a queue in the > httpd child processes. I think that putting a queue in each child > is always going to be tricky because you can get things stuck in the > queue of one busy child process while other child processes are idle. > > What I like about designs like leader/follower and prefork is that > they share one queue across all child processes (and the queue is > maintained by the TCP stack). > > --Brian > > Billy Rose [EMAIL PROTECTED] > -----Original Message- > From: Aaron Bannert [mailto:[EMAIL PROTECTED]] > Sent: Wednesday, April 17, 2002 12:35 PM > To: [EMAIL PROTECTED] > Subject: Re: cvs commit: httpd-2.0/server/mpm/worker worker.c > > > > > Aren't global pools still cleaned up on exit? If the > threads are still > > > running we'll still have the same problem. The only way I > see to fix this > > > is to make sure that all threads have terminated before > cleaning up > > > the pool. > > > > I don't see that they're getting cleaned up on exit. > > Pools that are created with a NULL parent are actually created as > child-pools of the global_pool. The global_pool is destroyed in > apr_pool_terminate(), which is called from apr_terminate(), which > is registered with atexit(). > > > As far as making sure all threads have terminated before cleaning up > > the pool: How do we do that in a graceless shutdown? If we hang > > around much longer, the parent is going to kill us and we won't be > > able to run cleanups anyway. > > I don't see any good way out of this situation, here are two bad ways > I see out: > > 1) check for workers_may_exit upon returning from any > EINTR-returning syscall >Con: yuck, the number of places where we would have to do this is > way too large. > > 2) introduce apr_thread_cancel() >Con: a) many thread libraries leak kernel resources when > threads are >cancelled. > b) We'd also have to introduce > apr_thread_setcancelstate, et al. >*and* we would have to be sure to clean up thing like the >accept mutex (it would be bad to be
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
> > Aren't global pools still cleaned up on exit? If the threads are still > > running we'll still have the same problem. The only way I see to fix this > > is to make sure that all threads have terminated before cleaning up > > the pool. > > I don't see that they're getting cleaned up on exit. Pools that are created with a NULL parent are actually created as child-pools of the global_pool. The global_pool is destroyed in apr_pool_terminate(), which is called from apr_terminate(), which is registered with atexit(). > As far as making sure all threads have terminated before cleaning up > the pool: How do we do that in a graceless shutdown? If we hang > around much longer, the parent is going to kill us and we won't be > able to run cleanups anyway. I don't see any good way out of this situation, here are two bad ways I see out: 1) check for workers_may_exit upon returning from any EINTR-returning syscall Con: yuck, the number of places where we would have to do this is way too large. 2) introduce apr_thread_cancel() Con: a) many thread libraries leak kernel resources when threads are cancelled. b) We'd also have to introduce apr_thread_setcancelstate, et al. *and* we would have to be sure to clean up thing like the accept mutex (it would be bad to be canceled while holding the accept mutex). A couple questions: - What happens when we call shutdown() on the listen socket? Are accepted sockets allowed to remain (I'm assuming so). - Could the listener thread keep a list of accepted socket descriptors and then close them all when we receive the signal to gracelessly stop the server? We could optionally _not_ handle the resulting socket errors (Afterall, that might be good information to have -- the admin just intentionally killed off a bunch of connections.) -aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Aaron Bannert <[EMAIL PROTECTED]> writes: > On Wed, Apr 17, 2002 at 03:45:27PM -, [EMAIL PROTECTED] wrote: > > trawick 02/04/17 08:45:27 > > > > Modified:server/mpm/worker worker.c > > Log: > > use an independent pool for threads so that when we abandon them > > during graceless termination the cleanups on pchild won't mess with > > stuff they are still referencing > > Aren't global pools still cleaned up on exit? If the threads are still > running we'll still have the same problem. The only way I see to fix this > is to make sure that all threads have terminated before cleaning up > the pool. I don't see that they're getting cleaned up on exit. As far as making sure all threads have terminated before cleaning up the pool: How do we do that in a graceless shutdown? If we hang around much longer, the parent is going to kill us and we won't be able to run cleanups anyway. -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
"Sander Striker" <[EMAIL PROTECTED]> writes: > Isn't this the introduction of a memory leak? > The threads keep being created, and the pool is _never_ > cleared. worker does not continually create threads, so no memory leak/no growing footprint over time... the OS cleans it up when we exit -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
Isn't this the introduction of a memory leak?
The threads keep being created, and the pool is _never_
cleared.
Sander
> -Original Message-
> From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]]
> Sent: 17 April 2002 17:45
> To: [EMAIL PROTECTED]
> Subject: cvs commit: httpd-2.0/server/mpm/worker worker.c
>
>
> trawick 02/04/17 08:45:27
>
> Modified:server/mpm/worker worker.c
> Log:
> use an independent pool for threads so that when we abandon them
> during graceless termination the cleanups on pchild won't mess with
> stuff they are still referencing
>
> Revision ChangesPath
> 1.116 +11 -3 httpd-2.0/server/mpm/worker/worker.c
>
> Index: worker.c
> ===
> RCS file: /home/cvs/httpd-2.0/server/mpm/worker/worker.c,v
> retrieving revision 1.115
> retrieving revision 1.116
> diff -u -r1.115 -r1.116
> --- worker.c12 Apr 2002 19:58:52 - 1.115
> +++ worker.c17 Apr 2002 15:45:27 - 1.116
> @@ -228,6 +228,10 @@
>
>static apr_pool_t *pconf; /* Pool for config stuff */
>static apr_pool_t *pchild;/* Pool for httpd child stuff */
> +static apr_pool_t *thread_pool; /* pool for APR to use for our
> + * threads (shouldn't call it
> + * pthreads :) )
> + */
>
>static pid_t ap_my_pid; /* Linux getpid() doesn't work except in main
> thread. Use this instead */
> @@ -916,7 +920,7 @@
>my_info->tid = -1; /* listener thread doesn't have a thread slot */
>my_info->sd = 0;
>rv = apr_thread_create(&ts->listener, thread_attr, listener_thread,
> - my_info, pchild);
> + my_info, thread_pool);
>if (rv != APR_SUCCESS) {
>ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf,
> "apr_thread_create: unable to create listener thread");
> @@ -988,7 +992,7 @@
> * done because it lets us deal with tid better.
> */
>rv = apr_thread_create(&threads[i], thread_attr,
> - worker_thread, my_info, pchild);
> + worker_thread, my_info, thread_pool);
>if (rv != APR_SUCCESS) {
>ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf,
>"apr_thread_create: unable to create worker thread");
> @@ -1118,6 +1122,10 @@
>
>ap_my_pid = getpid();
>apr_pool_create(&pchild, pconf);
> +/* pool passed to apr_thread_create() can't be cleaned up with pchild
> + * since that causes segfaults with graceless termination
> + */
> +apr_pool_create(&thread_pool, NULL);
>
>/*stuff to do before we switch id's, so we have permissions.*/
>ap_reopen_scoreboard(pchild, NULL, 0);
> @@ -1182,7 +1190,7 @@
>ts->threadattr = thread_attr;
>
>rv = apr_thread_create(&start_thread_id, thread_attr, start_threads,
> - ts, pchild);
> + ts, thread_pool);
>if (rv != APR_SUCCESS) {
>ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf,
> "apr_thread_create: unable to create worker thread");
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Fri, 29 Mar 2002, Cliff Woolley wrote: > On Fri, 29 Mar 2002, Doug MacEachern wrote: > > > fyi: t/php/virtual produces the same stacktrace > > I'll look into this this afternoon. great. probably easier to work with t/modules/include2.t, stacktrace looks like they suffer the same problem. > Has PHP really been updated for the new buckets API already?? yup and modperl-2.0 too.
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Fri, 29 Mar 2002, Doug MacEachern wrote: > fyi: t/php/virtual produces the same stacktrace I'll look into this this afternoon. Has PHP really been updated for the new buckets API already?? --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Fri, 29 Mar 2002, Doug MacEachern wrote: > not sure if this is related to the bucket list change or mod_includes > changes or what, but i just checked in a test adapted from modperl that > dumps core. stacktrace below from t/TEST t/modules/include2.t fyi: t/php/virtual produces the same stacktrace i have php checked out like so: % cvs -d ... co -rphp_4_1_2 php4 % cd php4 % cvs co -rphp_4_1_2 Zend TSRM % cd sapi/apache2filter % cvs up -A *.[ch] % cd ../../.. % ./buildconf && ./configure --with-apxs2=... && make
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Fri, 29 Mar 2002, Doug MacEachern wrote: > another problem after fixing the httpd-test c-modules to compile: > t/apache/passbrigade eats all cpu. have not looked into it. nevermind. i didn't notice the modules had been updated and my cvs commit up-to-date check failed. this test is working fine for me now.
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Line 3186 in mod_include is hit when it has run through the whole brigade and found no tags. It is just forwarding the brigade. This will be the normal case for files that get parsed unnecessarily. This seems to work for me. No brigades are harmed in this process. I suspect that there is another problem, not mod_include this time. :) I'm still tracking a different core dump in mod_include. Hopefully I'll have it fixed soon. Paul J. Reder Doug MacEachern wrote: > not sure if this is related to the bucket list change or mod_includes > changes or what, but i just checked in a test adapted from modperl that > dumps core. stacktrace below from t/TEST t/modules/include2.t > > #0 0x0815a897 in ?? () at eval.c:41 > 41 eval.c: No such file or directory. > in eval.c > #1 0x4001dbe3 in apr_brigade_cleanup (data=0x81c77a0) > at >/home/dougm/apache/farm/src/httpd-2.0-cvs/srclib/apr-util/buckets/apr_brigade.c:86 > #2 0x4001dc3c in apr_brigade_destroy (b=0x81c77a0) > at >/home/dougm/apache/farm/src/httpd-2.0-cvs/srclib/apr-util/buckets/apr_brigade.c:97 > #3 0x0807f731 in core_output_filter (f=0x81c7548, b=0x81c77a0) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/core.c:3758 > #4 0x08075bc0 in ap_pass_brigade (next=0x81c7548, bb=0x81cc418) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/util_filter.c:534 > #5 0x08063bce in ap_http_header_filter (f=0x81cfb40, b=0x81cc418) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/modules/http/http_protocol.c:1472 > #6 0x08075bc0 in ap_pass_brigade (next=0x81cfb40, bb=0x81cc418) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/util_filter.c:534 > #7 0x08078a0a in ap_content_length_filter (f=0x81cfb28, b=0x81cc418) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/protocol.c:1263 > #8 0x08075bc0 in ap_pass_brigade (next=0x81cfb28, bb=0x81cc498) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/util_filter.c:534 > #9 0x4031692f in send_parsed_content (bb=0xb344, r=0x81cf1c0, f=0x81cbad8) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/modules/filters/mod_include.c:3186 > #11 0x08075bc0 in ap_pass_brigade (next=0x81cbad8, bb=0x81cbbf0) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/util_filter.c:534 > #12 0x0807e723 in default_handler (r=0x81cf1c0) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/core.c:3247 > #13 0x0806950f in ap_run_handler (r=0x81cf1c0) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/config.c:193 > #14 0x08069b8d in ap_invoke_handler (r=0x81cf1c0) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/config.c:373 > #15 0x080665dd in ap_process_request (r=0x81cf1c0) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/modules/http/http_request.c:261 > #16 0x08061355 in ap_process_http_connection (c=0x81c7270) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/modules/http/http_core.c:291 > #17 0x0807379b in ap_run_process_connection (c=0x81c7270) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/connection.c:85 > #18 0x08073b42 in ap_process_connection (c=0x81c7270, csd=0x81c71a0) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/connection.c:207 > #19 0x08067d6f in child_main (child_num_arg=0) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/mpm/prefork/prefork.c:675 > #20 0x08067ef8 in make_child (s=0x80a5f10, slot=0) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/mpm/prefork/prefork.c:770 > #21 0x08067f6d in startup_children (number_to_start=1) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/mpm/prefork/prefork.c:793 > #22 0x080683a0 in ap_mpm_run (_pconf=0x80a41c0, plog=0x80ce268, s=0x80a5f10) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/mpm/prefork/prefork.c:1016 > #23 0x0806e644 in main (argc=6, argv=0xb654) > at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/main.c:618 > #24 0x401d9507 in __libc_start_main (main=0x806de50 , argc=6, > ubp_av=0xb654, init=0x805eb14 <_init>, fini=0x808cd10 <_fini>, > rtld_fini=0x4000dc14 <_dl_fini>, stack_end=0xb64c) > at ../sysdeps/generic/libc-start.c:129 > > > > -- Paul J. Reder --- "The strength of the Constitution lies entirely in the determination of each citizen to defend it. Only if every single citizen feels duty bound to do his share in this defense are the constitutional rights secure." -- Albert Einstein
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
another problem after fixing the httpd-test c-modules to compile: t/apache/passbrigade eats all cpu. have not looked into it.
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
not sure if this is related to the bucket list change or mod_includes changes or what, but i just checked in a test adapted from modperl that dumps core. stacktrace below from t/TEST t/modules/include2.t #0 0x0815a897 in ?? () at eval.c:41 41 eval.c: No such file or directory. in eval.c #1 0x4001dbe3 in apr_brigade_cleanup (data=0x81c77a0) at /home/dougm/apache/farm/src/httpd-2.0-cvs/srclib/apr-util/buckets/apr_brigade.c:86 #2 0x4001dc3c in apr_brigade_destroy (b=0x81c77a0) at /home/dougm/apache/farm/src/httpd-2.0-cvs/srclib/apr-util/buckets/apr_brigade.c:97 #3 0x0807f731 in core_output_filter (f=0x81c7548, b=0x81c77a0) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/core.c:3758 #4 0x08075bc0 in ap_pass_brigade (next=0x81c7548, bb=0x81cc418) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/util_filter.c:534 #5 0x08063bce in ap_http_header_filter (f=0x81cfb40, b=0x81cc418) at /home/dougm/apache/farm/src/httpd-2.0-cvs/modules/http/http_protocol.c:1472 #6 0x08075bc0 in ap_pass_brigade (next=0x81cfb40, bb=0x81cc418) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/util_filter.c:534 #7 0x08078a0a in ap_content_length_filter (f=0x81cfb28, b=0x81cc418) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/protocol.c:1263 #8 0x08075bc0 in ap_pass_brigade (next=0x81cfb28, bb=0x81cc498) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/util_filter.c:534 #9 0x4031692f in send_parsed_content (bb=0xb344, r=0x81cf1c0, f=0x81cbad8) at /home/dougm/apache/farm/src/httpd-2.0-cvs/modules/filters/mod_include.c:3186 #11 0x08075bc0 in ap_pass_brigade (next=0x81cbad8, bb=0x81cbbf0) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/util_filter.c:534 #12 0x0807e723 in default_handler (r=0x81cf1c0) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/core.c:3247 #13 0x0806950f in ap_run_handler (r=0x81cf1c0) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/config.c:193 #14 0x08069b8d in ap_invoke_handler (r=0x81cf1c0) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/config.c:373 #15 0x080665dd in ap_process_request (r=0x81cf1c0) at /home/dougm/apache/farm/src/httpd-2.0-cvs/modules/http/http_request.c:261 #16 0x08061355 in ap_process_http_connection (c=0x81c7270) at /home/dougm/apache/farm/src/httpd-2.0-cvs/modules/http/http_core.c:291 #17 0x0807379b in ap_run_process_connection (c=0x81c7270) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/connection.c:85 #18 0x08073b42 in ap_process_connection (c=0x81c7270, csd=0x81c71a0) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/connection.c:207 #19 0x08067d6f in child_main (child_num_arg=0) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/mpm/prefork/prefork.c:675 #20 0x08067ef8 in make_child (s=0x80a5f10, slot=0) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/mpm/prefork/prefork.c:770 #21 0x08067f6d in startup_children (number_to_start=1) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/mpm/prefork/prefork.c:793 #22 0x080683a0 in ap_mpm_run (_pconf=0x80a41c0, plog=0x80ce268, s=0x80a5f10) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/mpm/prefork/prefork.c:1016 #23 0x0806e644 in main (argc=6, argv=0xb654) at /home/dougm/apache/farm/src/httpd-2.0-cvs/server/main.c:618 #24 0x401d9507 in __libc_start_main (main=0x806de50 , argc=6, ubp_av=0xb654, init=0x805eb14 <_init>, fini=0x808cd10 <_fini>, rtld_fini=0x4000dc14 <_dl_fini>, stack_end=0xb64c) at ../sysdeps/generic/libc-start.c:129
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
[EMAIL PROTECTED] wrote: >jim 02/03/29 06:33:51 > > Modified:.CHANGES > include scoreboard.h > os/tpf os.c > server scoreboard.c > server/mpm/netware mpm_netware.c > server/mpm/prefork prefork.c > server/mpm/worker worker.c > Log: > The old, legacy (and unused) code in which the scoreboard was totally > and completely contained in a file (SCOREBOARD_FILE) has been > removed. This does not affect scoreboards which are *mapped* to > files using named-shared-memory at all. This implies that scoreboards > must be based, at some level, on native shared memory (mmap, shm_open, > shmget, whatever), but the code has assumed that for quite awhile > now. Having the scoreboard be *based* on a file makes no sense today. > Just one problem: the httpd no longer links. mpm_common.c contains a call to MPM_SYNC_CHILD_TABLE, and for most MPMs that resolves to "(ap_sync_scoreboard_image())" (which no longer exists). --Brian
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On 29 Mar 2002 [EMAIL PROTECTED] wrote: > jwoolley02/03/29 00:17:26 > > Modified:. damn near everything > Log: > BUCKET FREELISTS > > Add an allocator-passing mechanism throughout the bucket brigades API. Yep, that's it... the beast is finally in. I've tested to the best of my ability with every last module I can compile... and I took a stab in the dark at all of the rest (MPMs, mod_isapi, etc). The MPM changes will hopefully compile at least... in a few cases there's probably a better place to create and destroy the bucket allocator so that we can get the memory recycling benefit out of the thing. For now it doesn't matter because apr_bucket_alloc_create() doesn't do anything. I just needed something to pass in to ap_run_create_connection(). Let me know if anything egregious happened and I'll take care of it... --Cliff
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Jeff Trawick wrote: > > No big hurry... I just didn't think it was intended to be the way it > is currently committed. > We needed some way to indicate valid mutex methods, and the change in TAKE1 didn't allow the quote concats. Using a const char array is a good idea, but I was loath to use a nasty global. But to allow for it in help, that's what we got. I'll ignore perchild for the moment and work on adding the mutex name stuff to the struct and a mechanism to grab it. -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ "A society that will trade a little liberty for a little order will lose both and deserve neither" - T.Jefferson
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
[EMAIL PROTECTED] writes: > trawick 02/03/20 11:53:19 > > Modified:server/mpm/worker worker.c > Log: > write a debug message to the log when we're stuck in the sicko state > of trying to take over scoreboard slots that aren't going to be released > (we could also be stalled while taking over slots if a thread in child > gracefully terminating is serving a long-running request) note that this sicko state is really easy to get to today given that the scoreboard isn't cleared on a non-graceful restart... just do apachectl restart... but that will be fixed soon enough and we're left with something more subtle that seems to have been present for a while -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Jim Jagielski <[EMAIL PROTECTED]> writes: > Ideally, this should be localized... other entities may want that info > and we don't want to have "implementations" spread out... > > But the code passes check here as well. > > I'll add this in once the lock stuff settles, unless it bothers you enough > that you'd like it folded in now :) No big hurry... I just didn't think it was intended to be the way it is currently committed. -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
I think I have a version that addresses the concerns... running out to a quick meeting, so I'll test and post a bit later... -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ "A society that will trade a little liberty for a little order will lose both and deserve neither" - T.Jefferson
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Ideally, this should be localized... other entities may want that info and we don't want to have "implementations" spread out... But the code passes check here as well. I'll add this in once the lock stuff settles, unless it bothers you enough that you'd like it folded in now :) -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ "A society that will trade a little liberty for a little order will lose both and deserve neither" - T.Jefferson
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Jeff Trawick <[EMAIL PROTECTED]> writes:
> Jim Jagielski <[EMAIL PROTECTED]> writes:
>
> > Jeff Trawick wrote:
> > >
> > > > Bring 2.0 up to parity, a bit, with how much info we provide to
> > > > the admin regarding valid values for AcceptMutex. Should also
> > > > tell 'em what "default" actually maps to, but that can wait.
> > >
> > > I don't think this works as you desired.
> > >
> > > I just did an update (no diffs in this tree) and for a config with no
> > > AcceptMutex I get this at startup:
> > >
> > > [notice] AcceptMutex: Valid accept mutexes for this platform and MPM
> > > are: default, fcntl, sysvsem, pthread.
Here is a prototype showing the help text displayed via httpd -L,
keeping the use of AP_INIT_TAKE1.
It compiles/runs with no warnings or errors with gcc -Wall and xlc
(IBM's fairly anal compiler for AIX).
Index: server/core.c
===
RCS file: /home/cvspublic/httpd-2.0/server/core.c,v
retrieving revision 1.164
diff -u -r1.164 core.c
--- server/core.c 20 Mar 2002 02:05:42 - 1.164
+++ server/core.c 20 Mar 2002 18:39:39 -
@@ -2802,6 +2802,18 @@
* The AllowOverride of Fileinfo allows webmasters to turn it off
*/
+#ifdef AP_MPM_WANT_SET_ACCEPT_LOCK_MECH
+static const char accept_mutex_help_string[] =
+"default"
+#if APR_HAS_SYSVSEM_SERIALIZE
+", sysvsem"
+#endif
+#if APR_HAS_FCNTL_SERIALIZE
+", fcntl"
+#endif
+;
+#endif
+
static const command_rec core_cmds[] = {
/* Old access config file commands */
@@ -3003,7 +3015,7 @@
#endif
#ifdef AP_MPM_WANT_SET_ACCEPT_LOCK_MECH
AP_INIT_TAKE1("AcceptMutex", ap_mpm_set_accept_lock_mech, NULL, RSRC_CONF, \
- "The system mutex implementation to use for the accept mutex"),
+ accept_mutex_help_string),
#endif
{ NULL }
};
--
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Yes, the perchild one was the main one that concerned me... Aaron Bannert wrote: > > On Wed, Mar 20, 2002 at 01:01:07PM -0500, Jim Jagielski wrote: > > > Accept mutexes should all be apr_proc_mutex_t types, since the apr_lock_t > > > type is being phased out and will be soon removed from APR. > > > > And that's the other issue as well... Let that settle down first :) > > I made an attempt to convert all the remaining uses of apr_lock_t a > few weeks ago, and the only places where it is left will be in pieces > of code that don't compile for me (mod_auth_digest, perchild mpm). > I can't remember if I posted a patch for the beos MPM, but that should > be trivial as well. Prefork, Worker, and WinNT are converted. > > -aaron > -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ "A society that will trade a little liberty for a little order will lose both and deserve neither" - T.Jefferson
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Wed, Mar 20, 2002 at 01:01:07PM -0500, Jim Jagielski wrote: > > Accept mutexes should all be apr_proc_mutex_t types, since the apr_lock_t > > type is being phased out and will be soon removed from APR. > > And that's the other issue as well... Let that settle down first :) I made an attempt to convert all the remaining uses of apr_lock_t a few weeks ago, and the only places where it is left will be in pieces of code that don't compile for me (mod_auth_digest, perchild mpm). I can't remember if I posted a patch for the beos MPM, but that should be trivial as well. Prefork, Worker, and WinNT are converted. -aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Jeff Trawick wrote: > > It doesn't look too cool :) I agree! :) > You don't have to use the macro (of course that extra type-checking > via the designated initializers is nice)... > > If you use the macro, couldn't you pass a variable to the macro, and > use the preprocessor to build the string pointed to by that variable? > > static const char *accept_mutex_help = > "Valid accept mutexes for this platform and MPM are: default" > #if APR_HAS_FLOCK_SERIALIZE > ", flock" > #endif > #if APR_HAS_FCNTL_SERIALIZE > ", fcntl" > #endif > #if APR_HAS_SYSVSEM_SERIALIZE && !defined(PERCHILD_MPM) > ", sysvsem" > #endif > #if APR_HAS_PROC_PTHREAD_SERIALIZE > , pthread" > #endif > "." > > then specify accept_mutex_help as the last parm to AP_INIT_TAKE1()? > I'll be looking at that ;) Not sure if it will actually take a variable though... -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ "A society that will trade a little liberty for a little order will lose both and deserve neither" - T.Jefferson
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Aaron Bannert wrote: > > Accept mutexes should all be apr_proc_mutex_t types, since the apr_lock_t > type is being phased out and will be soon removed from APR. And that's the other issue as well... Let that settle down first :) -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ "A society that will trade a little liberty for a little order will lose both and deserve neither" - T.Jefferson
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Wed, Mar 20, 2002 at 12:41:21PM -0500, Jim Jagielski wrote: > Actually, it does exactly what I intended, for the present. :) > > Agreed, and this is part of what I alluded to above about "tell 'em > what default is"... For right now, since TAKE1 is a macro, you > can't use normal preprocessor "foo " "bar" concats to build up the > supported string, so the info message for AllowMutex is hard to > be very 1.3-like. So, the place to put it is in error log (we need > *some* way of noting valid mutexes). Ideally the solution is > something that reports what default maps to, but it requires some > APR work and some consolidation since the MPM's make use of > different locking calls (apr_proc_mutex_lock, apr_lock_acquire) > and thus need a good solution for a common function for something > like ap_mutex_method_name() with the addition of the name to > the actual lock structs... Accept mutexes should all be apr_proc_mutex_t types, since the apr_lock_t type is being phased out and will be soon removed from APR. Personally I'd like to see the default set in autoconf and dictated to APR. That would give us the side-effect benefit of having the default defined in a cpp symbol. -aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Jim Jagielski <[EMAIL PROTECTED]> writes: > Jeff Trawick wrote: > > > > > Bring 2.0 up to parity, a bit, with how much info we provide to > > > the admin regarding valid values for AcceptMutex. Should also > > > tell 'em what "default" actually maps to, but that can wait. > > > > I don't think this works as you desired. > > > > I just did an update (no diffs in this tree) and for a config with no > > AcceptMutex I get this at startup: > > > > [notice] AcceptMutex: Valid accept mutexes for this platform and MPM > > are: default, fcntl, sysvsem, pthread. > > > > Isn't that the message for httpd -L output? > > > > The 1.3 startup message is something like this: > > > > [notice] Accept mutex: fcntl (Default: fcntl) > > > > much more appropriate > > -- > > Actually, it does exactly what I intended, for the present. :) It doesn't look too cool :) > Agreed, and this is part of what I alluded to above about "tell 'em > what default is"... For right now, since TAKE1 is a macro, you > can't use normal preprocessor "foo " "bar" concats to build up the > supported string, so the info message for AllowMutex is hard to > be very 1.3-like. You don't have to use the macro (of course that extra type-checking via the designated initializers is nice)... If you use the macro, couldn't you pass a variable to the macro, and use the preprocessor to build the string pointed to by that variable? static const char *accept_mutex_help = "Valid accept mutexes for this platform and MPM are: default" #if APR_HAS_FLOCK_SERIALIZE ", flock" #endif #if APR_HAS_FCNTL_SERIALIZE ", fcntl" #endif #if APR_HAS_SYSVSEM_SERIALIZE && !defined(PERCHILD_MPM) ", sysvsem" #endif #if APR_HAS_PROC_PTHREAD_SERIALIZE , pthread" #endif "." then specify accept_mutex_help as the last parm to AP_INIT_TAKE1()? (untested :) ) -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Jeff Trawick wrote: > > > Bring 2.0 up to parity, a bit, with how much info we provide to > > the admin regarding valid values for AcceptMutex. Should also > > tell 'em what "default" actually maps to, but that can wait. > > I don't think this works as you desired. > > I just did an update (no diffs in this tree) and for a config with no > AcceptMutex I get this at startup: > > [notice] AcceptMutex: Valid accept mutexes for this platform and MPM > are: default, fcntl, sysvsem, pthread. > > Isn't that the message for httpd -L output? > > The 1.3 startup message is something like this: > > [notice] Accept mutex: fcntl (Default: fcntl) > > much more appropriate > -- Actually, it does exactly what I intended, for the present. :) Agreed, and this is part of what I alluded to above about "tell 'em what default is"... For right now, since TAKE1 is a macro, you can't use normal preprocessor "foo " "bar" concats to build up the supported string, so the info message for AllowMutex is hard to be very 1.3-like. So, the place to put it is in error log (we need *some* way of noting valid mutexes). Ideally the solution is something that reports what default maps to, but it requires some APR work and some consolidation since the MPM's make use of different locking calls (apr_proc_mutex_lock, apr_lock_acquire) and thus need a good solution for a common function for something like ap_mutex_method_name() with the addition of the name to the actual lock structs... -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ "A society that will trade a little liberty for a little order will lose both and deserve neither" - T.Jefferson
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
[EMAIL PROTECTED] writes: > jim 02/03/20 08:44:13 > > Modified:.CHANGES >include mpm_common.h >server mpm_common.c >server/mpm/mpmt_os2 mpmt_os2.c >server/mpm/netware mpm_netware.c >server/mpm/perchild perchild.c >server/mpm/prefork prefork.c >server/mpm/worker worker.c > Log: > Bring 2.0 up to parity, a bit, with how much info we provide to > the admin regarding valid values for AcceptMutex. Should also > tell 'em what "default" actually maps to, but that can wait. I don't think this works as you desired. I just did an update (no diffs in this tree) and for a config with no AcceptMutex I get this at startup: [notice] AcceptMutex: Valid accept mutexes for this platform and MPM are: default, fcntl, sysvsem, pthread. Isn't that the message for httpd -L output? The 1.3 startup message is something like this: [notice] Accept mutex: fcntl (Default: fcntl) much more appropriate -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Greg Ames <[EMAIL PROTECTED]> writes: > [EMAIL PROTECTED] wrote: > > > +/* XXX unfortunate issue: > > + * with apachectl restart (not graceful), previous generation dies > > + * abruptly with no chance to clean up scoreboard entries; when new > > + * generation is started, processes can loop forever in start_threads() > > + * waiting for scoreboard entries for threads of prior generation to > > + * get cleaned up... > > yikes! > > Did that break when the scoreboard moved to the process pool? Looks like > ap_cleanup_scoreboard won't run any more. no, it is unrelated... it was occurring before yesterday's commits, probably for a long time (I ran into the annoying series of SIGTERM+SIGTERM+SIGTERM+SIGKILL messages when testing some of my changes and went back and then verified that it happened on cvs head) but some knowledge of how the scoreboard used to work shows that the comment shouldn't be right Here is the actual sequence of events that I did: start (wait) graceful (wait) restart (wait) stop This wasn't a once or twice thing. Always after this sequence I saw the annoying messages when I did apachectl stop. Sometimes (40%?) I saw a few such messages also when I did apachectl restart. This was on multiple OSs as well. I only noticed the problem (new children with no place to go) when I did the "restart." But a restart clobbers (well, *did*) the scoreboard so how can a newly-started child be waiting forever for the scoreboard slots in this scenario? While the problem really exists (children that never find any slots), I don't honestly know the cause and intend to remove most of the comment so that it doesn't lead anybody down the wrong path. Also, I think I'll put in a debug message every 180 iterations (3 minutes) if the child process hasn't made any progress taking over more slots. -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Greg Ames wrote: > > [EMAIL PROTECTED] wrote: > > > +/* XXX unfortunate issue: > > + * with apachectl restart (not graceful), previous generation dies > > + * abruptly with no chance to clean up scoreboard entries; when new > > + * generation is started, processes can loop forever in start_threads() > > + * waiting for scoreboard entries for threads of prior generation to > > + * get cleaned up... > > yikes! > > Did that break when the scoreboard moved to the process pool? Looks like > ap_cleanup_scoreboard won't run any more. ooops, sorry. I see that your comment preceded the process pool change. Maybe it was the shmem change that broke it? In any case, we need to design a reliable way to fix the problem you described. Greg
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
[EMAIL PROTECTED] wrote: > +/* XXX unfortunate issue: > + * with apachectl restart (not graceful), previous generation dies > + * abruptly with no chance to clean up scoreboard entries; when new > + * generation is started, processes can loop forever in start_threads() > + * waiting for scoreboard entries for threads of prior generation to > + * get cleaned up... yikes! Did that break when the scoreboard moved to the process pool? Looks like ap_cleanup_scoreboard won't run any more. Greg
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
> > Log: > > The pre_mpm hook creates server-lifetime objects (or at least, for > the > > generations across graceful restarts.) They should use the process > pool. > > The pre_mpm hook doesn't seem generically useful. It seems to be tied > to our current idea of how the scoreboard should be managed. The per_mpm phase was originally written to provide a place to create shared memory that the server can use. Because our best map to how shared memory should work is the scoreboard, that is how the hook was implemented. Ryan
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
[EMAIL PROTECTED] writes:
> wrowe 02/03/19 21:58:21
>
> Modified:server/mpm/beos beos.c
>server/mpm/netware mpm_netware.c
>server/mpm/perchild perchild.c
>server/mpm/prefork prefork.c
>server/mpm/winnt mpm_winnt.c
>server/mpm/worker worker.c
> Log:
> The pre_mpm hook creates server-lifetime objects (or at least, for the
> generations across graceful restarts.) They should use the process pool.
The pre_mpm hook doesn't seem generically useful. It seems to be tied
to our current idea of how the scoreboard should be managed.
> Index: beos.c
> ===
> RCS file: /home/cvs/httpd-2.0/server/mpm/beos/beos.c,v
> retrieving revision 1.88
> retrieving revision 1.89
> diff -u -r1.88 -r1.89
> --- beos.c 15 Mar 2002 00:50:31 - 1.88
> +++ beos.c 20 Mar 2002 05:58:20 - 1.89
> @@ -839,7 +839,7 @@
>
>if (!is_graceful) {
>/* setup the scoreboard shared memory */
> -if (ap_run_pre_mpm(pconf, SB_SHARED) != OK) {
> +if (ap_run_pre_mpm(s->process->pool, SB_SHARED) != OK) {
>return 1;
>}
Now the scoreboard lives forever and it is safe to call
ap_run_pre_mpm() at any restart. One piece I think you have missed is
proper initialization of the scoreboard after a non-graceful restart.
For a graceful restart we don't want cleared scoreboard values (and
we don't).
For a non-graceful restart we do want a fresh (or at least cleared)
scoreboard (and we don't get that any more).
Isn't non-graceful restart now broken? (There is plenty of
opportunity for me to be confused here.)
Can you remind me what was broken before this commit?
--
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Aaron Bannert <[EMAIL PROTECTED]> writes: > On Tue, Mar 05, 2002 at 07:02:46PM -0500, Jeff Trawick wrote: > > > Will they actually hold the semaphore while they are servicing long-lived > > > connections? > > > > no... the semaphore is held only during the poll+accept > > > > > I guess we'd have to make it so that as soon as that worker* > > > is done with that connection it checks to see if it is time to quit w/o > > > hitting the semaphore again. Would that work? > > > > Current code doesn't try to obtain the semaphore again if it is time > > to go away. > > In that case, the mutex error is completely valid and we should be > considering why the listener thread has not escaped from the accept loop > when the POD told it to do so. I don't understand your concern. I've never seen a case where the listener thread doesn't escape from the accept loop. Sometimes it escapes before it checks the pod (because of a mutex error) but at least it escapes. --- Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site: http://www.geocities.com/SiliconValley/Park/9289/ Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Tue, Mar 05, 2002 at 07:02:46PM -0500, Jeff Trawick wrote: > > Will they actually hold the semaphore while they are servicing long-lived > > connections? > > no... the semaphore is held only during the poll+accept > > > I guess we'd have to make it so that as soon as that worker* > > is done with that connection it checks to see if it is time to quit w/o > > hitting the semaphore again. Would that work? > > Current code doesn't try to obtain the semaphore again if it is time > to go away. In that case, the mutex error is completely valid and we should be considering why the listener thread has not escaped from the accept loop when the POD told it to do so. -aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Aaron Bannert <[EMAIL PROTECTED]> writes: > On Tue, Mar 05, 2002 at 05:07:38PM -0500, Jeff Trawick wrote: > > Aaron Bannert <[EMAIL PROTECTED]> writes: > > > > > On Tue, Mar 05, 2002 at 04:53:23PM -0500, Jeff Trawick wrote: > > > > Here is the sequence of events: > > > > > > > > 1) a thread in child process A is waiting on semaphore > > > > 2) graceful restart triggered > > > > 3) parent process cleans up the semaphore > > > > 4) that thread in child process A gets a failure (EIDRM) from the > > > >semaphore obtain and gracefully brings down other threads in child > > > >process A > > > > > > My point is that #3 and #4 are in the wrong order. There should be > > > nobody waiting on a semaphore when the parent cleans it up. > > > > For a graceful restart, the parent can't wait around for all children > > to go away before it cleans up and gets the next generation started. > > It needs to let worker threads in the old children die gradually as > > they finish processing active connection. That process can take a > > long time. > > Will they actually hold the semaphore while they are servicing long-lived > connections? no... the semaphore is held only during the poll+accept > I guess we'd have to make it so that as soon as that worker* > is done with that connection it checks to see if it is time to quit w/o > hitting the semaphore again. Would that work? Current code doesn't try to obtain the semaphore again if it is time to go away. > It still seems weird to > me that we're essentially ignoring accept mutex errors during > graceful. (shrug) -- Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site: http://www.geocities.com/SiliconValley/Park/9289/ Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Tue, Mar 05, 2002 at 05:07:38PM -0500, Jeff Trawick wrote: > Aaron Bannert <[EMAIL PROTECTED]> writes: > > > On Tue, Mar 05, 2002 at 04:53:23PM -0500, Jeff Trawick wrote: > > > Here is the sequence of events: > > > > > > 1) a thread in child process A is waiting on semaphore > > > 2) graceful restart triggered > > > 3) parent process cleans up the semaphore > > > 4) that thread in child process A gets a failure (EIDRM) from the > > >semaphore obtain and gracefully brings down other threads in child > > >process A > > > > My point is that #3 and #4 are in the wrong order. There should be > > nobody waiting on a semaphore when the parent cleans it up. > > For a graceful restart, the parent can't wait around for all children > to go away before it cleans up and gets the next generation started. > It needs to let worker threads in the old children die gradually as > they finish processing active connection. That process can take a > long time. Will they actually hold the semaphore while they are servicing long-lived connections? I guess we'd have to make it so that as soon as that worker* is done with that connection it checks to see if it is time to quit w/o hitting the semaphore again. Would that work? It still seems weird to me that we're essentially ignoring accept mutex errors during graceful. *"worker" meaning the smallest execution unit required to process a request, defined per-MPM. > (I thought about not cleaning up the semaphore at restart time, but > Greg pointed out that doing so doesn't allow the admin to change the > AcceptMutex foo setting without bringing down the server.) Yeah, that sounds reasonable to me. -aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Aaron Bannert <[EMAIL PROTECTED]> writes: > On Tue, Mar 05, 2002 at 04:53:23PM -0500, Jeff Trawick wrote: > > Here is the sequence of events: > > > > 1) a thread in child process A is waiting on semaphore > > 2) graceful restart triggered > > 3) parent process cleans up the semaphore > > 4) that thread in child process A gets a failure (EIDRM) from the > >semaphore obtain and gracefully brings down other threads in child > >process A > > My point is that #3 and #4 are in the wrong order. There should be > nobody waiting on a semaphore when the parent cleans it up. For a graceful restart, the parent can't wait around for all children to go away before it cleans up and gets the next generation started. It needs to let worker threads in the old children die gradually as they finish processing active connection. That process can take a long time. (I thought about not cleaning up the semaphore at restart time, but Greg pointed out that doing so doesn't allow the admin to change the AcceptMutex foo setting without bringing down the server.) -- Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site: http://www.geocities.com/SiliconValley/Park/9289/ Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Tue, Mar 05, 2002 at 04:53:23PM -0500, Jeff Trawick wrote: > Here is the sequence of events: > > 1) a thread in child process A is waiting on semaphore > 2) graceful restart triggered > 3) parent process cleans up the semaphore > 4) that thread in child process A gets a failure (EIDRM) from the >semaphore obtain and gracefully brings down other threads in child >process A My point is that #3 and #4 are in the wrong order. There should be nobody waiting on a semaphore when the parent cleans it up. > Normally in step 4 we'd get a high-severity log message. I changed > the code to set the severity to debug since everything is working as > designed. The process isn't going away prematurely, threads with > active connections have a chance to finish serving the connection, > etc. > > A variation on the sequence above is > > 1) a thread in child process B holds the semaphore and is blocked in >poll() > 2) graceful restart triggered > 3) parent process cleans up semaphore and wakes up children > 4) that thread in child process B returns from poll() and gets a >failure from the semaphore release and tries to gracefully bring >down other threads in child process B > > As with the previous scenario, everything is working fine. There is > no sense having a high-priority log message about an expected > condition. Again, same problem. The children should all be gone by the time the parent cleans up the semaphore. The POD should ensure that nobody is sitting in poll, since the single listener will wake up and read from the POD, signal all siblings to die, and then release the semaphore. -aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Aaron Bannert <[EMAIL PROTECTED]> writes: > I disagree with this patch. If we are getting failures on locks > then we have a bug somewhere and I'd rather not see us cover up > the problem by decreasing the verbosity. Here is the sequence of events: 1) a thread in child process A is waiting on semaphore 2) graceful restart triggered 3) parent process cleans up the semaphore 4) that thread in child process A gets a failure (EIDRM) from the semaphore obtain and gracefully brings down other threads in child process A Normally in step 4 we'd get a high-severity log message. I changed the code to set the severity to debug since everything is working as designed. The process isn't going away prematurely, threads with active connections have a chance to finish serving the connection, etc. A variation on the sequence above is 1) a thread in child process B holds the semaphore and is blocked in poll() 2) graceful restart triggered 3) parent process cleans up semaphore and wakes up children 4) that thread in child process B returns from poll() and gets a failure from the semaphore release and tries to gracefully bring down other threads in child process B As with the previous scenario, everything is working fine. There is no sense having a high-priority log message about an expected condition. -- Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site: http://www.geocities.com/SiliconValley/Park/9289/ Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
I disagree with this patch. If we are getting failures on locks
then we have a bug somewhere and I'd rather not see us cover up
the problem by decreasing the verbosity.
-aaron
On Tue, Mar 05, 2002 at 09:18:07PM -, [EMAIL PROTECTED] wrote:
> trawick 02/03/05 13:18:07
>
> Modified:server/mpm/worker worker.c
> Log:
> failures on the accept mutex are common at restart time, so be smart
> about the log level and use APLOG_DEBUG if we're restarting
>
> Revision ChangesPath
> 1.84 +14 -2 httpd-2.0/server/mpm/worker/worker.c
>
> Index: worker.c
> ===
> RCS file: /home/cvs/httpd-2.0/server/mpm/worker/worker.c,v
> retrieving revision 1.83
> retrieving revision 1.84
> diff -u -r1.83 -r1.84
> --- worker.c5 Mar 2002 21:01:24 - 1.83
> +++ worker.c5 Mar 2002 21:18:07 - 1.84
> @@ -622,7 +622,13 @@
>
>if ((rv = SAFE_ACCEPT(apr_proc_mutex_lock(accept_mutex)))
>!= APR_SUCCESS) {
> -ap_log_error(APLOG_MARK, APLOG_EMERG, rv, ap_server_conf,
> +int level = APLOG_EMERG;
> +
> +if (ap_scoreboard_image->parent[process_slot].generation !=
> +ap_scoreboard_image->global->running_generation) {
> +level = APLOG_DEBUG; /* common to get these at restart time */
> +}
> +ap_log_error(APLOG_MARK, level, rv, ap_server_conf,
> "apr_proc_mutex_lock failed. Attempting to shutdown "
> "process gracefully.");
>signal_workers();
> @@ -694,7 +700,13 @@
>}
>if ((rv = SAFE_ACCEPT(apr_proc_mutex_unlock(accept_mutex)))
>!= APR_SUCCESS) {
> -ap_log_error(APLOG_MARK, APLOG_EMERG, rv, ap_server_conf,
> +int level = APLOG_EMERG;
> +
> +if (ap_scoreboard_image->parent[process_slot].generation !=
> +ap_scoreboard_image->global->running_generation) {
> +level = APLOG_DEBUG; /* common to get these at restart time */
> +}
> +ap_log_error(APLOG_MARK, level, rv, ap_server_conf,
> "apr_proc_mutex_unlock failed. Attempting to "
> "shutdown process gracefully.");
>signal_workers();
>
>
>
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Thu, Feb 14, 2002 at 02:48:19AM -, [EMAIL PROTECTED] wrote: > aaron 02/02/13 18:48:19 > > Modified:server/mpm/worker worker.c > Log: > Retain signal handling in the worker MPM for the one_process case > (httpd with -DDEBUG, -X, or -DONE_PROCESS). > > Fix -X, -DNO_DETACH, -DONE_PROCESS, etc. flags. > > Tested on solaris w/ start/stop, restart, graceful, and with the > above debugging flags. Please review and test this code. It's a little hairy and I want to make sure I got all the cases on all the platforms and didn't break anything in the process. -aaron
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
I finally had time to review this patch, and I have some comments.
> Log:
> This patch restores most of Ryan's patch (11/12/2001) to remove the
> client_socket from the conn_rec. Diffs from Ryan's patch include:
>
> - rename the create_connection hook to install_transport_filters
> - move the point of invocation of the hook till after the call to
> after ap_update_vhost_given_ip to enable the hook to use vhost
> config info in its decision making.
>
> -AP_CORE_DECLARE(void) ap_process_connection(conn_rec *c)
> +AP_CORE_DECLARE(void) ap_process_connection(conn_rec *c,
apr_socket_t *csd)
I was very careful to keep the csd a void * instead of an apr_socket_t
*, because you have no way of knowing that the module that did the
accept will be giving us a socket, or at least not an apr_socket_t.
Right now, Aaron and I are hacking Apache to allow it to accept
connections over a Unix Domain Socket. This patch will break that
abstraction completely.
>{
>ap_update_vhost_given_ip(c);
>
> +ap_run_install_transport_filters(c, csd);
> +
>ap_run_pre_connection(c);
Why did we create a new hook to put here? This is the last place a new
hook is needed, in five lines of code in this function, we call three
hooks. Why not just add an argument to the pre_connection hook that
gives people access to the socket?
Unless there are any complaints, I'll be making both of those changes
today or tomorrow.
Ryan
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
> Modified:.CHANGES STATUS >server/mpm/perchild perchild.c >server/mpm/prefork prefork.c >server/mpm/worker worker.c > Log: > Not being able to bind to a socket is a fatal error. This makes all > MPMs treat it as such. We now print a message to the console, and > return > a non-zero status code. That should have read "all Unix MPMs". If I can figure out the rest of the MPMs, I will port this change to them as well. Ryan
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
From: "Bill Stoddard" <[EMAIL PROTECTED]> Sent: Monday, February 04, 2002 12:01 PM > Actually, this patch is not the only (or even main) culprit. The switch to use the >shared > scoreboard is fatally broken. I am in the process of keeping the spirit of Ryan's >patch to > only use pre_mpm in the parent and revert back to non shared scoreboard for Windows. Agreed. But I think you are seeing a problem reported some time back, before the shared score was even implemented. Seems [perhaps] that some users can't run as a service with the bin/ pathing we implemented several months ago. Users report that moving everything from bin/ into the apache root works. I've never seen this, so I can't comment. But please do _not_ go reverting patches without reviewing them... would you kindly explain exactly _what_ is fatally broken? Bill
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
Ahhh, I hadn't tried that yet. Ryan -- Ryan Bloom [EMAIL PROTECTED] 645 Howard St. [EMAIL PROTECTED] San Francisco, CA > -Original Message- > From: Bill Stoddard [mailto:[EMAIL PROTECTED]] > Sent: Monday, February 04, 2002 10:04 AM > To: [EMAIL PROTECTED]; [EMAIL PROTECTED] > Subject: Re: cvs commit: httpd-2.0/server/mpm/worker worker.c > > Start apache as a service. That seems to make a difference. > > Bill > > > I'm not having this problem. I just started Apache, and killed off the > > child process five times on my XP box. Each time, a new child was > > created to take its place. > > > > I stopped the child by opening the Task Manager, and killing the > > process. It took some time to make sure I wasn't killing the parent > > process, because for some reason, the pids don't just go in increasing > > order like you would expect. > > > > I would help debug this problem, but since I can't duplicate, I am kind > > of in the dark. The best I can suggest, is take a look at the > > information that is being set about the generation in the scoreboard. > > It is possible that some of the fields in the scoreboard aren't being > > initialized in the parent anymore. > > > > Ryan > > > > -- > > Ryan Bloom [EMAIL PROTECTED] > > 645 Howard St. [EMAIL PROTECTED] > > San Francisco, CA > > > > > -----Original Message- > > > From: Bill Stoddard [mailto:[EMAIL PROTECTED]] > > > Sent: Monday, February 04, 2002 8:58 AM > > > To: [EMAIL PROTECTED] > > > Subject: Fw: cvs commit: httpd-2.0/server/mpm/worker worker.c > > > > > > This patch seems to have broken restarts and child recovery on > > Windows. > > > Investigating... > > > > > > Bill > > > >
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Mon, Feb 04, 2002 at 01:01:49PM -0500, Bill Stoddard wrote: > Actually, this patch is not the only (or even main) culprit. The switch to use the >shared > scoreboard is fatally broken. I am in the process of keeping the spirit of Ryan's >patch to > only use pre_mpm in the parent and revert back to non shared scoreboard for Windows. How is it fatally broken? -aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Start apache as a service. That seems to make a difference. Bill > I'm not having this problem. I just started Apache, and killed off the > child process five times on my XP box. Each time, a new child was > created to take its place. > > I stopped the child by opening the Task Manager, and killing the > process. It took some time to make sure I wasn't killing the parent > process, because for some reason, the pids don't just go in increasing > order like you would expect. > > I would help debug this problem, but since I can't duplicate, I am kind > of in the dark. The best I can suggest, is take a look at the > information that is being set about the generation in the scoreboard. > It is possible that some of the fields in the scoreboard aren't being > initialized in the parent anymore. > > Ryan > > -- > Ryan Bloom [EMAIL PROTECTED] > 645 Howard St. [EMAIL PROTECTED] > San Francisco, CA > > > -Original Message- > > From: Bill Stoddard [mailto:[EMAIL PROTECTED]] > > Sent: Monday, February 04, 2002 8:58 AM > > To: [EMAIL PROTECTED] > > Subject: Fw: cvs commit: httpd-2.0/server/mpm/worker worker.c > > > > This patch seems to have broken restarts and child recovery on > Windows. > > Investigating... > > > > Bill > >
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Actually, this patch is not the only (or even main) culprit. The switch to use the
shared
scoreboard is fatally broken. I am in the process of keeping the spirit of Ryan's
patch to
only use pre_mpm in the parent and revert back to non shared scoreboard for Windows.
Bill
- Original Message -
From: "Bill Stoddard" <[EMAIL PROTECTED]>
To: <[EMAIL PROTECTED]>
Sent: Monday, February 04, 2002 11:57 AM
Subject: Fw: cvs commit: httpd-2.0/server/mpm/worker worker.c
> This patch seems to have broken restarts and child recovery on Windows.
Investigating...
>
> Bill
>
>
> > rbb 02/01/30 14:35:57
> >
> > Modified:include scoreboard.h
> >server scoreboard.c
> >server/mpm/prefork prefork.c
> >server/mpm/winnt mpm_winnt.c
> >server/mpm/worker worker.c
> > Log:
> > Change the Windows MPM to only use the pre_mpm phase in the parent process.
> > The child processes use the child_init phase to reattach to the shared
> > memory. This makes Windows work like Unix, which should make it easier
> > for module authors to write portable modules.
> >
> > Revision ChangesPath
> > 1.39 +3 -3 httpd-2.0/include/scoreboard.h
> >
> > Index: scoreboard.h
> > ===
> > RCS file: /home/cvs/httpd-2.0/include/scoreboard.h,v
> > retrieving revision 1.38
> > retrieving revision 1.39
> > diff -u -r1.38 -r1.39
> > --- scoreboard.h 28 Jan 2002 00:41:31 - 1.38
> > +++ scoreboard.h 30 Jan 2002 22:35:56 - 1.39
> > @@ -73,6 +73,7 @@
> >#include "apr_hooks.h"
> >#include "apr_thread_proc.h"
> >#include "apr_portable.h"
> > +#include "apr_shm.h"
> >
> >/* Scoreboard info on a process is, for now, kept very brief ---
> > * just status value and pid (the latter so that the caretaker process
> > @@ -113,8 +114,7 @@
> > */
> >typedef enum {
> >SB_NOT_SHARED = 1,
> > -SB_SHARED = 2, /* PARENT */
> > -SB_SHARED_CHILD = 3
> > +SB_SHARED = 2
> >} ap_scoreboard_e;
> >
> >#define SB_WORKING 0 /* The server is busy and the child is useful. */
> > @@ -185,7 +185,7 @@
> >AP_DECLARE(void) ap_increment_counts(ap_sb_handle_t *sbh, request_rec *r);
> >
> >int ap_create_scoreboard(apr_pool_t *p, ap_scoreboard_e t);
> > -apr_status_t reopen_scoreboard(apr_pool_t *p, int detached);
> > +apr_status_t ap_reopen_scoreboard(apr_pool_t *p, apr_shm_t **shm, int detached);
> >void ap_init_scoreboard(void *shared_score);
> >int ap_calc_scoreboard_size(void);
> >apr_status_t ap_cleanup_scoreboard(void *d);
> >
> >
> >
> > 1.53 +9 -17 httpd-2.0/server/scoreboard.c
> >
> > Index: scoreboard.c
> > ===
> > RCS file: /home/cvs/httpd-2.0/server/scoreboard.c,v
> > retrieving revision 1.52
> > retrieving revision 1.53
> > diff -u -r1.52 -r1.53
> > --- scoreboard.c 28 Jan 2002 00:41:31 - 1.52
> > +++ scoreboard.c 30 Jan 2002 22:35:56 - 1.53
> > @@ -143,7 +143,8 @@
> >{
> >char *more_storage;
> >int i;
> > -
> > +
> > + ap_calc_scoreboard_size();
> >ap_scoreboard_image =
> >calloc(1, sizeof(scoreboard) + server_limit * sizeof(worker_score *));
> >more_storage = shared_score;
> > @@ -200,7 +201,7 @@
> >/* If detach is non-zero, this is a seperate child process,
> > * if zero, it is a forked child.
> > */
> > -apr_status_t reopen_scoreboard(apr_pool_t *p, int detached)
> > +apr_status_t ap_reopen_scoreboard(apr_pool_t *p, apr_shm_t **shm, int detached)
> >{
> >#if APR_HAS_SHARED_MEMORY
> >if (!detached) {
> > @@ -215,6 +216,9 @@
> >}
> >/* everything will be cleared shortly */
> >#endif
> > +if (*shm) {
> > +*shm = ap_scoreboard_shm;
> > +}
> >return APR_SUCCESS;
> >}
> >
> > @@ -260,14 +264,6 @@
> >memset(sb_shared, 0, scoreboard_size);
> >ap_init_scoreboard(sb_shared);
> >}
> > -else if (sb_type == SB_SHARED_CHILD) {
> > -void *sb_shared;
> > -rv = reopen_scoreboard(p, 1);
&
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
I'm not having this problem. I just started Apache, and killed off the child process five times on my XP box. Each time, a new child was created to take its place. I stopped the child by opening the Task Manager, and killing the process. It took some time to make sure I wasn't killing the parent process, because for some reason, the pids don't just go in increasing order like you would expect. I would help debug this problem, but since I can't duplicate, I am kind of in the dark. The best I can suggest, is take a look at the information that is being set about the generation in the scoreboard. It is possible that some of the fields in the scoreboard aren't being initialized in the parent anymore. Ryan -- Ryan Bloom [EMAIL PROTECTED] 645 Howard St. [EMAIL PROTECTED] San Francisco, CA > -Original Message- > From: Bill Stoddard [mailto:[EMAIL PROTECTED]] > Sent: Monday, February 04, 2002 8:58 AM > To: [EMAIL PROTECTED] > Subject: Fw: cvs commit: httpd-2.0/server/mpm/worker worker.c > > This patch seems to have broken restarts and child recovery on Windows. > Investigating... > > Bill
Fw: cvs commit: httpd-2.0/server/mpm/worker worker.c
This patch seems to have broken restarts and child recovery on Windows.
Investigating...
Bill
> rbb 02/01/30 14:35:57
>
> Modified:include scoreboard.h
>server scoreboard.c
>server/mpm/prefork prefork.c
>server/mpm/winnt mpm_winnt.c
>server/mpm/worker worker.c
> Log:
> Change the Windows MPM to only use the pre_mpm phase in the parent process.
> The child processes use the child_init phase to reattach to the shared
> memory. This makes Windows work like Unix, which should make it easier
> for module authors to write portable modules.
>
> Revision ChangesPath
> 1.39 +3 -3 httpd-2.0/include/scoreboard.h
>
> Index: scoreboard.h
> ===
> RCS file: /home/cvs/httpd-2.0/include/scoreboard.h,v
> retrieving revision 1.38
> retrieving revision 1.39
> diff -u -r1.38 -r1.39
> --- scoreboard.h 28 Jan 2002 00:41:31 - 1.38
> +++ scoreboard.h 30 Jan 2002 22:35:56 - 1.39
> @@ -73,6 +73,7 @@
>#include "apr_hooks.h"
>#include "apr_thread_proc.h"
>#include "apr_portable.h"
> +#include "apr_shm.h"
>
>/* Scoreboard info on a process is, for now, kept very brief ---
> * just status value and pid (the latter so that the caretaker process
> @@ -113,8 +114,7 @@
> */
>typedef enum {
>SB_NOT_SHARED = 1,
> -SB_SHARED = 2, /* PARENT */
> -SB_SHARED_CHILD = 3
> +SB_SHARED = 2
>} ap_scoreboard_e;
>
>#define SB_WORKING 0 /* The server is busy and the child is useful. */
> @@ -185,7 +185,7 @@
>AP_DECLARE(void) ap_increment_counts(ap_sb_handle_t *sbh, request_rec *r);
>
>int ap_create_scoreboard(apr_pool_t *p, ap_scoreboard_e t);
> -apr_status_t reopen_scoreboard(apr_pool_t *p, int detached);
> +apr_status_t ap_reopen_scoreboard(apr_pool_t *p, apr_shm_t **shm, int detached);
>void ap_init_scoreboard(void *shared_score);
>int ap_calc_scoreboard_size(void);
>apr_status_t ap_cleanup_scoreboard(void *d);
>
>
>
> 1.53 +9 -17 httpd-2.0/server/scoreboard.c
>
> Index: scoreboard.c
> ===
> RCS file: /home/cvs/httpd-2.0/server/scoreboard.c,v
> retrieving revision 1.52
> retrieving revision 1.53
> diff -u -r1.52 -r1.53
> --- scoreboard.c 28 Jan 2002 00:41:31 - 1.52
> +++ scoreboard.c 30 Jan 2002 22:35:56 - 1.53
> @@ -143,7 +143,8 @@
>{
>char *more_storage;
>int i;
> -
> +
> + ap_calc_scoreboard_size();
>ap_scoreboard_image =
>calloc(1, sizeof(scoreboard) + server_limit * sizeof(worker_score *));
>more_storage = shared_score;
> @@ -200,7 +201,7 @@
>/* If detach is non-zero, this is a seperate child process,
> * if zero, it is a forked child.
> */
> -apr_status_t reopen_scoreboard(apr_pool_t *p, int detached)
> +apr_status_t ap_reopen_scoreboard(apr_pool_t *p, apr_shm_t **shm, int detached)
>{
>#if APR_HAS_SHARED_MEMORY
>if (!detached) {
> @@ -215,6 +216,9 @@
>}
>/* everything will be cleared shortly */
>#endif
> +if (*shm) {
> +*shm = ap_scoreboard_shm;
> +}
>return APR_SUCCESS;
>}
>
> @@ -260,14 +264,6 @@
>memset(sb_shared, 0, scoreboard_size);
>ap_init_scoreboard(sb_shared);
>}
> -else if (sb_type == SB_SHARED_CHILD) {
> -void *sb_shared;
> -rv = reopen_scoreboard(p, 1);
> -if (rv || !(sb_shared = apr_shm_baseaddr_get(ap_scoreboard_shm))) {
> -return HTTP_INTERNAL_SERVER_ERROR;
> -}
> -ap_init_scoreboard(sb_shared);
> -}
>else
>#endif
>{
> @@ -282,14 +278,10 @@
>ap_init_scoreboard(sb_mem);
>}
>}
> -/* can't just memset() */
> -if (sb_type != SB_SHARED_CHILD) {
> -ap_scoreboard_image->global->sb_type = sb_type;
> -ap_scoreboard_image->global->running_generation = running_gen;
> -apr_pool_cleanup_register(p, NULL, ap_cleanup_scoreboard,
> - apr_pool_cleanup_null);
> -}
> +ap_scoreboard_image->global->sb_type = sb_type;
> +ap_scoreboard_image->global->running_generation = running_gen;
>ap_restart_time = apr_time_now();
> +apr_pool_cleanup_register(p, NULL, ap_cleanup_scoreboard,
>apr_pool_cleanup_null);
>return OK;
>}
>
>
>
>
> 1.236 +1 -1 httpd-2.0/server/mpm/prefork/prefork.c
>
> Index: prefork.c
> ===
> RCS file: /home/cvs/httpd-2.0/server/mpm/prefork/prefork.c,v
> retrieving revision 1.235
> retrieving revision 1.236
> diff -u -r1.235 -r1.236
> --- prefork.c 29 Jan 2002 22:31:25 - 1.
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
[EMAIL PROTECTED] wrote: >trawick 02/01/30 03:56:26 > > Modified:server/mpm/worker worker.c > Log: > get rid of a bunch of warnings about unused variables > Thanks for catching that. I really need to start adding -Wall to all my makefiles --Brian
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
> On Sun, 27 Jan 2002, Bill Stoddard wrote: > > > > 1) Do exactly what I told you to do when I committed the patch > > > originally. Let's take an example of SSL. Your SSL vhost must know > > > which socket it is on, because it can't be on a plain HTTP socket. So, > > > for that socket, you replace the accept_function, and that allows you to > > > use your own create_conn hook to create the connection and insert your > > > own filters for the bottom of the stack. In fact, regardless of what > > > you are configuring, if you are creating a vhost that doesn't speak > > > plain HTTP, you must know which socket you are listening on, and you can > > > do exactly what you want. > > > > > An SSL socket is a normal BSD socket for the purposed of accept(). No distinction >is made, > > so this suggestion isn't sufficient to solve the problem. > > This suggestion can solve the problem, but you would need to add > information to the ssl_Accept function. Yes, under the covers this would > look very much like unixd_accept, but it would allow your > create_connection function to do the right thing. > > > > 2) Ignore this patch, and do what you would do if it wasn't there, > > > which I presume is to add another filter above the CORE_IN and CORE > > > filters so that they are never called. That will work just fine with > > > this patch in place. > > > > Partially true. I was adding the SSL_IN/SSL_OUT filters in pre_connection which no longer > > has access to the socket because it is hidden away in the core_net_rec structure. Relying > > on the hack is not a cool way to do this. There were some other things broken with this as > > well that I don't have time to get into. Also, adding core in/out filters in the > > pre_connection hook is just wrong. I have a suggestion to fix this. > > I would prefer to know what else was broken with this. You are correct > that you didn't have access to the socket, you shouldn't. > That was the whole point of the change. I agree > If you are using the CORE/CORE_IN filters, > then you don't need access to the socket. If you aren't using the > CORE/CORE_IN filters, then you should also take control of the accept, > because they are all tied together. Ryan, that last argument is pedantic and I completely disagree with it. If a module author is satisfied with the accept logic the MPM proivdes, there is no reason to make the module author duplicate it in an accept function. Sure, in the documentation, advise against it on principle, but don't force unnecessary work on module authors just to satisfy pedantic urges. Can we compromise? My recommendation captures 95% of the essence of what you are trying to accomplish yet still provides a bit of additional freedom to module authors. It is MUCH closer to what you want that what we have now. In fact, I would argue that it is by some accounts better than your original code (its more intuitive that only one network i/o filter should ever be installed and it provides the same degree of hiding of the client socket). Bill > > Ryan > > > > > I think this suggestion should make everyone happy: > > Leave ap_new_connection() in place, do NOT pass in the csd though (and remove > > client_socket from conn_rec). Create a new hook that runs immediately before the > > pre_connection hook (and after ap_update_vhost_given_ip(), I think that is the > > function...). This new hook is a RUN_FIRST hook that installs the desired network >io > > filter (CORE_IN, CORE_OUT, SSL_IN, SSL_OUT whatever). Need to pass this hook the >csd and > > conn_rec. The difference between this new hook and pre_connection hook is that the >new > > hook is a RUN_FIRST hook. This will guarantee that one and only one network i/o >filter > > will ever get installed. The new hook allocates and inits the core_net_rec to >maintain the > > client socket. This is -very- close to your original patch. > > > > Could name the new hook install_networkio or something like that. If you like this idea, I > > will implement it tonight. > > > > ___ > Ryan Bloom[EMAIL PROTECTED] > 550 Jean St > Oakland CA 94610 > --- > >
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Sun, 27 Jan 2002, Bill Stoddard wrote: > > 1) Do exactly what I told you to do when I committed the patch > > originally. Let's take an example of SSL. Your SSL vhost must know > > which socket it is on, because it can't be on a plain HTTP socket. So, > > for that socket, you replace the accept_function, and that allows you to > > use your own create_conn hook to create the connection and insert your > > own filters for the bottom of the stack. In fact, regardless of what > > you are configuring, if you are creating a vhost that doesn't speak > > plain HTTP, you must know which socket you are listening on, and you can > > do exactly what you want. > > > An SSL socket is a normal BSD socket for the purposed of accept(). No distinction is >made, > so this suggestion isn't sufficient to solve the problem. This suggestion can solve the problem, but you would need to add information to the ssl_Accept function. Yes, under the covers this would look very much like unixd_accept, but it would allow your create_connection function to do the right thing. > > 2) Ignore this patch, and do what you would do if it wasn't there, > > which I presume is to add another filter above the CORE_IN and CORE > > filters so that they are never called. That will work just fine with > > this patch in place. > > Partially true. I was adding the SSL_IN/SSL_OUT filters in pre_connection which no >longer > has access to the socket because it is hidden away in the core_net_rec structure. >Relying > on the hack is not a cool way to do this. There were some other things broken with >this as > well that I don't have time to get into. Also, adding core in/out filters in the > pre_connection hook is just wrong. I have a suggestion to fix this. I would prefer to know what else was broken with this. You are correct that you didn't have access to the socket, you shouldn't. That was the whole point of the change. If you are using the CORE/CORE_IN filters, then you don't need access to the socket. If you aren't using the CORE/CORE_IN filters, then you should also take control of the accept, because they are all tied together. Basically, the accept function and the end_point filters are the only locations that should have any network functions, and all of those functions need to agree. That is why it isn't allowed to implement one without the other. Separating the filters from the accept function is wrong IMHO, because you are breaking the abstraction of the network logic. Ryan > > I think this suggestion should make everyone happy: > Leave ap_new_connection() in place, do NOT pass in the csd though (and remove > client_socket from conn_rec). Create a new hook that runs immediately before the > pre_connection hook (and after ap_update_vhost_given_ip(), I think that is the > function...). This new hook is a RUN_FIRST hook that installs the desired network io > filter (CORE_IN, CORE_OUT, SSL_IN, SSL_OUT whatever). Need to pass this hook the csd >and > conn_rec. The difference between this new hook and pre_connection hook is that the >new > hook is a RUN_FIRST hook. This will guarantee that one and only one network i/o >filter > will ever get installed. The new hook allocates and inits the core_net_rec to >maintain the > client socket. This is -very- close to your original patch. > > Could name the new hook install_networkio or something like that. If you like this >idea, I > will implement it tonight. ___ Ryan Bloom [EMAIL PROTECTED] 550 Jean St Oakland CA 94610 ---
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
> There are two ways to do it. > > 1) Do exactly what I told you to do when I committed the patch > originally. Let's take an example of SSL. Your SSL vhost must know > which socket it is on, because it can't be on a plain HTTP socket. So, > for that socket, you replace the accept_function, and that allows you to > use your own create_conn hook to create the connection and insert your > own filters for the bottom of the stack. In fact, regardless of what > you are configuring, if you are creating a vhost that doesn't speak > plain HTTP, you must know which socket you are listening on, and you can > do exactly what you want. > An SSL socket is a normal BSD socket for the purposed of accept(). No distinction is made, so this suggestion isn't sufficient to solve the problem. > 2) Ignore this patch, and do what you would do if it wasn't there, > which I presume is to add another filter above the CORE_IN and CORE > filters so that they are never called. That will work just fine with > this patch in place. Partially true. I was adding the SSL_IN/SSL_OUT filters in pre_connection which no longer has access to the socket because it is hidden away in the core_net_rec structure. Relying on the hack is not a cool way to do this. There were some other things broken with this as well that I don't have time to get into. Also, adding core in/out filters in the pre_connection hook is just wrong. I have a suggestion to fix this. I think this suggestion should make everyone happy: Leave ap_new_connection() in place, do NOT pass in the csd though (and remove client_socket from conn_rec). Create a new hook that runs immediately before the pre_connection hook (and after ap_update_vhost_given_ip(), I think that is the function...). This new hook is a RUN_FIRST hook that installs the desired network io filter (CORE_IN, CORE_OUT, SSL_IN, SSL_OUT whatever). Need to pass this hook the csd and conn_rec. The difference between this new hook and pre_connection hook is that the new hook is a RUN_FIRST hook. This will guarantee that one and only one network i/o filter will ever get installed. The new hook allocates and inits the core_net_rec to maintain the client socket. This is -very- close to your original patch. Could name the new hook install_networkio or something like that. If you like this idea, I will implement it tonight. Bill > > Ryan > > -- > Ryan Bloom [EMAIL PROTECTED] > 645 Howard St. [EMAIL PROTECTED] > San Francisco, CA > > > -Original Message- > > From: Bill Stoddard [mailto:[EMAIL PROTECTED]] > > Sent: Sunday, January 27, 2002 12:55 PM > > To: [EMAIL PROTECTED]; [EMAIL PROTECTED] > > Subject: Re: cvs commit: httpd-2.0/server/mpm/worker worker.c > > > > Ryan, you are vetoing my veto. Consider this patch a veto of your > 11/12 > > patch. I am open > > to suggestions to determine a compromise. > > > > I need to replace core_in and core_out based on configuration. How can > I > > do that with your > > patch in place? > > > > Bill > > > > > > - Original Message - > > From: "Ryan Bloom" <[EMAIL PROTECTED]> > > To: <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]> > > Sent: Sunday, January 27, 2002 3:05 PM > > Subject: RE: cvs commit: httpd-2.0/server/mpm/worker worker.c > > > > > > > > stoddard02/01/27 04:52:08 > > > > > > > > Modified:.CHANGES > > > >include http_connection.h httpd.h > > > >modules/http http_core.c > > > >modules/proxy proxy_ftp.c proxy_http.c > > > >server connection.c core.c > > > >server/mpm/beos beos.c > > > >server/mpm/mpmt_os2 mpmt_os2_child.c > > > >server/mpm/netware mpm_netware.c > > > >server/mpm/perchild perchild.c > > > >server/mpm/prefork prefork.c > > > >server/mpm/winnt mpm_winnt.c > > > >server/mpm/worker worker.c > > > > Log: > > > > Remove the create_connection hook and put the client_socket back > > > into > > > > the > > > > conn_rec. The create_connection_hook has a design flaw that > prevents > > > it > > > > from making decisions based on vhost information. > > > > > > -1. You can't back out a patch because it doesn't do something it > > > wasn't designed to do! That hook was meant to allow different > filters > > > to be added based on the connection used, not based on the > > > configuration. If you want to do something based on the > configuration, > > > you have to wait until the request has been read. Removing the > socket > > > from the conn_rec has all sorts of advantages, not the least being > that > > > it keeps people from using the socket without going through filters. > > > > > > Ryan > > > > > > > > > > >
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
There are two ways to do it. 1) Do exactly what I told you to do when I committed the patch originally. Let's take an example of SSL. Your SSL vhost must know which socket it is on, because it can't be on a plain HTTP socket. So, for that socket, you replace the accept_function, and that allows you to use your own create_conn hook to create the connection and insert your own filters for the bottom of the stack. In fact, regardless of what you are configuring, if you are creating a vhost that doesn't speak plain HTTP, you must know which socket you are listening on, and you can do exactly what you want. 2) Ignore this patch, and do what you would do if it wasn't there, which I presume is to add another filter above the CORE_IN and CORE filters so that they are never called. That will work just fine with this patch in place. Ryan -- Ryan Bloom [EMAIL PROTECTED] 645 Howard St. [EMAIL PROTECTED] San Francisco, CA > -Original Message- > From: Bill Stoddard [mailto:[EMAIL PROTECTED]] > Sent: Sunday, January 27, 2002 12:55 PM > To: [EMAIL PROTECTED]; [EMAIL PROTECTED] > Subject: Re: cvs commit: httpd-2.0/server/mpm/worker worker.c > > Ryan, you are vetoing my veto. Consider this patch a veto of your 11/12 > patch. I am open > to suggestions to determine a compromise. > > I need to replace core_in and core_out based on configuration. How can I > do that with your > patch in place? > > Bill > > > - Original Message - > From: "Ryan Bloom" <[EMAIL PROTECTED]> > To: <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]> > Sent: Sunday, January 27, 2002 3:05 PM > Subject: RE: cvs commit: httpd-2.0/server/mpm/worker worker.c > > > > > stoddard02/01/27 04:52:08 > > > > > > Modified:.CHANGES > > >include http_connection.h httpd.h > > >modules/http http_core.c > > >modules/proxy proxy_ftp.c proxy_http.c > > >server connection.c core.c > > >server/mpm/beos beos.c > > >server/mpm/mpmt_os2 mpmt_os2_child.c > > >server/mpm/netware mpm_netware.c > > >server/mpm/perchild perchild.c > > >server/mpm/prefork prefork.c > > >server/mpm/winnt mpm_winnt.c > > >server/mpm/worker worker.c > > > Log: > > > Remove the create_connection hook and put the client_socket back > > into > > > the > > > conn_rec. The create_connection_hook has a design flaw that prevents > > it > > > from making decisions based on vhost information. > > > > -1. You can't back out a patch because it doesn't do something it > > wasn't designed to do! That hook was meant to allow different filters > > to be added based on the connection used, not based on the > > configuration. If you want to do something based on the configuration, > > you have to wait until the request has been read. Removing the socket > > from the conn_rec has all sorts of advantages, not the least being that > > it keeps people from using the socket without going through filters. > > > > Ryan > > > > > >
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Ryan, you are vetoing my veto. Consider this patch a veto of your 11/12 patch. I am open to suggestions to determine a compromise. I need to replace core_in and core_out based on configuration. How can I do that with your patch in place? Bill - Original Message - From: "Ryan Bloom" <[EMAIL PROTECTED]> To: <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]> Sent: Sunday, January 27, 2002 3:05 PM Subject: RE: cvs commit: httpd-2.0/server/mpm/worker worker.c > > stoddard02/01/27 04:52:08 > > > > Modified:.CHANGES > >include http_connection.h httpd.h > >modules/http http_core.c > >modules/proxy proxy_ftp.c proxy_http.c > >server connection.c core.c > >server/mpm/beos beos.c > >server/mpm/mpmt_os2 mpmt_os2_child.c > >server/mpm/netware mpm_netware.c > >server/mpm/perchild perchild.c > >server/mpm/prefork prefork.c > >server/mpm/winnt mpm_winnt.c > >server/mpm/worker worker.c > > Log: > > Remove the create_connection hook and put the client_socket back > into > > the > > conn_rec. The create_connection_hook has a design flaw that prevents > it > > from making decisions based on vhost information. > > -1. You can't back out a patch because it doesn't do something it > wasn't designed to do! That hook was meant to allow different filters > to be added based on the connection used, not based on the > configuration. If you want to do something based on the configuration, > you have to wait until the request has been read. Removing the socket > from the conn_rec has all sorts of advantages, not the least being that > it keeps people from using the socket without going through filters. > > Ryan > > >
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
> stoddard02/01/27 04:52:08 > > Modified:.CHANGES >include http_connection.h httpd.h >modules/http http_core.c >modules/proxy proxy_ftp.c proxy_http.c >server connection.c core.c >server/mpm/beos beos.c >server/mpm/mpmt_os2 mpmt_os2_child.c >server/mpm/netware mpm_netware.c >server/mpm/perchild perchild.c >server/mpm/prefork prefork.c >server/mpm/winnt mpm_winnt.c >server/mpm/worker worker.c > Log: > Remove the create_connection hook and put the client_socket back into > the > conn_rec. The create_connection_hook has a design flaw that prevents it > from making decisions based on vhost information. -1. You can't back out a patch because it doesn't do something it wasn't designed to do! That hook was meant to allow different filters to be added based on the connection used, not based on the configuration. If you want to do something based on the configuration, you have to wait until the request has been read. Removing the socket from the conn_rec has all sorts of advantages, not the least being that it keeps people from using the socket without going through filters. Ryan
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Is this the same issue I just reported with: "Subject: [HEAD] --with-mpm=worker under FreeBSD 4.5 does nothing?" If so, then there is another bug in there somewhere, since I just checked, and I'm running v1.60 of worker.c: revision 1.60 date: 2002/01/11 08:01:11; author: brianp; state: Exp; lines: +1 -1 Fix for a segfault in the worker MPM during graceful shutdown: The per-transaction pools in the worker MPM can't be children of the listener thread's pool, because that pool may go out of scope while some workers are still procesing requests using the transaction pools. jail# cvs status worker.c === File: worker.c Status: Up-to-date Working revision:1.60 Repository revision: 1.60/home/cvspublic/httpd-2.0/server/mpm/worker/worker.c,v Sticky Tag: (none) Sticky Date: (none) Sticky Options: (none) On Fri, 11 Jan 2002, David Reid wrote: > This also fixes the shutdown issue with worker MPM on FreeBSD which is good > :) > > david > > > > brianp 02/01/11 00:01:11 > > > > Modified:.CHANGES > >server/mpm/worker worker.c > > Log: > > Fix for a segfault in the worker MPM during graceful shutdown: > > The per-transaction pools in the worker MPM can't be children of > > the listener thread's pool, because that pool may go out of scope > > while some workers are still procesing requests using the transaction > > pools. > > > > >
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
This also fixes the shutdown issue with worker MPM on FreeBSD which is good :) david > brianp 02/01/11 00:01:11 > > Modified:.CHANGES >server/mpm/worker worker.c > Log: > Fix for a segfault in the worker MPM during graceful shutdown: > The per-transaction pools in the worker MPM can't be children of > the listener thread's pool, because that pool may go out of scope > while some workers are still procesing requests using the transaction > pools. >
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
[EMAIL PROTECTED] writes: > brianp 02/01/11 00:01:11 > > Modified:.CHANGES >server/mpm/worker worker.c > Log: > Fix for a segfault in the worker MPM during graceful shutdown: > The per-transaction pools in the worker MPM can't be children of > the listener thread's pool, because that pool may go out of scope > while some workers are still procesing requests using the transaction > pools. Thanks! I can no longer hit the graceful restart segfault on either AIX or Solaris. -- Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site: http://www.geocities.com/SiliconValley/Park/9289/ Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
[EMAIL PROTECTED] wrote: >brianp 02/01/11 00:01:11 > > Modified:.CHANGES > server/mpm/worker worker.c > Log: > Fix for a segfault in the worker MPM during graceful shutdown: > The per-transaction pools in the worker MPM can't be children of > the listener thread's pool, because that pool may go out of scope > while some workers are still procesing requests using the transaction > pools. > This should fix both segfaults that Jeff reported on Solaris: the graceful shutdown one, and the out-of-file-descriptors one. --Brian
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Aaron updated the STATUS file already to remove the showstopper --Brian David Reid wrote: >Brian, > >Can you check and remove the showstopper if it solves the problem? > >david > > >>aaron 01/12/27 09:06:40 >> >> Modified:server/mpm/worker worker.c >> Log: >> Take advantage of the new usable apr_thread_exit(). >> >> > >
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Brian, Can you check and remove the showstopper if it solves the problem? david > aaron 01/12/27 09:06:40 > > Modified:server/mpm/worker worker.c > Log: > Take advantage of the new usable apr_thread_exit(). >
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Ugh! I've been meaning to patch the prototype for apr_thread_exit for a long time (so that it takes an apr_status_t instead of a pointer), but it got lost in the shuffle. I'll see if I can fix this sometime this week so we don't have to have ackward &rv stuff like this. -aaron On Sun, Dec 23, 2001 at 01:56:49PM -, [EMAIL PROTECTED] wrote: > dreid 01/12/23 05:56:49 > > Modified:server/mpm/worker worker.c > Log: > This fixes a segfault that showed up on BeOS and may catch other systems. > > Revision ChangesPath > 1.51 +4 -2 httpd-2.0/server/mpm/worker/worker.c > > Index: worker.c > === > RCS file: /home/cvs/httpd-2.0/server/mpm/worker/worker.c,v > retrieving revision 1.50 > retrieving revision 1.51 > diff -u -r1.50 -r1.51 > --- worker.c2001/12/19 14:49:22 1.50 > +++ worker.c2001/12/23 13:56:49 1.51 > @@ -790,7 +790,8 @@ >worker_thread_count--; >apr_thread_mutex_unlock(worker_thread_count_mutex); > > -apr_thread_exit(thd, APR_SUCCESS); > +rv = APR_SUCCESS; > +apr_thread_exit(thd, &rv); >return NULL; >} > > @@ -879,7 +880,8 @@ > * "life_status" is almost right, but it's in the worker's structure, and > * the name could be clearer. gla > */ > -apr_thread_exit(thd, APR_SUCCESS); > +rv = APR_SUCCESS; > +apr_thread_exit(thd, &rv); >return NULL; >} > > > >
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Sat, Dec 08, 2001 at 01:38:05AM -, [EMAIL PROTECTED] wrote: > jerenkrantz01/12/07 17:38:05 > > Modified:.STATUS CHANGES >server listen.c >server/mpm/worker worker.c > Log: > Fix segfault when restarting worker MPM. We can not examine the POD as > a normal listener. This works for me. Please test it out. The problem was in two places - make_pipe_of_death didn't 0 out bind_addr and we didn't check for a null bind_addr in alloc_listener. /me goes off to update all of my httpd machines. -- justin
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
> > --- perchild.c2001/11/10 18:26:29 1.82 > > +++ perchild.c2001/11/12 23:49:07 1.83 > > @@ -502,7 +502,7 @@ > >ap_sock_disable_nagle(sock); > >} > > > > -current_conn = ap_new_connection(p, ap_server_conf, sock, > > conn_id); +current_conn = ap_run_create_connection(p, ap_server_conf, > > sock, conn_id); > > There is an extra parameter there, but I think it should stay and the rest > should change :-) I'm getting to the rest of the comments in this e-mail slowly, but I did the above. I could go either way, so rather than argue, I just did it. :-) > Otherwise... a good move. Can we toss the pre_connection stuff? Which pre_connection stuff? All of it, like that entire hook. Probably. Doing that work after the accept makes more sense in my mind. Ryan __ Ryan Bloom [EMAIL PROTECTED] Covalent Technologies [EMAIL PROTECTED] --
