Re: ssl_die() and pool cleanup
On Wed, Nov 20, 2013 at 5:19 AM, Kaspar Brand httpd-dev.2...@velox.chwrote: On 18.11.2013 14:59, Jeff Trawick wrote: Has anyone looked at making ssl_die() clean up pools on the way out (presumably by calling some function besides exit())? It is rather easy to end up with a bunch of stranded IPC objects while debugging your SSL config. Oh yes, a major annoyance I'm also occasionally running into. * XXX: The config hooks should return errors instead of calling exit(). Gave it a try, see attachment. Not yet extensively tested (*), so perhaps incomplete. But httpd now properly cleans up for me, e.g. when a SIGHUPing fails, as shown in this log extract: [Wed Nov 20 11:02:14.304528 2013] [mpm_worker:notice] [pid 23918:tid 3214934016] AH00298: SIGHUP received. Attempting to restart [Wed Nov 20 11:02:14.544660 2013] [ssl:info] [pid 23918:tid 3214934016] AH02200: Loading certificate private key of SSL-aware server 'server:443' [Wed Nov 20 11:02:14.545137 2013] [ssl:emerg] [pid 23918:tid 3214934016] AH02241: Init: Unable to read server certificate from file /tmp/snakeoil.pem [Wed Nov 20 11:02:14.545240 2013] [ssl:emerg] [pid 23918:tid 3214934016] SSL Library Error: error:0D06B08E:asn1 encoding routines:ASN1_D2I_READ_BIO:not enough data [Wed Nov 20 11:02:14.545278 2013] [ssl:emerg] [pid 23918:tid 3214934016] AH02312: Fatal error initialising mod_ssl, exiting. [Wed Nov 20 11:02:14.545310 2013] [:emerg] [pid 23918:tid 3214934016] AH00020: Configuration Failed, exiting (Configuration Failed, exiting is the key here - this comes from main.c and will call destroy_and_exit_process() to clean up.) Kaspar (*) The changes related to ssl_read_pkcs7 in particular are fairly superficial, but I think we should drop that PKCS#7 stuff from mod_ssl anyway. This is what I found: The two calls to ssl_init_ctx() (engine_init) need to be checked for rv != APR_SUCCESS. The various calls to ssl_server_import_cert() in ssl_init_server_certs() need different rc checking than before. (Now ssl_server_import_cert() can return a fatal error instead of just a boolean.) (same for ssl_server_import_key()) The call to ssl_init_server_check() (engine_init) needs to be checked for rv != APR_SUCCESS. call to ssl_init_ctx_protocol() also needs a check same for ssl_init_ticket_key() It looks like some errors in the proxy config that previously were ignored now cause startup failures... (shrug) Not bad for boiling the ocean :) -- Born in Roswell... married an alien... http://emptyhammock.com/
Re: UDS Patch
Any luck with generating the diff yet? On Nov 19, 2013, at 3:08 PM, Jim Jagielski j...@jagunet.com wrote: The main thing is that it would be interesting to see the diffs between '2.4.6 w the (several) originally proposed UDS patches applied in order' and '2.4.6 w proposed backport'... Those diffs should show just the differences between the UDS implementations... On Nov 19, 2013, at 2:51 PM, Daniel Ruggeri drugg...@primary.net wrote: Yes, agreed. Not sure if I made it clear, but I did apply r1511313 for the tests I did today (but not the one from yesterday). Of the several emails sent, the following have been tested: 2.4.6 w the (several) originally proposed UDS patches applied in order 2.4.6 w proposed backport (the 2 chunks around the DNS changes fail to apply since they do not exist in 2.4.6) 2.4.6 w r1511313 + proposed backport + r1543174 I DID double check that the machine wasn't requesting DNS lookups for the socket name or anything strange against the DNS server - but that was only for the test I ran today. -- Daniel Ruggeri On 11/19/2013 1:43 PM, Jim Jagielski wrote: OK... the DNS lookup code seems to have changed between 2.4.6 and 2.4.7: https://svn.apache.org/viewvc?view=revisionrevision=1511313 So I'm wondering if there's something there.
[RESULT] Re: [VOTE] Release Apache httpd 2.4.7 as GA
With +1 votes from jim,covener,trawick,gsmith,breser,noel.butler,hiding,jblond and h.reindl and NO -1 votes, I call the voting closed with the result of Releasing 2.4.7 as GA. Thx to all testers and voters! Will push to mirrors for announcement and release next week. On Nov 19, 2013, at 12:45 PM, Jim Jagielski j...@jagunet.com wrote: The pre-release test tarballs for Apache httpd 2.4.7 can be found at the usual place: http://httpd.apache.org/dev/dist/ I'm calling a VOTE on releasing these as Apache httpd 2.4.7 GA. [ ] +1: Good to go [ ] +0: meh [ ] -1: Danger Will Robinson. And why. Vote will last the normal 72 hrs. NOTE: The *-deps are only there for convenience.
Re: ssl_die() and pool cleanup
Maybe sk_X509_NAME_pop_free(ca_list, X509_NAME_free) should be called too before returning NULL in ssl_init_FindCAList() (so to avoid a leak in case of failure). Regards; Yann. On Fri, Nov 22, 2013 at 4:20 PM, Jeff Trawick traw...@gmail.com wrote: On Wed, Nov 20, 2013 at 5:19 AM, Kaspar Brand httpd-dev.2...@velox.chwrote: On 18.11.2013 14:59, Jeff Trawick wrote: Has anyone looked at making ssl_die() clean up pools on the way out (presumably by calling some function besides exit())? It is rather easy to end up with a bunch of stranded IPC objects while debugging your SSL config. Oh yes, a major annoyance I'm also occasionally running into. * XXX: The config hooks should return errors instead of calling exit(). Gave it a try, see attachment. Not yet extensively tested (*), so perhaps incomplete. But httpd now properly cleans up for me, e.g. when a SIGHUPing fails, as shown in this log extract: [Wed Nov 20 11:02:14.304528 2013] [mpm_worker:notice] [pid 23918:tid 3214934016] AH00298: SIGHUP received. Attempting to restart [Wed Nov 20 11:02:14.544660 2013] [ssl:info] [pid 23918:tid 3214934016] AH02200: Loading certificate private key of SSL-aware server 'server:443' [Wed Nov 20 11:02:14.545137 2013] [ssl:emerg] [pid 23918:tid 3214934016] AH02241: Init: Unable to read server certificate from file /tmp/snakeoil.pem [Wed Nov 20 11:02:14.545240 2013] [ssl:emerg] [pid 23918:tid 3214934016] SSL Library Error: error:0D06B08E:asn1 encoding routines:ASN1_D2I_READ_BIO:not enough data [Wed Nov 20 11:02:14.545278 2013] [ssl:emerg] [pid 23918:tid 3214934016] AH02312: Fatal error initialising mod_ssl, exiting. [Wed Nov 20 11:02:14.545310 2013] [:emerg] [pid 23918:tid 3214934016] AH00020: Configuration Failed, exiting (Configuration Failed, exiting is the key here - this comes from main.c and will call destroy_and_exit_process() to clean up.) Kaspar (*) The changes related to ssl_read_pkcs7 in particular are fairly superficial, but I think we should drop that PKCS#7 stuff from mod_ssl anyway. This is what I found: The two calls to ssl_init_ctx() (engine_init) need to be checked for rv != APR_SUCCESS. The various calls to ssl_server_import_cert() in ssl_init_server_certs() need different rc checking than before. (Now ssl_server_import_cert() can return a fatal error instead of just a boolean.) (same for ssl_server_import_key()) The call to ssl_init_server_check() (engine_init) needs to be checked for rv != APR_SUCCESS. call to ssl_init_ctx_protocol() also needs a check same for ssl_init_ticket_key() It looks like some errors in the proxy config that previously were ignored now cause startup failures... (shrug) Not bad for boiling the ocean :) -- Born in Roswell... married an alien... http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem rpl...@apache.org wrote:
j...@apache.org wrote:
Author: jim
Date: Fri Nov 16 16:49:31 2012
New Revision: 1410459
URL: http://svn.apache.org/viewvc?rev=1410459view=rev
Log:
fdq expects a certain behavior from atomics... ensure that
the event mpms check this.
Modified:
httpd/httpd/trunk/docs/log-message-tags/next-number
httpd/httpd/trunk/server/mpm/event/event.c
httpd/httpd/trunk/server/mpm/eventopt/eventopt.c
Modified: httpd/httpd/trunk/server/mpm/event/event.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1410459r1=1410458r2=1410459view=diff
==
--- httpd/httpd/trunk/server/mpm/event/event.c (original)
+++ httpd/httpd/trunk/server/mpm/event/event.c Fri Nov 16 16:49:31 2012
@@ -2928,6 +2928,18 @@ static int event_pre_config(apr_pool_t *
}
++retained-module_loads;
if (retained-module_loads == 2) {
+int i;
+static apr_uint32_t foo = 0;
+
+apr_atomic_inc32(foo);
+apr_atomic_dec32(foo);
+apr_atomic_dec32(foo);
+i = apr_atomic_dec32(foo);
+if (i = 0) {
Why can we expect i 0? apr_atomic_dec32 returns 0 if the dec causes foo
to become zero and it returns non zero
otherwise. Shouldn't this behavior the same across all platforms? And if
not should that be fixed in APR?
icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb here.
--enable-nonportable-atomics is specified for apr, though I haven't checked
what that does with icc.
+ap_log_error(APLOG_MARK, APLOG_CRIT, rv, NULL,
APLOGNO(02405)
+ atomics not working as expected);
+return HTTP_INTERNAL_SERVER_ERROR;
+}
rv = apr_pollset_create(event_pollset, 1, plog,
APR_POLLSET_THREADSAFE |
APR_POLLSET_NOCOPY);
if (rv != APR_SUCCESS) {
Regards
Rüdiger
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: [VOTE] Release Apache httpd 2.4.7 as GA
On Wed, Nov 20, 2013 at 2:06 PM, Jeff Trawick traw...@gmail.com wrote: On Tue, Nov 19, 2013 at 12:45 PM, Jim Jagielski j...@jagunet.com wrote: The pre-release test tarballs for Apache httpd 2.4.7 can be found at the usual place: http://httpd.apache.org/dev/dist/ I'm calling a VOTE on releasing these as Apache httpd 2.4.7 GA. [ ] +1: Good to go [ ] +0: meh [ ] -1: Danger Will Robinson. And why. [X] +1: Good to go Linux RHEL5-ish: httpd 2.4.7+apr-1.5.0+apr-util-1.5.3+mod_perl httpd24threading branch (current)+latest libxml2/pcre/zlib/other modules **but no mod_ssl** passes httpd test suite passes the same mod_perl tests that passed with httpd 2.4.6 I forgot to try this with icc at the time :( Event no longer works with httpd and apr built with icc. (See r1410459 commit thread.) More digging on my part is needed to find out which atomic code gets used. Win2008 R2: same combination of code Visual Studio 2012 64-bit (cmake NMake Makefiles generator) passes the same httpd tests that passed with 2.4.6 (running all testcases cleanly on Windows is not currently possible) not able to try mod_perl test suite in this environment currently -- Born in Roswell... married an alien... http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
On Nov 22, 2013, at 2:22 PM, Jeff Trawick traw...@gmail.com wrote:
On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem rpl...@apache.org wrote:
j...@apache.org wrote:
+i = apr_atomic_dec32(foo);
+if (i = 0) {
Why can we expect i 0? apr_atomic_dec32 returns 0 if the dec causes foo to
become zero and it returns non zero
otherwise. Shouldn't this behavior the same across all platforms? And if not
should that be fixed in APR?
icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb here.
--enable-nonportable-atomics is specified for apr, though I haven't checked
what that does with icc.
As noted back with the orig update, this test is due to the
fdqueue code in the new event:
apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info,
apr_pool_t * pool_to_recycle)
{
apr_status_t rv;
int prev_idlers;
ap_push_pool(queue_info, pool_to_recycle);
/* Atomically increment the count of idle workers */
/*
* TODO: The atomics expect unsigned whereas we're using signed.
* Need to double check that they work as expected or else
* rework how we determine blocked.
* UPDATE: Correct operation is performed during open_logs()
*/
prev_idlers = apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));
/* If other threads are waiting on a worker, wake one up */
if (prev_idlers 0) {
See the comments (The atomics expect unsigned whereas...) for
the reason, etc.
When you say icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb
here.
do you mean that you get the 'atomics not working as expected' error
(and the internal server error) or that it core dumps?
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski j...@jagunet.com wrote:
On Nov 22, 2013, at 2:22 PM, Jeff Trawick traw...@gmail.com wrote:
On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem rpl...@apache.org
wrote:
j...@apache.org wrote:
+i = apr_atomic_dec32(foo);
+if (i = 0) {
Why can we expect i 0? apr_atomic_dec32 returns 0 if the dec causes
foo to become zero and it returns non zero
otherwise. Shouldn't this behavior the same across all platforms? And if
not should that be fixed in APR?
icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb here.
--enable-nonportable-atomics is specified for apr, though I haven't
checked what that does with icc.
As noted back with the orig update, this test is due to the
fdqueue code in the new event:
apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info,
apr_pool_t * pool_to_recycle)
{
apr_status_t rv;
int prev_idlers;
ap_push_pool(queue_info, pool_to_recycle);
/* Atomically increment the count of idle workers */
/*
* TODO: The atomics expect unsigned whereas we're using signed.
* Need to double check that they work as expected or else
* rework how we determine blocked.
* UPDATE: Correct operation is performed during open_logs()
*/
prev_idlers = apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));
/* If other threads are waiting on a worker, wake one up */
if (prev_idlers 0) {
See the comments (The atomics expect unsigned whereas...) for
the reason, etc.
When you say icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0)
bomb here.
do you mean that you get the 'atomics not working as expected' error
(and the internal server error) or that it core dumps?
atomics not working as expected
Let me see what code is used...
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: [VOTE] Release Apache httpd 2.4.7 as GA
Note, the only think changed in event now (via https://svn.apache.org/viewvc?view=revisionrevision=1542560) is that event *checks* that atomics work as required for event... if the check fails, it means that event has been broken on that system, assuming it ever hit blocked idlers, for a *long* time. On Nov 22, 2013, at 2:27 PM, Jeff Trawick traw...@gmail.com wrote: On Wed, Nov 20, 2013 at 2:06 PM, Jeff Trawick traw...@gmail.com wrote: On Tue, Nov 19, 2013 at 12:45 PM, Jim Jagielski j...@jagunet.com wrote: The pre-release test tarballs for Apache httpd 2.4.7 can be found at the usual place: http://httpd.apache.org/dev/dist/ I'm calling a VOTE on releasing these as Apache httpd 2.4.7 GA. [ ] +1: Good to go [ ] +0: meh [ ] -1: Danger Will Robinson. And why. [X] +1: Good to go Linux RHEL5-ish: httpd 2.4.7+apr-1.5.0+apr-util-1.5.3+mod_perl httpd24threading branch (current)+latest libxml2/pcre/zlib/other modules **but no mod_ssl** passes httpd test suite passes the same mod_perl tests that passed with httpd 2.4.6 I forgot to try this with icc at the time :( Event no longer works with httpd and apr built with icc. (See r1410459 commit thread.) More digging on my part is needed to find out which atomic code gets used. Win2008 R2: same combination of code Visual Studio 2012 64-bit (cmake NMake Makefiles generator) passes the same httpd tests that passed with 2.4.6 (running all testcases cleanly on Windows is not currently possible) not able to try mod_perl test suite in this environment currently -- Born in Roswell... married an alien... http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Note, the only think changed in event now (via
https://svn.apache.org/viewvc?view=revisionrevision=1542560)
is that event *checks* that atomics work as required for
event... if the check fails, it means that event has
been broken on that system, assuming it ever hit
blocked idlers, for a *long* time...
You should be seeing it in trunk as well...
On Nov 22, 2013, at 2:43 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski j...@jagunet.com wrote:
On Nov 22, 2013, at 2:22 PM, Jeff Trawick traw...@gmail.com wrote:
On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem rpl...@apache.org wrote:
j...@apache.org wrote:
+i = apr_atomic_dec32(foo);
+if (i = 0) {
Why can we expect i 0? apr_atomic_dec32 returns 0 if the dec causes foo
to become zero and it returns non zero
otherwise. Shouldn't this behavior the same across all platforms? And if
not should that be fixed in APR?
icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb here.
--enable-nonportable-atomics is specified for apr, though I haven't checked
what that does with icc.
As noted back with the orig update, this test is due to the
fdqueue code in the new event:
apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info,
apr_pool_t * pool_to_recycle)
{
apr_status_t rv;
int prev_idlers;
ap_push_pool(queue_info, pool_to_recycle);
/* Atomically increment the count of idle workers */
/*
* TODO: The atomics expect unsigned whereas we're using signed.
* Need to double check that they work as expected or else
* rework how we determine blocked.
* UPDATE: Correct operation is performed during open_logs()
*/
prev_idlers = apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));
/* If other threads are waiting on a worker, wake one up */
if (prev_idlers 0) {
See the comments (The atomics expect unsigned whereas...) for
the reason, etc.
When you say icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0)
bomb here.
do you mean that you get the 'atomics not working as expected' error
(and the internal server error) or that it core dumps?
atomics not working as expected
Let me see what code is used...
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski j...@jagunet.com wrote:
Note, the only think changed in event now (via
https://svn.apache.org/viewvc?view=revisionrevision=1542560)
is that event *checks* that atomics work as required for
event... if the check fails, it means that event has
been broken on that system, assuming it ever hit
blocked idlers, for a *long* time...
Got it... fdqueue.c is asking for trouble...
I'm using atomic/unix/ia32.c with icc too.
Need to compare generated code... I hate stuff like int foo() { unsigned
char x; ... return x; }
You should be seeing it in trunk as well...
On Nov 22, 2013, at 2:43 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski j...@jagunet.com wrote:
On Nov 22, 2013, at 2:22 PM, Jeff Trawick traw...@gmail.com wrote:
On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem rpl...@apache.org
wrote:
j...@apache.org wrote:
+i = apr_atomic_dec32(foo);
+if (i = 0) {
Why can we expect i 0? apr_atomic_dec32 returns 0 if the dec causes
foo to become zero and it returns non zero
otherwise. Shouldn't this behavior the same across all platforms? And
if not should that be fixed in APR?
icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb here.
--enable-nonportable-atomics is specified for apr, though I haven't
checked what that does with icc.
As noted back with the orig update, this test is due to the
fdqueue code in the new event:
apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info,
apr_pool_t * pool_to_recycle)
{
apr_status_t rv;
int prev_idlers;
ap_push_pool(queue_info, pool_to_recycle);
/* Atomically increment the count of idle workers */
/*
* TODO: The atomics expect unsigned whereas we're using signed.
* Need to double check that they work as expected or else
* rework how we determine blocked.
* UPDATE: Correct operation is performed during open_logs()
*/
prev_idlers = apr_atomic_inc32((apr_uint32_t
*)(queue_info-idlers));
/* If other threads are waiting on a worker, wake one up */
if (prev_idlers 0) {
See the comments (The atomics expect unsigned whereas...) for
the reason, etc.
When you say icc (Intel) builds of httpd 2.4.7 event MPM (with
apr-1.5.0) bomb here.
do you mean that you get the 'atomics not working as expected' error
(and the internal server error) or that it core dumps?
atomics not working as expected
Let me see what code is used...
--
Born in Roswell... married an alien...
http://emptyhammock.com/
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
On Nov 22, 2013, at 3:24 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski j...@jagunet.com wrote:
Note, the only think changed in event now (via
https://svn.apache.org/viewvc?view=revisionrevision=1542560)
is that event *checks* that atomics work as required for
event... if the check fails, it means that event has
been broken on that system, assuming it ever hit
blocked idlers, for a *long* time...
Got it... fdqueue.c is asking for trouble...
Yeah.
I'm using atomic/unix/ia32.c with icc too.
Need to compare generated code... I hate stuff like int foo() { unsigned
char x; ... return x; }
You should be seeing it in trunk as well...
On Nov 22, 2013, at 2:43 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski j...@jagunet.com wrote:
On Nov 22, 2013, at 2:22 PM, Jeff Trawick traw...@gmail.com wrote:
On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem rpl...@apache.org wrote:
j...@apache.org wrote:
+i = apr_atomic_dec32(foo);
+if (i = 0) {
Why can we expect i 0? apr_atomic_dec32 returns 0 if the dec causes foo
to become zero and it returns non zero
otherwise. Shouldn't this behavior the same across all platforms? And if
not should that be fixed in APR?
icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb here.
--enable-nonportable-atomics is specified for apr, though I haven't
checked what that does with icc.
As noted back with the orig update, this test is due to the
fdqueue code in the new event:
apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info,
apr_pool_t * pool_to_recycle)
{
apr_status_t rv;
int prev_idlers;
ap_push_pool(queue_info, pool_to_recycle);
/* Atomically increment the count of idle workers */
/*
* TODO: The atomics expect unsigned whereas we're using signed.
* Need to double check that they work as expected or else
* rework how we determine blocked.
* UPDATE: Correct operation is performed during open_logs()
*/
prev_idlers = apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));
/* If other threads are waiting on a worker, wake one up */
if (prev_idlers 0) {
See the comments (The atomics expect unsigned whereas...) for
the reason, etc.
When you say icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0)
bomb here.
do you mean that you get the 'atomics not working as expected' error
(and the internal server error) or that it core dumps?
atomics not working as expected
Let me see what code is used...
--
Born in Roswell... married an alien...
http://emptyhammock.com/
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski j...@jagunet.com wrote:
Note, the only think changed in event now (via
https://svn.apache.org/viewvc?view=revisionrevision=1542560)
is that event *checks* that atomics work as required for
event... if the check fails, it means that event has
been broken on that system, assuming it ever hit
blocked idlers, for a *long* time...
Got it... fdqueue.c is asking for trouble...
I'm using atomic/unix/ia32.c with icc too.
Need to compare generated code... I hate stuff like int foo() { unsigned
char x; ... return x; }
APR is documented as returning zero if the value becomes zero on
decrement, otherwise non-zero.
With gcc we use get __sync_sub_and_fetch(), which returns the new value
after the decrement.
With icc we use the assembler implementation in APR. I think that's
returning 1 instead of -1.
Here is where fdqueue needs to be able to see a negative return code:
apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
{
int prev_idlers;
prev_idlers = apr_atomic_dec32((apr_uint32_t *)(queue_info-idlers));
if (prev_idlers = 0) {
apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));/* back
out dec */
return APR_EAGAIN;
}
return APR_SUCCESS;
}
(prev in the ia32.c version of apr_atomic_dec32() and prev_idlers here
is deceiving.)
You should be seeing it in trunk as well...
On Nov 22, 2013, at 2:43 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski j...@jagunet.com wrote:
On Nov 22, 2013, at 2:22 PM, Jeff Trawick traw...@gmail.com wrote:
On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem rpl...@apache.org
wrote:
j...@apache.org wrote:
+i = apr_atomic_dec32(foo);
+if (i = 0) {
Why can we expect i 0? apr_atomic_dec32 returns 0 if the dec causes
foo to become zero and it returns non zero
otherwise. Shouldn't this behavior the same across all platforms? And
if not should that be fixed in APR?
icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb
here.
--enable-nonportable-atomics is specified for apr, though I haven't
checked what that does with icc.
As noted back with the orig update, this test is due to the
fdqueue code in the new event:
apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info,
apr_pool_t * pool_to_recycle)
{
apr_status_t rv;
int prev_idlers;
ap_push_pool(queue_info, pool_to_recycle);
/* Atomically increment the count of idle workers */
/*
* TODO: The atomics expect unsigned whereas we're using signed.
* Need to double check that they work as expected or else
* rework how we determine blocked.
* UPDATE: Correct operation is performed during open_logs()
*/
prev_idlers = apr_atomic_inc32((apr_uint32_t
*)(queue_info-idlers));
/* If other threads are waiting on a worker, wake one up */
if (prev_idlers 0) {
See the comments (The atomics expect unsigned whereas...) for
the reason, etc.
When you say icc (Intel) builds of httpd 2.4.7 event MPM (with
apr-1.5.0) bomb here.
do you mean that you get the 'atomics not working as expected' error
(and the internal server error) or that it core dumps?
atomics not working as expected
Let me see what code is used...
--
Born in Roswell... married an alien...
http://emptyhammock.com/
--
Born in Roswell... married an alien...
http://emptyhammock.com/
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
On Fri, Nov 22, 2013 at 3:40 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski j...@jagunet.com wrote:
Note, the only think changed in event now (via
https://svn.apache.org/viewvc?view=revisionrevision=1542560)
is that event *checks* that atomics work as required for
event... if the check fails, it means that event has
been broken on that system, assuming it ever hit
blocked idlers, for a *long* time...
Got it... fdqueue.c is asking for trouble...
I'm using atomic/unix/ia32.c with icc too.
Need to compare generated code... I hate stuff like int foo() {
unsigned char x; ... return x; }
APR is documented as returning zero if the value becomes zero on
decrement, otherwise non-zero.
With gcc we use get __sync_sub_and_fetch(), which returns the new value
after the decrement.
With icc we use the assembler implementation in APR. I think that's
returning 1 instead of -1.
It does a decrement long, which sets the zero flag as appropriate. Then it
does set-not-equal to set what becomes the return value to 0 if the result
of the decrement was 0 and 1 otherwise.
It should be easy to make it return the new value like the
__sync_sub_and_fetch() version does :)
Here is where fdqueue needs to be able to see a negative return code:
apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
{
int prev_idlers;
prev_idlers = apr_atomic_dec32((apr_uint32_t *)(queue_info-idlers));
if (prev_idlers = 0) {
apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));/*
back out dec */
return APR_EAGAIN;
}
return APR_SUCCESS;
}
(prev in the ia32.c version of apr_atomic_dec32() and prev_idlers here
is deceiving.)
You should be seeing it in trunk as well...
On Nov 22, 2013, at 2:43 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski j...@jagunet.com
wrote:
On Nov 22, 2013, at 2:22 PM, Jeff Trawick traw...@gmail.com wrote:
On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem rpl...@apache.org
wrote:
j...@apache.org wrote:
+i = apr_atomic_dec32(foo);
+if (i = 0) {
Why can we expect i 0? apr_atomic_dec32 returns 0 if the dec
causes foo to become zero and it returns non zero
otherwise. Shouldn't this behavior the same across all platforms?
And if not should that be fixed in APR?
icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb
here.
--enable-nonportable-atomics is specified for apr, though I haven't
checked what that does with icc.
As noted back with the orig update, this test is due to the
fdqueue code in the new event:
apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info,
apr_pool_t * pool_to_recycle)
{
apr_status_t rv;
int prev_idlers;
ap_push_pool(queue_info, pool_to_recycle);
/* Atomically increment the count of idle workers */
/*
* TODO: The atomics expect unsigned whereas we're using signed.
* Need to double check that they work as expected or else
* rework how we determine blocked.
* UPDATE: Correct operation is performed during open_logs()
*/
prev_idlers = apr_atomic_inc32((apr_uint32_t
*)(queue_info-idlers));
/* If other threads are waiting on a worker, wake one up */
if (prev_idlers 0) {
See the comments (The atomics expect unsigned whereas...) for
the reason, etc.
When you say icc (Intel) builds of httpd 2.4.7 event MPM (with
apr-1.5.0) bomb here.
do you mean that you get the 'atomics not working as expected' error
(and the internal server error) or that it core dumps?
atomics not working as expected
Let me see what code is used...
--
Born in Roswell... married an alien...
http://emptyhammock.com/
--
Born in Roswell... married an alien...
http://emptyhammock.com/
--
Born in Roswell... married an alien...
http://emptyhammock.com/
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
On Fri, Nov 22, 2013 at 3:57 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 3:40 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski j...@jagunet.com wrote:
Note, the only think changed in event now (via
https://svn.apache.org/viewvc?view=revisionrevision=1542560)
is that event *checks* that atomics work as required for
event... if the check fails, it means that event has
been broken on that system, assuming it ever hit
blocked idlers, for a *long* time...
Got it... fdqueue.c is asking for trouble...
I'm using atomic/unix/ia32.c with icc too.
Need to compare generated code... I hate stuff like int foo() {
unsigned char x; ... return x; }
APR is documented as returning zero if the value becomes zero on
decrement, otherwise non-zero.
With gcc we use get __sync_sub_and_fetch(), which returns the new value
after the decrement.
With icc we use the assembler implementation in APR. I think that's
returning 1 instead of -1.
It does a decrement long, which sets the zero flag as appropriate. Then
it does set-not-equal to set what becomes the return value to 0 if the
result of the decrement was 0 and 1 otherwise.
It should be easy to make it return the new value like the
__sync_sub_and_fetch() version does :)
Any Intel assembly gurus out there?
--- apr.orig/atomic/unix/ia32.c 2013-04-21 14:28:47.0 -0700
+++ apr/atomic/unix/ia32.c 2013-11-22 13:16:04.0 -0800
@@ -57,14 +57,14 @@
APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
{
-unsigned char prev;
+apr_uint32_t newvalue;
-asm volatile (lock; decl %0; setnz %1
- : =m (*mem), =qm (prev)
+asm volatile (lock; decl %0; movl %0,%1
+ : =m (*mem), =qm (newvalue)
: m (*mem)
: memory);
-return prev;
+return newvalue;
}
APR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
apr_uint32_t with,
It probably uses a zillion more cycles than the previous version. I wonder
what __sync_sub_and_fetch() does exactly.
Also, maybe stdcxx has an implementation somewhere that matches
__sync_sub_and_fetch() (I see
http://svn.apache.org/repos/asf/stdcxx/branches/4.2.x/include/rw/_atomic-sync.h,
but no time to look now.
Here is where fdqueue needs to be able to see a negative return code:
apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
{
int prev_idlers;
prev_idlers = apr_atomic_dec32((apr_uint32_t *)(queue_info-idlers));
if (prev_idlers = 0) {
apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));/*
back out dec */
return APR_EAGAIN;
}
return APR_SUCCESS;
}
(prev in the ia32.c version of apr_atomic_dec32() and prev_idlers
here is deceiving.)
You should be seeing it in trunk as well...
On Nov 22, 2013, at 2:43 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski j...@jagunet.com
wrote:
On Nov 22, 2013, at 2:22 PM, Jeff Trawick traw...@gmail.com wrote:
On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem rpl...@apache.org
wrote:
j...@apache.org wrote:
+i = apr_atomic_dec32(foo);
+if (i = 0) {
Why can we expect i 0? apr_atomic_dec32 returns 0 if the dec
causes foo to become zero and it returns non zero
otherwise. Shouldn't this behavior the same across all platforms?
And if not should that be fixed in APR?
icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb
here.
--enable-nonportable-atomics is specified for apr, though I haven't
checked what that does with icc.
As noted back with the orig update, this test is due to the
fdqueue code in the new event:
apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info,
apr_pool_t * pool_to_recycle)
{
apr_status_t rv;
int prev_idlers;
ap_push_pool(queue_info, pool_to_recycle);
/* Atomically increment the count of idle workers */
/*
* TODO: The atomics expect unsigned whereas we're using signed.
* Need to double check that they work as expected or else
* rework how we determine blocked.
* UPDATE: Correct operation is performed during open_logs()
*/
prev_idlers = apr_atomic_inc32((apr_uint32_t
*)(queue_info-idlers));
/* If other threads are waiting on a worker, wake one up */
if (prev_idlers 0) {
See the comments (The atomics expect unsigned whereas...) for
the reason, etc.
When you say icc (Intel) builds of httpd 2.4.7 event MPM (with
apr-1.5.0) bomb here.
do you mean that you get the 'atomics not working as expected' error
(and the internal server error) or that it core dumps?
atomics not working as expected
Let me see what code is used...
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
The thing is is not only do we worry about the return code
but also that the values we dec32 and inc32 also behave
as signed ints. Note below we worry also that queue_info-idlers
itself is signed, and can be 0 :
apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
{
int prev_idlers;
prev_idlers = apr_atomic_dec32((apr_uint32_t *)(queue_info-idlers));
if (prev_idlers = 0) {
apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));/* back out
dec */
return APR_EAGAIN;
}
return APR_SUCCESS;
}
apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t * queue_info,
int *had_to_block)
{
apr_status_t rv;
int prev_idlers;
/* Atomically decrement the idle worker count, saving the old value */
/* See TODO in ap_queue_info_set_idle() */
prev_idlers = apr_atomic_add32((apr_uint32_t *)(queue_info-idlers), -1);
/* Block if there weren't any idle workers */
if (prev_idlers = 0) {
rv = apr_thread_mutex_lock(queue_info-idlers_mutex);
if (rv != APR_SUCCESS) {
AP_DEBUG_ASSERT(0);
/* See TODO in ap_queue_info_set_idle() */
apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));/* back
out dec */
return rv;
}
/* Re-check the idle worker count to guard against a
* race condition. Now that we're in the mutex-protected
* region, one of two things may have happened:
* - If the idle worker count is still negative, the
* workers are all still busy, so it's safe to
* block on a condition variable.
* - If the idle worker count is non-negative, then a
* worker has become idle since the first check
* of queue_info-idlers above. It's possible
* that the worker has also signaled the condition
* variable--and if so, the listener missed it
* because it wasn't yet blocked on the condition
* variable. But if the idle worker count is
* now non-negative, it's safe for this function to
* return immediately.
*
* A negative value in queue_info-idlers tells how many
* threads are waiting on an idle worker.
*/
if (queue_info-idlers 0) {
*had_to_block = 1;
rv = apr_thread_cond_wait(queue_info-wait_for_idler,
queue_info-idlers_mutex);
On Nov 22, 2013, at 3:40 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski j...@jagunet.com wrote:
Note, the only think changed in event now (via
https://svn.apache.org/viewvc?view=revisionrevision=1542560)
is that event *checks* that atomics work as required for
event... if the check fails, it means that event has
been broken on that system, assuming it ever hit
blocked idlers, for a *long* time...
Got it... fdqueue.c is asking for trouble...
I'm using atomic/unix/ia32.c with icc too.
Need to compare generated code... I hate stuff like int foo() { unsigned
char x; ... return x; }
APR is documented as returning zero if the value becomes zero on decrement,
otherwise non-zero.
With gcc we use get __sync_sub_and_fetch(), which returns the new value after
the decrement.
With icc we use the assembler implementation in APR. I think that's
returning 1 instead of -1.
Here is where fdqueue needs to be able to see a negative return code:
apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
{
int prev_idlers;
prev_idlers = apr_atomic_dec32((apr_uint32_t *)(queue_info-idlers));
if (prev_idlers = 0) {
apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));/* back
out dec */
return APR_EAGAIN;
}
return APR_SUCCESS;
}
(prev in the ia32.c version of apr_atomic_dec32() and prev_idlers here is
deceiving.)
You should be seeing it in trunk as well...
On Nov 22, 2013, at 2:43 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski j...@jagunet.com wrote:
On Nov 22, 2013, at 2:22 PM, Jeff Trawick traw...@gmail.com wrote:
On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem rpl...@apache.org wrote:
j...@apache.org wrote:
+i = apr_atomic_dec32(foo);
+if (i = 0) {
Why can we expect i 0? apr_atomic_dec32 returns 0 if the dec causes foo
to become zero and it returns non zero
otherwise. Shouldn't this behavior the same across all platforms? And if
not should that be fixed in APR?
icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb here.
--enable-nonportable-atomics is specified for apr, though I haven't
checked what that does with icc.
As noted back with the orig update, this test is
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Anyone ever try OpenPA? https://trac.mcs.anl.gov/projects/openpa/ It's under MIT, fwiw.
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
On Fri, Nov 22, 2013 at 4:27 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 3:57 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 3:40 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski j...@jagunet.com wrote:
Note, the only think changed in event now (via
https://svn.apache.org/viewvc?view=revisionrevision=1542560)
is that event *checks* that atomics work as required for
event... if the check fails, it means that event has
been broken on that system, assuming it ever hit
blocked idlers, for a *long* time...
Got it... fdqueue.c is asking for trouble...
I'm using atomic/unix/ia32.c with icc too.
Need to compare generated code... I hate stuff like int foo() {
unsigned char x; ... return x; }
APR is documented as returning zero if the value becomes zero on
decrement, otherwise non-zero.
With gcc we use get __sync_sub_and_fetch(), which returns the new value
after the decrement.
With icc we use the assembler implementation in APR. I think that's
returning 1 instead of -1.
It does a decrement long, which sets the zero flag as appropriate. Then
it does set-not-equal to set what becomes the return value to 0 if the
result of the decrement was 0 and 1 otherwise.
It should be easy to make it return the new value like the
__sync_sub_and_fetch() version does :)
Any Intel assembly gurus out there?
--- apr.orig/atomic/unix/ia32.c 2013-04-21 14:28:47.0 -0700
+++ apr/atomic/unix/ia32.c 2013-11-22 13:16:04.0 -0800
@@ -57,14 +57,14 @@
APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
{
-unsigned char prev;
+apr_uint32_t newvalue;
-asm volatile (lock; decl %0; setnz %1
- : =m (*mem), =qm (prev)
+asm volatile (lock; decl %0; movl %0,%1
+ : =m (*mem), =qm (newvalue)
: m (*mem)
: memory);
-return prev;
+return newvalue;
}
APR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
apr_uint32_t with,
It probably uses a zillion more cycles than the previous version. I
wonder what __sync_sub_and_fetch() does exactly.
Also, maybe stdcxx has an implementation somewhere that matches
__sync_sub_and_fetch() (I see
http://svn.apache.org/repos/asf/stdcxx/branches/4.2.x/include/rw/_atomic-sync.h,
but no time to look now.
Most (I won't swear exactly which ones ;) ) APR implementations of
apr_atomic_dec32() return the new value. The z/OS code in
atomic/os390/atomic.c needs an obvious fix too. (IIUC, Event MPM should
work there because of recently added pollset support, though I guess a few
lines in configure and event need to be patched to recognize the new
pollset implementation as usable by event.)
Here is where fdqueue needs to be able to see a negative return code:
apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
{
int prev_idlers;
prev_idlers = apr_atomic_dec32((apr_uint32_t
*)(queue_info-idlers));
if (prev_idlers = 0) {
apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));/*
back out dec */
return APR_EAGAIN;
}
return APR_SUCCESS;
}
(prev in the ia32.c version of apr_atomic_dec32() and prev_idlers
here is deceiving.)
You should be seeing it in trunk as well...
On Nov 22, 2013, at 2:43 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski j...@jagunet.com
wrote:
On Nov 22, 2013, at 2:22 PM, Jeff Trawick traw...@gmail.com wrote:
On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem rpl...@apache.org
wrote:
j...@apache.org wrote:
+i = apr_atomic_dec32(foo);
+if (i = 0) {
Why can we expect i 0? apr_atomic_dec32 returns 0 if the dec
causes foo to become zero and it returns non zero
otherwise. Shouldn't this behavior the same across all platforms?
And if not should that be fixed in APR?
icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb
here.
--enable-nonportable-atomics is specified for apr, though I
haven't checked what that does with icc.
As noted back with the orig update, this test is due to the
fdqueue code in the new event:
apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info,
apr_pool_t * pool_to_recycle)
{
apr_status_t rv;
int prev_idlers;
ap_push_pool(queue_info, pool_to_recycle);
/* Atomically increment the count of idle workers */
/*
* TODO: The atomics expect unsigned whereas we're using signed.
* Need to double check that they work as expected or else
* rework how we determine blocked.
* UPDATE: Correct operation is performed during open_logs()
*/
prev_idlers = apr_atomic_inc32((apr_uint32_t
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
On Fri, Nov 22, 2013 at 4:32 PM, Jim Jagielski j...@jagunet.com wrote:
The thing is is not only do we worry about the return code
but also that the values we dec32 and inc32 also behave
as signed ints. Note below we worry also that queue_info-idlers
itself is signed, and can be 0 :
Okay gurus, tell me where I'm confused (maybe I'm trainable):
I guess the opportunity for failure is if one of the APR implementations of
apr_atomic_dec32() is setting the return value in C code. But doing that
(i.e., not having the assembly code set a local variable which is returned)
is uncommon.
Imagine that this existing z/OS code is changed to simply return new_val.
Conversion from 32-bit unsigned int to 32-bit signed int is undefined IIUC
and in the unlikely case that the compiler tried to change the bits some
trick would be needed in the code.
int apr_atomic_dec32(volatile apr_uint32_t *mem)
{
apr_uint32_t old, new_val;
old = *mem; /* old is automatically updated on cs failure */
do {
new_val = old - 1;
} while (__cs(old, (cs_t *)mem, new_val));
return new_val != 0;
}
apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
{
int prev_idlers;
prev_idlers = apr_atomic_dec32((apr_uint32_t *)(queue_info-idlers));
if (prev_idlers = 0) {
apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));/*
back out dec */
return APR_EAGAIN;
}
return APR_SUCCESS;
}
apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t * queue_info,
int *had_to_block)
{
apr_status_t rv;
int prev_idlers;
/* Atomically decrement the idle worker count, saving the old value */
/* See TODO in ap_queue_info_set_idle() */
prev_idlers = apr_atomic_add32((apr_uint32_t *)(queue_info-idlers),
-1);
/* Block if there weren't any idle workers */
if (prev_idlers = 0) {
rv = apr_thread_mutex_lock(queue_info-idlers_mutex);
if (rv != APR_SUCCESS) {
AP_DEBUG_ASSERT(0);
/* See TODO in ap_queue_info_set_idle() */
apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));/*
back out dec */
return rv;
}
/* Re-check the idle worker count to guard against a
* race condition. Now that we're in the mutex-protected
* region, one of two things may have happened:
* - If the idle worker count is still negative, the
* workers are all still busy, so it's safe to
* block on a condition variable.
* - If the idle worker count is non-negative, then a
* worker has become idle since the first check
* of queue_info-idlers above. It's possible
* that the worker has also signaled the condition
* variable--and if so, the listener missed it
* because it wasn't yet blocked on the condition
* variable. But if the idle worker count is
* now non-negative, it's safe for this function to
* return immediately.
*
* A negative value in queue_info-idlers tells how many
* threads are waiting on an idle worker.
*/
if (queue_info-idlers 0) {
*had_to_block = 1;
rv = apr_thread_cond_wait(queue_info-wait_for_idler,
queue_info-idlers_mutex);
On Nov 22, 2013, at 3:40 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick traw...@gmail.com wrote:
On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski j...@jagunet.com wrote:
Note, the only think changed in event now (via
https://svn.apache.org/viewvc?view=revisionrevision=1542560)
is that event *checks* that atomics work as required for
event... if the check fails, it means that event has
been broken on that system, assuming it ever hit
blocked idlers, for a *long* time...
Got it... fdqueue.c is asking for trouble...
I'm using atomic/unix/ia32.c with icc too.
Need to compare generated code... I hate stuff like int foo() {
unsigned char x; ... return x; }
APR is documented as returning zero if the value becomes zero on
decrement, otherwise non-zero.
With gcc we use get __sync_sub_and_fetch(), which returns the new value
after the decrement.
With icc we use the assembler implementation in APR. I think that's
returning 1 instead of -1.
Here is where fdqueue needs to be able to see a negative return code:
apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
{
int prev_idlers;
prev_idlers = apr_atomic_dec32((apr_uint32_t
*)(queue_info-idlers));
if (prev_idlers = 0) {
apr_atomic_inc32((apr_uint32_t *)(queue_info-idlers));/*
back out dec */
return APR_EAGAIN;
}
return APR_SUCCESS;
}
(prev in the ia32.c version of
Re: UDS Patch
Sorry, I thought the diffs I sent off list were good enough. I'll have to see if I even still have the original build lying around. Effectively, I just took the list of patches in the backport proposal and applied them one at a time to the 2.4.6 sources. If I can't find the build, I'll do the same over and send that instead. -- Daniel Ruggeri On 11/22/2013 10:38 AM, Jim Jagielski wrote: Any luck with generating the diff yet? On Nov 19, 2013, at 3:08 PM, Jim Jagielski j...@jagunet.com wrote: The main thing is that it would be interesting to see the diffs between '2.4.6 w the (several) originally proposed UDS patches applied in order' and '2.4.6 w proposed backport'... Those diffs should show just the differences between the UDS implementations... On Nov 19, 2013, at 2:51 PM, Daniel Ruggeri drugg...@primary.net wrote: Yes, agreed. Not sure if I made it clear, but I did apply r1511313 for the tests I did today (but not the one from yesterday). Of the several emails sent, the following have been tested: 2.4.6 w the (several) originally proposed UDS patches applied in order 2.4.6 w proposed backport (the 2 chunks around the DNS changes fail to apply since they do not exist in 2.4.6) 2.4.6 w r1511313 + proposed backport + r1543174 I DID double check that the machine wasn't requesting DNS lookups for the socket name or anything strange against the DNS server - but that was only for the test I ran today. -- Daniel Ruggeri On 11/19/2013 1:43 PM, Jim Jagielski wrote: OK... the DNS lookup code seems to have changed between 2.4.6 and 2.4.7: https://svn.apache.org/viewvc?view=revisionrevision=1511313 So I'm wondering if there's something there.
