Mailing List Archive

Re: svn commit: r1893014 - in /httpd/httpd/trunk: changes-entries/event_maintenance_spawn_limit.txt server/mpm/event/event.c
On 9/7/21 11:34 AM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Tue Sep 7 09:34:09 2021
> New Revision: 1893014
>
> URL: http://svn.apache.org/viewvc?rev=1893014&view=rev
> Log:
> mpm_event: Fix children processes possibly not stopped on graceful restart.
>
> The number of children spawned can go above active_daemons_limit due to
> exponential idle_spawn_rate growth (x 2), enforce the upper limit in
> perform_idle_server_maintenance(). PR 63169.
>
> Proposed by: Joel Self <joelself gmail.com>
>
> Added:
> httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt
> Modified:
> httpd/httpd/trunk/server/mpm/event/event.c
>
> Added: httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt?rev=1893014&view=auto
> ==============================================================================
> --- httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt (added)
> +++ httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt Tue Sep 7 09:34:09 2021
> @@ -0,0 +1,2 @@
> + *) mpm_event: Fix children processes possibly not stopped on graceful
> + restart. PR 63169. [Joel Self <joelself gmail.com>]
> \ No newline at end of file
>
> Modified: httpd/httpd/trunk/server/mpm/event/event.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1893014&r1=1893013&r2=1893014&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Tue Sep 7 09:34:09 2021
> @@ -3237,6 +3237,9 @@ static void perform_idle_server_maintena
> if (free_length > retained->idle_spawn_rate[child_bucket]) {
> free_length = retained->idle_spawn_rate[child_bucket];
> }
> + if (free_length + active_daemons > active_daemons_limit) {
> + free_length = active_daemons_limit - active_daemons;

Are we sure that we can't have cases where active_daemons_limit < active_daemons and free_length gets below zero?
Shouldn't we do something like

free_length = free_length < 0 ? 0 : free_length;

in addition.

Regards

Rüdiger
Re: svn commit: r1893014 - in /httpd/httpd/trunk: changes-entries/event_maintenance_spawn_limit.txt server/mpm/event/event.c [ In reply to ]
On Tue, Sep 7, 2021 at 6:08 PM Ruediger Pluem <rpluem@apache.org> wrote:
>
> On 9/7/21 11:34 AM, ylavic@apache.org wrote:
> > Author: ylavic
> > Date: Tue Sep 7 09:34:09 2021
> > New Revision: 1893014
> >
> > URL: http://svn.apache.org/viewvc?rev=1893014&view=rev
> > Log:
> > mpm_event: Fix children processes possibly not stopped on graceful restart.
> >
> > The number of children spawned can go above active_daemons_limit due to
> > exponential idle_spawn_rate growth (x 2), enforce the upper limit in
> > perform_idle_server_maintenance(). PR 63169.
> >
> > Proposed by: Joel Self <joelself gmail.com>
> >
> > Added:
> > httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt
> > Modified:
> > httpd/httpd/trunk/server/mpm/event/event.c
> >
> > Added: httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt
> > URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt?rev=1893014&view=auto
> > ==============================================================================
> > --- httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt (added)
> > +++ httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt Tue Sep 7 09:34:09 2021
> > @@ -0,0 +1,2 @@
> > + *) mpm_event: Fix children processes possibly not stopped on graceful
> > + restart. PR 63169. [Joel Self <joelself gmail.com>]
> > \ No newline at end of file
> >
> > Modified: httpd/httpd/trunk/server/mpm/event/event.c
> > URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1893014&r1=1893013&r2=1893014&view=diff
> > ==============================================================================
> > --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> > +++ httpd/httpd/trunk/server/mpm/event/event.c Tue Sep 7 09:34:09 2021
> > @@ -3237,6 +3237,9 @@ static void perform_idle_server_maintena
> > if (free_length > retained->idle_spawn_rate[child_bucket]) {
> > free_length = retained->idle_spawn_rate[child_bucket];
> > }
> > + if (free_length + active_daemons > active_daemons_limit) {
> > + free_length = active_daemons_limit - active_daemons;
>
> Are we sure that we can't have cases where active_daemons_limit < active_daemons and free_length gets below zero?

If free_length (signed int) gets negative we'll do nothing in the "for
(i = 0; i < free_length; ...)" loop below, so I think we are safe?

Cheers;
Yann.
Re: svn commit: r1893014 - in /httpd/httpd/trunk: changes-entries/event_maintenance_spawn_limit.txt server/mpm/event/event.c [ In reply to ]
Hi,

maybe completely off-topic, but we have the same kind of code in worker.c.

Just my 2c,

CJ

Le 07/09/2021 à 11:34, ylavic@apache.org a écrit :
> Author: ylavic
> Date: Tue Sep 7 09:34:09 2021
> New Revision: 1893014
>
> URL: http://svn.apache.org/viewvc?rev=1893014&view=rev
> Log:
> mpm_event: Fix children processes possibly not stopped on graceful restart.
>
> The number of children spawned can go above active_daemons_limit due to
> exponential idle_spawn_rate growth (x 2), enforce the upper limit in
> perform_idle_server_maintenance(). PR 63169.
>
> Proposed by: Joel Self <joelself gmail.com>
>
> Added:
> httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt
> Modified:
> httpd/httpd/trunk/server/mpm/event/event.c
>
> Added: httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt?rev=1893014&view=auto
> ==============================================================================
> --- httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt (added)
> +++ httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt Tue Sep 7 09:34:09 2021
> @@ -0,0 +1,2 @@
> + *) mpm_event: Fix children processes possibly not stopped on graceful
> + restart. PR 63169. [Joel Self <joelself gmail.com>]
> \ No newline at end of file
>
> Modified: httpd/httpd/trunk/server/mpm/event/event.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1893014&r1=1893013&r2=1893014&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Tue Sep 7 09:34:09 2021
> @@ -3237,6 +3237,9 @@ static void perform_idle_server_maintena
> if (free_length > retained->idle_spawn_rate[child_bucket]) {
> free_length = retained->idle_spawn_rate[child_bucket];
> }
> + if (free_length + active_daemons > active_daemons_limit) {
> + free_length = active_daemons_limit - active_daemons;
> + }
> if (retained->idle_spawn_rate[child_bucket] >= 8) {
> ap_log_error(APLOG_MARK, APLOG_INFO, 0, ap_server_conf, APLOGNO(00486)
> "server seems busy, (you may need "
>
>
Re: svn commit: r1893014 - in /httpd/httpd/trunk: changes-entries/event_maintenance_spawn_limit.txt server/mpm/event/event.c [ In reply to ]
On 9/7/21 6:41 PM, Yann Ylavic wrote:
> On Tue, Sep 7, 2021 at 6:08 PM Ruediger Pluem <rpluem@apache.org> wrote:
>>
>> On 9/7/21 11:34 AM, ylavic@apache.org wrote:
>>> Author: ylavic
>>> Date: Tue Sep 7 09:34:09 2021
>>> New Revision: 1893014
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1893014&view=rev
>>> Log:
>>> mpm_event: Fix children processes possibly not stopped on graceful restart.
>>>
>>> The number of children spawned can go above active_daemons_limit due to
>>> exponential idle_spawn_rate growth (x 2), enforce the upper limit in
>>> perform_idle_server_maintenance(). PR 63169.
>>>
>>> Proposed by: Joel Self <joelself gmail.com>
>>>
>>> Added:
>>> httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt
>>> Modified:
>>> httpd/httpd/trunk/server/mpm/event/event.c
>>>
>>> Added: httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt
>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt?rev=1893014&view=auto
>>> ==============================================================================
>>> --- httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt (added)
>>> +++ httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt Tue Sep 7 09:34:09 2021
>>> @@ -0,0 +1,2 @@
>>> + *) mpm_event: Fix children processes possibly not stopped on graceful
>>> + restart. PR 63169. [Joel Self <joelself gmail.com>]
>>> \ No newline at end of file
>>>
>>> Modified: httpd/httpd/trunk/server/mpm/event/event.c
>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1893014&r1=1893013&r2=1893014&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>>> +++ httpd/httpd/trunk/server/mpm/event/event.c Tue Sep 7 09:34:09 2021
>>> @@ -3237,6 +3237,9 @@ static void perform_idle_server_maintena
>>> if (free_length > retained->idle_spawn_rate[child_bucket]) {
>>> free_length = retained->idle_spawn_rate[child_bucket];
>>> }
>>> + if (free_length + active_daemons > active_daemons_limit) {
>>> + free_length = active_daemons_limit - active_daemons;
>>
>> Are we sure that we can't have cases where active_daemons_limit < active_daemons and free_length gets below zero?
>
> If free_length (signed int) gets negative we'll do nothing in the "for
> (i = 0; i < free_length; ...)" loop below, so I think we are safe?

Fair enough. But we could log a strange AH00486 with a negative number of children we want to spawn.
But for avoiding this it would be sufficient to put

free_length < 0 ? 0 : free_length

as parameter for the ap_log_error call instead of free_length. This would limit the effort to the case where we need to log.


Regards

Rüdiger
Re: svn commit: r1893014 - in /httpd/httpd/trunk: changes-entries/event_maintenance_spawn_limit.txt server/mpm/event/event.c [ In reply to ]
On Tue, Sep 7, 2021 at 9:28 PM Ruediger Pluem <rpluem@apache.org> wrote:
>
> On 9/7/21 6:41 PM, Yann Ylavic wrote:
> > On Tue, Sep 7, 2021 at 6:08 PM Ruediger Pluem <rpluem@apache.org> wrote:
> >>
> >> On 9/7/21 11:34 AM, ylavic@apache.org wrote:
> >>> Author: ylavic
> >>> Date: Tue Sep 7 09:34:09 2021
> >>> New Revision: 1893014
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=1893014&view=rev
> >>> Log:
> >>> mpm_event: Fix children processes possibly not stopped on graceful restart.
> >>>
> >>> The number of children spawned can go above active_daemons_limit due to
> >>> exponential idle_spawn_rate growth (x 2), enforce the upper limit in
> >>> perform_idle_server_maintenance(). PR 63169.
> >>>
> >>> Proposed by: Joel Self <joelself gmail.com>
> >>>
> >>> Added:
> >>> httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt
> >>> Modified:
> >>> httpd/httpd/trunk/server/mpm/event/event.c
> >>>
> >>> Added: httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt
> >>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt?rev=1893014&view=auto
> >>> ==============================================================================
> >>> --- httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt (added)
> >>> +++ httpd/httpd/trunk/changes-entries/event_maintenance_spawn_limit.txt Tue Sep 7 09:34:09 2021
> >>> @@ -0,0 +1,2 @@
> >>> + *) mpm_event: Fix children processes possibly not stopped on graceful
> >>> + restart. PR 63169. [Joel Self <joelself gmail.com>]
> >>> \ No newline at end of file
> >>>
> >>> Modified: httpd/httpd/trunk/server/mpm/event/event.c
> >>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1893014&r1=1893013&r2=1893014&view=diff
> >>> ==============================================================================
> >>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> >>> +++ httpd/httpd/trunk/server/mpm/event/event.c Tue Sep 7 09:34:09 2021
> >>> @@ -3237,6 +3237,9 @@ static void perform_idle_server_maintena
> >>> if (free_length > retained->idle_spawn_rate[child_bucket]) {
> >>> free_length = retained->idle_spawn_rate[child_bucket];
> >>> }
> >>> + if (free_length + active_daemons > active_daemons_limit) {
> >>> + free_length = active_daemons_limit - active_daemons;
> >>
> >> Are we sure that we can't have cases where active_daemons_limit < active_daemons and free_length gets below zero?
> >
> > If free_length (signed int) gets negative we'll do nothing in the "for
> > (i = 0; i < free_length; ...)" loop below, so I think we are safe?
>
> Fair enough. But we could log a strange AH00486 with a negative number of children we want to spawn.
> But for avoiding this it would be sufficient to put
>
> free_length < 0 ? 0 : free_length
>
> as parameter for the ap_log_error call instead of free_length. This would limit the effort to the case where we need to log.

In r1893073, I have added an APLOG_TRACE1 and cleared free_length when
active_daemons >= active_daemons_limit.


Cheers;
Yann.
Re: svn commit: r1893014 - in /httpd/httpd/trunk: changes-entries/event_maintenance_spawn_limit.txt server/mpm/event/event.c [ In reply to ]
On Tue, Sep 7, 2021 at 8:29 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> maybe completely off-topic, but we have the same kind of code in worker.c.

This may happen with mpm_worker too (with specific MPM tunings), but
we have no report so far and mainly no active_daemons accounting
there.
This means more changes than in mpm_event, so I'd rather wait for
someone being hit by this..

Maybe we could use total_non_dead and ap_daemons_limit instead of
active_daemons and active_daemons_limit respectively, but mpm_event
and mpm_worker don't really handle graceful restart in the same way
anymore so this would need more analysis.

Cheers;
Yann.