On Thu, Dec 2, 2021 at 4:32 PM Yann Ylavic <[email protected]> wrote:
>
> Or maybe we could simply:
> --- server/mpm/event/event.c (revision 1895394)
> +++ server/mpm/event/event.c (working copy)
> @@ -3186,8 +3186,8 @@ static void perform_idle_server_maintenance(int ch
> * requests. If the server load changes many times, many such
> * gracefully finishing processes may accumulate, filling up the
> * scoreboard. To avoid running out of scoreboard entries, we
> - * don't shut down more processes when the total number of processes
> - * is high, until there are more than max_workers/4 idle threads.
> + * don't shut down more processes if there are quiescing ones
> + * already (i.e. retained->total_daemons > active_daemons).
> *
> * XXX It would be nice if we could
> * XXX - kill processes without keepalive connections first
> @@ -3195,13 +3195,7 @@ static void perform_idle_server_maintenance(int ch
> * XXX depending on server load, later be able to resurrect them
> * or kill them
> */
> - if ((retained->total_daemons <= active_daemons_limit
> - && retained->total_daemons < server_limit)
> - /* The above test won't transition from true to false until a
> child
> - * exits by itself (i.e. MaxRequestsPerChild reached), so the
> below
> - * test makes sure that the situation unblocks when the load
> falls
> - * significantly (regardless of MaxRequestsPerChild, e.g. 0) */
> - || idle_thread_count > max_workers/4 / num_buckets) {
> + if (retained->total_daemons == active_daemons) {
> /* Kill off one child */
> ap_mpm_podx_signal(retained->buckets[child_bucket].pod,
> AP_MPM_PODX_GRACEFUL);
> ?
> And then kill processes only if there are no stopping ones already..
Full patch attached, we need to retain active_daemons too for this to
work actually.
Cheers;
Yann.
Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c (revision 1895394)
+++ server/mpm/event/event.c (working copy)
@@ -173,8 +173,6 @@ static int ap_daemons_to_start = 0; /* Sta
static int min_spare_threads = 0; /* MinSpareThreads */
static int max_spare_threads = 0; /* MaxSpareThreads */
static int active_daemons_limit = 0; /* MaxRequestWorkers / ThreadsPerChild */
-static int active_daemons = 0; /* workers that still active, i.e. are
- not shutting down gracefully */
static int max_workers = 0; /* MaxRequestWorkers */
static int server_limit = 0; /* ServerLimit */
static int thread_limit = 0; /* ThreadLimit */
@@ -427,8 +425,11 @@ typedef struct event_retained_data {
* Not kept up-to-date when shutdown is pending.
*/
int total_daemons;
-
/*
+ * Workers that still active, i.e. are not shutting down gracefully.
+ */
+ int active_daemons;
+ /*
* idle_spawn_rate is the number of children that will be spawned on the
* next maintenance cycle if there aren't enough idle servers. It is
* maintained per listeners bucket, doubled up to MAX_SPAWN_RATE, and
@@ -3069,7 +3070,7 @@ static int make_child(server_rec * s, int slot, in
ap_scoreboard_image->parent[slot].quiescing = 0;
ap_scoreboard_image->parent[slot].not_accepting = 0;
event_note_child_started(slot, pid);
- active_daemons++;
+ retained->active_daemons++;
retained->total_daemons++;
return 0;
}
@@ -3119,7 +3120,7 @@ static void perform_idle_server_maintenance(int ch
int child_threads_active = 0;
if (ps->quiescing == 1) {
ps->quiescing = 2;
- active_daemons--;
+ retained->active_daemons--;
}
for (j = 0; j < threads_per_child; j++) {
int status = ap_scoreboard_image->servers[i][j].status;
@@ -3178,8 +3179,7 @@ static void perform_idle_server_maintenance(int ch
}
}
- if (idle_thread_count > max_spare_threads / num_buckets)
- {
+ if (idle_thread_count > max_spare_threads / num_buckets) {
/*
* Child processes that we ask to shut down won't die immediately
* but may stay around for a long time when they finish their
@@ -3186,8 +3186,8 @@ static void perform_idle_server_maintenance(int ch
* requests. If the server load changes many times, many such
* gracefully finishing processes may accumulate, filling up the
* scoreboard. To avoid running out of scoreboard entries, we
- * don't shut down more processes when the total number of processes
- * is high, until there are more than max_workers/4 idle threads.
+ * don't shut down more processes if there are stopping ones
+ * already (i.e. total_daemons > active_daemons).
*
* XXX It would be nice if we could
* XXX - kill processes without keepalive connections first
@@ -3195,26 +3195,21 @@ static void perform_idle_server_maintenance(int ch
* XXX depending on server load, later be able to resurrect them
* or kill them
*/
- if ((retained->total_daemons <= active_daemons_limit
- && retained->total_daemons < server_limit)
- /* The above test won't transition from true to false until a child
- * exits by itself (i.e. MaxRequestsPerChild reached), so the below
- * test makes sure that the situation unblocks when the load falls
- * significantly (regardless of MaxRequestsPerChild, e.g. 0) */
- || idle_thread_count > max_workers/4 / num_buckets) {
- /* Kill off one child */
+ int more = (retained->total_daemons == retained->active_daemons);
+ ap_log_error(APLOG_MARK, APLOG_TRACE5, 0, ap_server_conf,
+ "Will%s shut down one child: "
+ "total daemons %d / active daemons %d / "
+ "active limit %d / ServerLimit %d / "
+ "idle threads %d / max workers %d",
+ (more) ? "" : " not",
+ retained->total_daemons, retained->active_daemons,
+ active_daemons_limit, server_limit,
+ idle_thread_count, max_workers);
+ if (more) {
ap_mpm_podx_signal(retained->buckets[child_bucket].pod,
AP_MPM_PODX_GRACEFUL);
- retained->idle_spawn_rate[child_bucket] = 1;
- } else {
- /* Still busy enough, don't kill */
- ap_log_error(APLOG_MARK, APLOG_TRACE5, 0, ap_server_conf,
- "Not shutting down child: total daemons %d / "
- "active limit %d / ServerLimit %d / "
- "idle threads %d / max workers %d",
- retained->total_daemons, active_daemons_limit,
- server_limit, idle_thread_count, max_workers);
}
+ retained->idle_spawn_rate[child_bucket] = 1;
}
else if (idle_thread_count < min_spare_threads / num_buckets) {
if (active_thread_count >= max_workers / num_buckets) {
@@ -3248,9 +3243,9 @@ static void perform_idle_server_maintenance(int ch
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) {
- if (active_daemons < active_daemons_limit) {
- free_length = active_daemons_limit - active_daemons;
+ if (free_length + retained->active_daemons > active_daemons_limit) {
+ if (retained->active_daemons < active_daemons_limit) {
+ free_length = active_daemons_limit - retained->active_daemons;
}
else {
ap_log_error(APLOG_MARK, APLOG_TRACE1, 0, ap_server_conf,
@@ -3257,7 +3252,7 @@ static void perform_idle_server_maintenance(int ch
"server is at active daemons limit, spawning "
"of %d children cancelled: %d/%d active, "
"rate %d", free_length,
- active_daemons, active_daemons_limit,
+ retained->active_daemons, active_daemons_limit,
retained->idle_spawn_rate[child_bucket]);
free_length = 0;
}
@@ -3270,7 +3265,7 @@ static void perform_idle_server_maintenance(int ch
"spawning %d children, there are around %d idle "
"threads, %d active children, and %d children "
"that are shutting down", free_length,
- idle_thread_count, active_daemons,
+ idle_thread_count, retained->active_daemons,
retained->total_daemons);
}
for (i = 0; i < free_length; ++i) {
@@ -3277,7 +3272,7 @@ static void perform_idle_server_maintenance(int ch
ap_log_error(APLOG_MARK, APLOG_TRACE5, 0, ap_server_conf,
"Spawning new child: slot %d active / "
"total daemons: %d/%d",
- free_slots[i], active_daemons,
+ free_slots[i], retained->active_daemons,
retained->total_daemons);
make_child(ap_server_conf, free_slots[i], child_bucket);
}
@@ -3359,7 +3354,7 @@ static void server_main_loop(int remaining_childre
event_note_child_killed(child_slot, 0, 0);
ps = &ap_scoreboard_image->parent[child_slot];
if (!ps->quiescing)
- active_daemons--;
+ retained->active_daemons--;
ps->quiescing = 0;
/* NOTE: We don't dec in the (child_slot < 0) case! */
retained->total_daemons--;
@@ -3534,8 +3529,6 @@ static int event_run(apr_pool_t * _pconf, apr_pool
}
retained->mpm->num_buckets = num_buckets;
- active_daemons = 0;
-
/* Don't thrash since num_buckets depends on the
* system and the number of online CPU cores...
*/
