On Thu, Dec 2, 2021 at 4:32 PM Yann Ylavic <ylavic....@gmail.com> 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... */