From a3c1168ac293f16d9343d248795bb4c246aaff4a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Aug 2018 16:26:27 +0200 Subject: [PATCH] core: rework StopWhenUnneeded= logic Previously, we'd act immediately on StopWhenUnneeded= when a unit state changes. With this rework we'll maintain a queue instead: whenever there's the chance that StopWhenUneeded= might have an effect we enqueue the unit, and process it later when we have nothing better to do. This should make the implementation a bit more reliable, as the unit notify event cannot immediately enqueue tons of side-effect jobs that might contradict each other, but we do so only in a strictly ordered fashion, from the main event loop. This slightly changes the check when to consider a unit "unneeded". Previously, we'd assume that a unit in "deactivating" state could also be cleaned up. With this new logic we'll only consider units unneeded that are fully up and have no job queued. This means that whenever there's something pending for a unit we won't clean it up. --- src/core/manager.c | 43 ++++++++++++++++ src/core/manager.h | 3 ++ src/core/unit.c | 124 +++++++++++++++++++++++++-------------------- src/core/unit.h | 7 +++ 4 files changed, 121 insertions(+), 56 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 8dc3f5e55f..88bfb71cf1 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1211,6 +1211,45 @@ static unsigned manager_dispatch_gc_job_queue(Manager *m) { return n; } +static unsigned manager_dispatch_stop_when_unneeded_queue(Manager *m) { + unsigned n = 0; + Unit *u; + int r; + + assert(m); + + while ((u = m->stop_when_unneeded_queue)) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + assert(m->stop_when_unneeded_queue); + + assert(u->in_stop_when_unneeded_queue); + LIST_REMOVE(stop_when_unneeded_queue, m->stop_when_unneeded_queue, u); + u->in_stop_when_unneeded_queue = false; + + n++; + + if (!unit_is_unneeded(u)) + continue; + + log_unit_debug(u, "Unit is not needed anymore."); + + /* If stopping a unit fails continuously we might enter a stop loop here, hence stop acting on the + * service being unnecessary after a while. */ + + if (!ratelimit_below(&u->auto_stop_ratelimit)) { + log_unit_warning(u, "Unit not needed anymore, but not stopping since we tried this too often recently."); + continue; + } + + /* Ok, nobody needs us anymore. Sniff. Then let's commit suicide */ + r = manager_add_job(u->manager, JOB_STOP, u, JOB_FAIL, &error, NULL); + if (r < 0) + log_unit_warning_errno(u, r, "Failed to enqueue stop job, ignoring: %s", bus_error_message(&error, r)); + } + + return n; +} + static void manager_clear_jobs_and_units(Manager *m) { Unit *u; @@ -1228,6 +1267,7 @@ static void manager_clear_jobs_and_units(Manager *m) { assert(!m->cleanup_queue); assert(!m->gc_unit_queue); assert(!m->gc_job_queue); + assert(!m->stop_when_unneeded_queue); assert(hashmap_isempty(m->jobs)); assert(hashmap_isempty(m->units)); @@ -2797,6 +2837,9 @@ int manager_loop(Manager *m) { if (manager_dispatch_cgroup_realize_queue(m) > 0) continue; + if (manager_dispatch_stop_when_unneeded_queue(m) > 0) + continue; + if (manager_dispatch_dbus_queue(m) > 0) continue; diff --git a/src/core/manager.h b/src/core/manager.h index 342258fcf9..f7701b1c8d 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -137,6 +137,9 @@ struct Manager { /* Target units whose default target dependencies haven't been set yet */ LIST_HEAD(Unit, target_deps_queue); + /* Units that might be subject to StopWhenUnneeded= clean-up */ + LIST_HEAD(Unit, stop_when_unneeded_queue); + sd_event *event; /* This maps PIDs we care about to units that are interested in. We allow multiple units to he interested in diff --git a/src/core/unit.c b/src/core/unit.c index 17f4ff3ebd..172624a547 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -438,6 +438,22 @@ void unit_add_to_dbus_queue(Unit *u) { u->in_dbus_queue = true; } +void unit_add_to_stop_when_unneeded_queue(Unit *u) { + assert(u); + + if (u->in_stop_when_unneeded_queue) + return; + + if (!u->stop_when_unneeded) + return; + + if (!UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(u))) + return; + + LIST_PREPEND(stop_when_unneeded_queue, u->manager->stop_when_unneeded_queue, u); + u->in_stop_when_unneeded_queue = true; +} + static void bidi_set_free(Unit *u, Hashmap *h) { Unit *other; Iterator i; @@ -634,6 +650,9 @@ void unit_free(Unit *u) { if (u->in_target_deps_queue) LIST_REMOVE(target_deps_queue, u->manager->target_deps_queue, u); + if (u->in_stop_when_unneeded_queue) + LIST_REMOVE(stop_when_unneeded_queue, u->manager->stop_when_unneeded_queue, u); + safe_close(u->ip_accounting_ingress_map_fd); safe_close(u->ip_accounting_egress_map_fd); @@ -1950,55 +1969,71 @@ bool unit_can_reload(Unit *u) { return UNIT_VTABLE(u)->reload; } -static void unit_check_unneeded(Unit *u) { - - _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - - static const UnitDependency needed_dependencies[] = { +bool unit_is_unneeded(Unit *u) { + static const UnitDependency deps[] = { UNIT_REQUIRED_BY, UNIT_REQUISITE_OF, UNIT_WANTED_BY, UNIT_BOUND_BY, }; - - unsigned j; - int r; + size_t j; assert(u); - /* If this service shall be shut down when unneeded then do - * so. */ - if (!u->stop_when_unneeded) - return; + return false; - if (!UNIT_IS_ACTIVE_OR_ACTIVATING(unit_active_state(u))) - return; + /* Don't clean up while the unit is transitioning or is even inactive. */ + if (!UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(u))) + return false; + if (u->job) + return false; - for (j = 0; j < ELEMENTSOF(needed_dependencies); j++) { + for (j = 0; j < ELEMENTSOF(deps); j++) { Unit *other; Iterator i; void *v; - HASHMAP_FOREACH_KEY(v, other, u->dependencies[needed_dependencies[j]], i) - if (unit_active_or_pending(other) || unit_will_restart(other)) - return; + /* If a dependending unit has a job queued, or is active (or in transitioning), or is marked for + * restart, then don't clean this one up. */ + + HASHMAP_FOREACH_KEY(v, other, u->dependencies[deps[j]], i) { + if (u->job) + return false; + + if (!UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other))) + return false; + + if (unit_will_restart(other)) + return false; + } } - /* If stopping a unit fails continuously we might enter a stop - * loop here, hence stop acting on the service being - * unnecessary after a while. */ - if (!ratelimit_below(&u->auto_stop_ratelimit)) { - log_unit_warning(u, "Unit not needed anymore, but not stopping since we tried this too often recently."); - return; + return true; +} + +static void check_unneeded_dependencies(Unit *u) { + + static const UnitDependency deps[] = { + UNIT_REQUIRES, + UNIT_REQUISITE, + UNIT_WANTS, + UNIT_BINDS_TO, + }; + size_t j; + + assert(u); + + /* Add all units this unit depends on to the queue that processes StopWhenUnneeded= behaviour. */ + + for (j = 0; j < ELEMENTSOF(deps); j++) { + Unit *other; + Iterator i; + void *v; + + HASHMAP_FOREACH_KEY(v, other, u->dependencies[deps[j]], i) + unit_add_to_stop_when_unneeded_queue(other); } - - log_unit_info(u, "Unit not needed anymore. Stopping."); - - /* Ok, nobody needs us anymore. Sniff. Then let's commit suicide */ - r = manager_add_job(u->manager, JOB_STOP, u, JOB_FAIL, &error, NULL); - if (r < 0) - log_unit_warning_errno(u, r, "Failed to enqueue stop job, ignoring: %s", bus_error_message(&error, r)); } static void unit_check_binds_to(Unit *u) { @@ -2098,29 +2133,6 @@ static void retroactively_stop_dependencies(Unit *u) { manager_add_job(u->manager, JOB_STOP, other, JOB_REPLACE, NULL, NULL); } -static void check_unneeded_dependencies(Unit *u) { - Unit *other; - Iterator i; - void *v; - - assert(u); - assert(UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(u))); - - /* Garbage collect services that might not be needed anymore, if enabled */ - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_REQUIRES], i) - if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other))) - unit_check_unneeded(other); - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_WANTS], i) - if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other))) - unit_check_unneeded(other); - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_REQUISITE], i) - if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other))) - unit_check_unneeded(other); - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_BINDS_TO], i) - if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other))) - unit_check_unneeded(other); -} - void unit_start_on_failure(Unit *u) { Unit *other; Iterator i; @@ -2423,7 +2435,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag } /* stop unneeded units regardless if going down was expected or not */ - if (UNIT_IS_INACTIVE_OR_DEACTIVATING(ns)) + if (UNIT_IS_INACTIVE_OR_FAILED(ns)) check_unneeded_dependencies(u); if (ns != os && ns == UNIT_FAILED) { @@ -2483,7 +2495,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag if (!MANAGER_IS_RELOADING(u->manager)) { /* Maybe we finished startup and are now ready for being stopped because unneeded? */ - unit_check_unneeded(u); + unit_add_to_stop_when_unneeded_queue(u); /* Maybe we finished startup, but something we needed has vanished? Let's die then. (This happens when * something BindsTo= to a Type=oneshot unit, as these units go directly from starting to inactive, diff --git a/src/core/unit.h b/src/core/unit.h index a9de983152..e14cfb8173 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -212,6 +212,9 @@ typedef struct Unit { /* Target dependencies queue */ LIST_FIELDS(Unit, target_deps_queue); + /* Queue of units with StopWhenUnneeded set that shell be checked for clean-up. */ + LIST_FIELDS(Unit, stop_when_unneeded_queue); + /* PIDs we keep an eye on. Note that a unit might have many * more, but these are the ones we care enough about to * process SIGCHLD for */ @@ -322,6 +325,7 @@ typedef struct Unit { bool in_cgroup_realize_queue:1; bool in_cgroup_empty_queue:1; bool in_target_deps_queue:1; + bool in_stop_when_unneeded_queue:1; bool sent_dbus_new_signal:1; @@ -613,6 +617,7 @@ void unit_add_to_dbus_queue(Unit *u); void unit_add_to_cleanup_queue(Unit *u); void unit_add_to_gc_queue(Unit *u); void unit_add_to_target_deps_queue(Unit *u); +void unit_add_to_stop_when_unneeded_queue(Unit *u); int unit_merge(Unit *u, Unit *other); int unit_merge_by_name(Unit *u, const char *other); @@ -749,6 +754,8 @@ bool unit_type_supported(UnitType t); bool unit_is_pristine(Unit *u); +bool unit_is_unneeded(Unit *u); + pid_t unit_control_pid(Unit *u); pid_t unit_main_pid(Unit *u);