You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2007/06/01 23:30:22 UTC
Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
On 06/01/2007 05:42 PM, jim@apache.org wrote:
> Author: jim
> Date: Fri Jun 1 08:42:57 2007
> New Revision: 543511
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=543511
> Log:
> Add in parent process PID table, to provide for
> a check against the pid values located in the
> scoreboard.
>
> Modified:
> httpd/httpd/branches/1.3.x/src/main/http_main.c
>
> Modified: httpd/httpd/branches/1.3.x/src/main/http_main.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/1.3.x/src/main/http_main.c?view=diff&rev=543511&r1=543510&r2=543511
> ==============================================================================
> --- httpd/httpd/branches/1.3.x/src/main/http_main.c (original)
> +++ httpd/httpd/branches/1.3.x/src/main/http_main.c Fri Jun 1 08:42:57 2007
> @@ -354,9 +354,17 @@
> char tpf_mutex_key[TPF_MUTEX_KEY_SIZE];
> #endif /* TPF */
>
> +/*
> + * Shared memory scoreboard
> + */
> scoreboard *ap_scoreboard_image = NULL;
>
> /*
> + * Parent process local storage of child pids
> + */
> +static table *pid_table;
For my understanding (and a bit of devils advocate here :-)): Why do we use a
table here and not a fixed size array (HARD_SERVER_LIMIT) of ints (apr_array of
pid_t in the 2.x case). If we keep the pids at the same index as in the
scoreboard the checks would be somewhat faster and simpler to do. Of course
we waste more memory.
> @@ -2787,6 +2821,11 @@
> if (pid == my_pid || pid == 0)
> continue;
>
> + if (!in_pid_table(pid)) {
> + ap_log_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, server_conf,
> + "Bad pid (%d) in scoreboard slot %d", pid, i);
> + continue;
> + }
> waitret = waitpid(pid, &status, WNOHANG);
> if (waitret == pid || waitret == -1) {
> ap_scoreboard_image->parent[i].pid = 0;
Don't we need to remove the pid from our local table here as well?
> @@ -5113,8 +5170,15 @@
> else if (ps->last_rtime + ss->timeout_len < now) {
> /* no progress, and the timeout length has been exceeded */
> ss->timeout_len = 0;
> - kill(ps->pid, SIG_TIMEOUT_KILL);
> - }
> + pid = ps->pid;
> + if (in_pid_table(pid)) {
> + kill(pid, SIG_TIMEOUT_KILL);
Don't we need to remove the pid from our local table here as well?
> + }
> + else {
> + ap_log_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, server_conf,
> + "Bad pid (%d) in scoreboard slot %d", pid, i);
> + }
> + }
> }
> #endif
> }
Regards
Rüdiger
Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jun 4, 2007, at 10:29 PM, David McCreedy wrote:
> June 04, 2007 5:51 PM David McCreedy wrote:
>
> >On 06/01/2007 05:42 PM, jim@apache.org wrote:
> >>
> >>I think I squashed those. Could you check out
> >>trunk and try another test? Thanks!
> >>
> >
> >It fixes the "Bad pid" error but I'm not sure all is well...
> >
> >On TPF we're not calling unset_pid_table on SIG_IDLE_KILLs.
> >I'll have to track down why.
> >
>
> I've figured out why some pids aren't being "unset" and I think it
> could affect other platforms besides TPF.
> They're hitting the else part of the "if (child_slot >= 0)"
> statement in http_main.c's standalone_main function.
> I think the unset_pid_table call should be moved before this "if".
> If we're in this section of code, don't we want to remove the pid
> from the table regardless of whether the slot is found in the
> scoreboard?
>
Yes, in fact Rüdiger just committed a patch
that does that.
Re: pid_table (Was: Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c)
Posted by Ruediger Pluem <rp...@apache.org>.
On 06/16/2007 05:40 PM, Jim Jagielski wrote:
> Status Update:
>
> The pid-table code is:
> o Applied to 1.3 branch
> o In httpd-2.0-pid-table branch (branches/2.0.x "fork")
> o In httpd-2.2-pid-table branch (branches/2.2.x "fork")
> o In httpd-pid-table branch (trunk "fork")
>
> Passes httpd-tests, as well as 'ab' with *very* small
> MaxRequestsPerChild and Max/MinSpare* settings to
> exercise the child fork/reap logic... so far, so good.
Thank you very much for pushing this forward.
> Proposal is to fold them into their respective branches
> (or trunk) in anticipation of additional testing
> and release. Comments?
I haven't got time so far to do any tests, but I only reviewed the code.
But given that and your review and test results I would propose the following:
1. Lets fold httpd-pid-table into trunk now.
2. Give it 72 hours (or less) there and then lets make the backport
proposals for 2.0.x / 2.2.x.
Regards
Rüdiger
Re: pid_table (Was: Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c)
Posted by Jim Jagielski <ji...@jaguNET.com>.
Status Update:
The pid-table code is:
o Applied to 1.3 branch
o In httpd-2.0-pid-table branch (branches/2.0.x "fork")
o In httpd-2.2-pid-table branch (branches/2.2.x "fork")
o In httpd-pid-table branch (trunk "fork")
Passes httpd-tests, as well as 'ab' with *very* small
MaxRequestsPerChild and Max/MinSpare* settings to
exercise the child fork/reap logic... so far, so good.
Proposal is to fold them into their respective branches
(or trunk) in anticipation of additional testing
and release. Comments?
Re: pid_table (Was: Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c)
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jun 6, 2007, at 9:13 AM, Jim Jagielski wrote:
>
> In the meantime, should I create a 2.2 branch for the 2.2-version
> of the pid_table code and backport the changes to that?
> Unless I hear otherwise, I'll likely do that since the
> backport from 2.2 to 2.0 shouldn't be that involved.
>
Done and done: passes tests.
http://svn.apache.org/repos/asf/httpd/httpd/branches/httpd-2.2-
pid-table
Next to come is 2.0, but, IMO, we should start folding these
in, ala 1.3
pid_table (Was: Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c)
Posted by Jim Jagielski <ji...@jaguNET.com>.
It looks the the 1.3 pid_table impl is pretty much on target.
I've been testing the trunk (2.3.x) version with no issues that
I've been able to see, but was wondering how many others
are testing...
In the meantime, should I create a 2.2 branch for the 2.2-version
of the pid_table code and backport the changes to that?
Unless I hear otherwise, I'll likely do that since the
backport from 2.2 to 2.0 shouldn't be that involved.
Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
Posted by David McCreedy <Mc...@msn.com>.
>On June 05, 2007 1:45 AM Rüdiger wrote
>> On June 04, 2007 5:51 PM David McCreedy wrote:
>> I've figured out why some pids aren't being "unset" and I think it could affect other platforms besides TPF.
>> They're hitting the else part of the "if (child_slot >= 0)" statement in http_main.c's standalone_main function.
>> I think the unset_pid_table call should be moved before this "if".
>> If we're in this section of code, don't we want to remove the pid from the table regardless of whether the slot is found in the scoreboard?
>
>Yes, we want to. Please have a look at r544218 (http://svn.apache.org/viewvc?view=rev&revision=544218<http://svn.apache.org/viewvc?view=rev&revision=544218>)
>
>
> Rüdiger
Perfect.
Thanks,
-David
----- Original Message -----
From: Plüm<mailto:Plüm> ; Rüdiger<mailto:Rüdiger> ; VF-Group<ma...@vodafone.com>
To: dev@httpd.apache.org<ma...@httpd.apache.org>
Sent: Tuesday, June 05, 2007 1:45 AM
Subject: AW: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
-----Ursprüngliche Nachricht-----
Von: David McCreedy
Gesendet: Dienstag, 5. Juni 2007 04:29
An: dev@httpd.apache.org<ma...@httpd.apache.org>
Betreff: Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
June 04, 2007 5:51 PM David McCreedy wrote:
>On 06/01/2007 05:42 PM, jim@apache.org<ma...@apache.org> wrote:
>>
>>I think I squashed those. Could you check out
>>trunk and try another test? Thanks!
>>
>
>It fixes the "Bad pid" error but I'm not sure all is well...
>
>On TPF we're not calling unset_pid_table on SIG_IDLE_KILLs.
>I'll have to track down why.
>
> I've figured out why some pids aren't being "unset" and I think it could affect other platforms besides TPF.
> They're hitting the else part of the "if (child_slot >= 0)" statement in http_main.c's standalone_main function.
> I think the unset_pid_table call should be moved before this "if".
> If we're in this section of code, don't we want to remove the pid from the table regardless of whether the slot is found in the scoreboard?
Yes, we want to. Please have a look at r544218 (http://svn.apache.org/viewvc?view=rev&revision=544218<http://svn.apache.org/viewvc?view=rev&revision=544218>)
Regards
Rüdiger
AW: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
Posted by Plüm,
Rüdiger,
VF-Group <ru...@vodafone.com>.
-----Ursprüngliche Nachricht-----
Von: David McCreedy
Gesendet: Dienstag, 5. Juni 2007 04:29
An: dev@httpd.apache.org
Betreff: Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
June 04, 2007 5:51 PM David McCreedy wrote:
>On 06/01/2007 05:42 PM, jim@apache.org wrote:
>>
>>I think I squashed those. Could you check out
>>trunk and try another test? Thanks!
>>
>
>It fixes the "Bad pid" error but I'm not sure all is well...
>
>On TPF we're not calling unset_pid_table on SIG_IDLE_KILLs.
>I'll have to track down why.
>
> I've figured out why some pids aren't being "unset" and I think it could affect other platforms besides TPF.
> They're hitting the else part of the "if (child_slot >= 0)" statement in http_main.c's standalone_main function.
> I think the unset_pid_table call should be moved before this "if".
> If we're in this section of code, don't we want to remove the pid from the table regardless of whether the slot is found in the scoreboard?
Yes, we want to. Please have a look at r544218 (http://svn.apache.org/viewvc?view=rev&revision=544218)
Regards
Rüdiger
Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
Posted by David McCreedy <Mc...@msn.com>.
June 04, 2007 5:51 PM David McCreedy wrote:
>On 06/01/2007 05:42 PM, jim@apache.org<ma...@apache.org> wrote:
>>
>>I think I squashed those. Could you check out
>>trunk and try another test? Thanks!
>>
>
>It fixes the "Bad pid" error but I'm not sure all is well...
>
>On TPF we're not calling unset_pid_table on SIG_IDLE_KILLs.
>I'll have to track down why.
>
I've figured out why some pids aren't being "unset" and I think it could affect other platforms besides TPF.
They're hitting the else part of the "if (child_slot >= 0)" statement in http_main.c's standalone_main function.
I think the unset_pid_table call should be moved before this "if".
If we're in this section of code, don't we want to remove the pid from the table regardless of whether the slot is found in the scoreboard?
-David McCreedy
Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
Posted by David McCreedy <Mc...@msn.com>.
On 06/01/2007 05:42 PM, jim@apache.org<ma...@apache.org> wrote:
>
>I think I squashed those. Could you check out
>trunk and try another test? Thanks!
>
It fixes the "Bad pid" error but I'm not sure all is well...
On TPF we're not calling unset_pid_table on SIG_IDLE_KILLs.
I'll have to track down why.
It seems OK on Linux but I see SIG_IDLE_KILL being sent twice to the same PID before it calls unset_pid_table.
That seems odd to me.
-David McCreedy
Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jun 4, 2007, at 2:35 AM, David McCreedy wrote:
> On 06/01/2007 05:42 PM, jim@apache.org wrote:
> > Author: jim
> > Date: Fri Jun 1 08:42:57 2007
> > New Revision: 543511
> >
> > URL: http://svn.apache.org/viewvc?view=rev&rev=543511
> > Log:
> > Add in parent process PID table, to provide for
> > a check against the pid values located in the
> > scoreboard.
> >
> > Modified:
> > httpd/httpd/branches/1.3.x/src/main/http_main.c
> >
>
> I tried out the code in apache-1.3_20070603161656.tar.gz on TPF
> because I
> thought the pid_table stuff might affect the platform.
>
I think I squashed those. Could you check out
trunk and try another test? Thanks!
Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jun 4, 2007, at 2:35 AM, David McCreedy wrote:
> On 06/01/2007 05:42 PM, jim@apache.org wrote:
> > Author: jim
> > Date: Fri Jun 1 08:42:57 2007
> > New Revision: 543511
> >
> > URL: http://svn.apache.org/viewvc?view=rev&rev=543511
> > Log:
> > Add in parent process PID table, to provide for
> > a check against the pid values located in the
> > scoreboard.
> >
> > Modified:
> > httpd/httpd/branches/1.3.x/src/main/http_main.c
> >
>
> I tried out the code in apache-1.3_20070603161656.tar.gz on TPF
> because I
> thought the pid_table stuff might affect the platform.
>
>
> Hopefully this is enough information to recreate the issues.
> If I left something out please let me know.
>
Thanks for the testing... I'm unable to recreate the problems
you're seeing, but at least I know what areas might be
causing them...
Thanks again!
Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
Posted by David McCreedy <Mc...@msn.com>.
On 06/01/2007 05:42 PM, jim@apache.org<ma...@apache.org> wrote:
> Author: jim
> Date: Fri Jun 1 08:42:57 2007
> New Revision: 543511
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=543511<http://svn.apache.org/viewvc?view=rev&rev=543511>
> Log:
> Add in parent process PID table, to provide for
> a check against the pid values located in the
> scoreboard.
>
> Modified:
> httpd/httpd/branches/1.3.x/src/main/http_main.c
>
I tried out the code in apache-1.3_20070603161656.tar.gz on TPF because I
thought the pid_table stuff might affect the platform.
I found some problems in main/http_main.c on both Linux and TPF:
1) The new snprintf calls need to be changed to ap_snprintf for it to
compile on TPF (and possibly other platforms without a native snprintf).
2) For both Linux and TPF, idle server maintenance can cause "Bad pid in
scoreboard slot" errors:
I tied up several children with requests and then let them time out so
they'd become idle.
(I lowered MinSpareServers to 1 and MaxSpareServers to 3 so it was easy to
reach the limits.)
This hit the kill(pid, SIG_TIMEOUT_KILL) and kill(pid, SIG_IDLE_KILL) code
in perform_idle_server_maintenance.
Those same pids showed up later in "Bad pid in scoreboard slot" errors.
3) For both Linux and TPF, "Bad pid in scoreboard slot" errors can loop:
I hit this with issue #2 above but presumably you could get it on any
in_pid_table error where that scoreboard slot is hit again at a later
time...
I got looping error messages if the idle child that initially cause the
"Bad pid ..." error was the one that perform_idle_server_maintenance was
trying to get rid of.
An in_pid_table() error writes to the log but leaves the scoreboard slot
as-is.
So when we come through perform_idle_server_maintenance the next time, the
same error is logged for the same slot and pid.
Again, and again.
Hopefully this is enough information to recreate the issues.
If I left something out please let me know.
-David McCreedy
Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Jim Jagielski wrote:
> Ruediger Pluem wrote:
>> For my understanding (and a bit of devils advocate here :-)): Why do we use a
>> table here and not a fixed size array (HARD_SERVER_LIMIT) of ints (apr_array of
>> pid_t in the 2.x case). If we keep the pids at the same index as in the
>> scoreboard the checks would be somewhat faster and simpler to do. Of course
>> we waste more memory.
>>
>
> We can afford the time taken I think, to save space and to
> be more efficient... I really dislike all that wasted space :)
One issue that can be solved here are the long-lost children come home,
since we can keep more PID's than fit in the scoreboard. It's worth
looking at.
Bill
Re: svn commit: r543511 - /httpd/httpd/branches/1.3.x/src/main/http_main.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
Ruediger Pluem wrote:
>
> For my understanding (and a bit of devils advocate here :-)): Why do we use a
> table here and not a fixed size array (HARD_SERVER_LIMIT) of ints (apr_array of
> pid_t in the 2.x case). If we keep the pids at the same index as in the
> scoreboard the checks would be somewhat faster and simpler to do. Of course
> we waste more memory.
>
We can afford the time taken I think, to save space and to
be more efficient... I really dislike all that wasted space :)
--
===========================================================================
Jim Jagielski [|] jim@jaguNET.com [|] http://www.jaguNET.com/
"If you can dodge a wrench, you can dodge a ball."