From db868d45f9a50a63a03231e0f475baebd87dab82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 28 May 2020 14:09:43 +0200 Subject: [PATCH 1/4] core: make unit_set_invocation_id static No functional change. --- src/core/unit.c | 74 ++++++++++++++++++++++++------------------------- src/core/unit.h | 1 - 2 files changed, 37 insertions(+), 38 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index b287c77117..bfdac7842b 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -3205,6 +3205,43 @@ char *unit_dbus_path_invocation_id(Unit *u) { return unit_dbus_path_from_name(u->invocation_id_string); } +static int unit_set_invocation_id(Unit *u, sd_id128_t id) { + int r; + + assert(u); + + /* Set the invocation ID for this unit. If we cannot, this will not roll back, but reset the whole thing. */ + + if (sd_id128_equal(u->invocation_id, id)) + return 0; + + if (!sd_id128_is_null(u->invocation_id)) + (void) hashmap_remove_value(u->manager->units_by_invocation_id, &u->invocation_id, u); + + if (sd_id128_is_null(id)) { + r = 0; + goto reset; + } + + r = hashmap_ensure_allocated(&u->manager->units_by_invocation_id, &id128_hash_ops); + if (r < 0) + goto reset; + + u->invocation_id = id; + sd_id128_to_string(id, u->invocation_id_string); + + r = hashmap_put(u->manager->units_by_invocation_id, &u->invocation_id, u); + if (r < 0) + goto reset; + + return 0; + +reset: + u->invocation_id = SD_ID128_NULL; + u->invocation_id_string[0] = 0; + return r; +} + int unit_set_slice(Unit *u, Unit *slice) { assert(u); assert(slice); @@ -5344,43 +5381,6 @@ void unit_notify_user_lookup(Unit *u, uid_t uid, gid_t gid) { unit_add_to_dbus_queue(u); } -int unit_set_invocation_id(Unit *u, sd_id128_t id) { - int r; - - assert(u); - - /* Set the invocation ID for this unit. If we cannot, this will not roll back, but reset the whole thing. */ - - if (sd_id128_equal(u->invocation_id, id)) - return 0; - - if (!sd_id128_is_null(u->invocation_id)) - (void) hashmap_remove_value(u->manager->units_by_invocation_id, &u->invocation_id, u); - - if (sd_id128_is_null(id)) { - r = 0; - goto reset; - } - - r = hashmap_ensure_allocated(&u->manager->units_by_invocation_id, &id128_hash_ops); - if (r < 0) - goto reset; - - u->invocation_id = id; - sd_id128_to_string(id, u->invocation_id_string); - - r = hashmap_put(u->manager->units_by_invocation_id, &u->invocation_id, u); - if (r < 0) - goto reset; - - return 0; - -reset: - u->invocation_id = SD_ID128_NULL; - u->invocation_id_string[0] = 0; - return r; -} - int unit_acquire_invocation_id(Unit *u) { sd_id128_t id; int r; diff --git a/src/core/unit.h b/src/core/unit.h index 6659406c2d..252a98eaff 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -829,7 +829,6 @@ void unit_unref_uid_gid(Unit *u, bool destroy_now); void unit_notify_user_lookup(Unit *u, uid_t uid, gid_t gid); -int unit_set_invocation_id(Unit *u, sd_id128_t id); int unit_acquire_invocation_id(Unit *u); bool unit_shall_confirm_spawn(Unit *u); From 3fb2326f3ed87aa0b26078d307ebfb299e36286d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 28 May 2020 14:58:35 +0200 Subject: [PATCH 2/4] shared/unit-file: make sure the old hashmaps and sets are freed upon replacement Possibly fixes #15220. (There might be another leak. I'm still investigating.) The leak would occur when the path cache was rebuilt. So in normal circumstances it wouldn't be too bad, since usually the path cache is not rebuilt too often. But the case in #15220, where new unit files are created in a loop and started, the leak occurs once for each unit file: $ for i in {1..300}; do cp ~/.config/systemd/user/test0001.service ~/.config/systemd/user/test$(printf %04d $i).service; systemctl --user start test$(printf %04d $i).service;done --- src/basic/hash-funcs.c | 2 ++ src/basic/hash-funcs.h | 1 + src/basic/hashmap.h | 8 ++++++++ src/basic/set.h | 8 ++++++++ src/core/manager.c | 2 +- src/shared/unit-file.c | 24 +++++++++++++----------- 6 files changed, 33 insertions(+), 12 deletions(-) diff --git a/src/basic/hash-funcs.c b/src/basic/hash-funcs.c index fee0ba98eb..cf279e5cbe 100644 --- a/src/basic/hash-funcs.c +++ b/src/basic/hash-funcs.c @@ -55,6 +55,8 @@ void path_hash_func(const char *q, struct siphash *state) { } DEFINE_HASH_OPS(path_hash_ops, char, path_hash_func, path_compare); +DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR(path_hash_ops_free, + char, path_hash_func, path_compare, free); void trivial_hash_func(const void *p, struct siphash *state) { siphash24_compress(&p, sizeof(p), state); diff --git a/src/basic/hash-funcs.h b/src/basic/hash-funcs.h index 264316c3dc..005d1b21d2 100644 --- a/src/basic/hash-funcs.h +++ b/src/basic/hash-funcs.h @@ -81,6 +81,7 @@ extern const struct hash_ops string_hash_ops_free_free; void path_hash_func(const char *p, struct siphash *state); extern const struct hash_ops path_hash_ops; +extern const struct hash_ops path_hash_ops_free; /* This will compare the passed pointers directly, and will not dereference them. This is hence not useful for strings * or suchlike. */ diff --git a/src/basic/hashmap.h b/src/basic/hashmap.h index 65adc92513..a3bc328142 100644 --- a/src/basic/hashmap.h +++ b/src/basic/hashmap.h @@ -89,6 +89,14 @@ OrderedHashmap *internal_ordered_hashmap_new(const struct hash_ops *hash_ops HA #define hashmap_new(ops) internal_hashmap_new(ops HASHMAP_DEBUG_SRC_ARGS) #define ordered_hashmap_new(ops) internal_ordered_hashmap_new(ops HASHMAP_DEBUG_SRC_ARGS) +#define hashmap_free_and_replace(a, b) \ + ({ \ + hashmap_free(a); \ + (a) = (b); \ + (b) = NULL; \ + 0; \ + }) + HashmapBase *internal_hashmap_free(HashmapBase *h, free_func_t default_free_key, free_func_t default_free_value); static inline Hashmap *hashmap_free(Hashmap *h) { return (void*) internal_hashmap_free(HASHMAP_BASE(h), NULL, NULL); diff --git a/src/basic/set.h b/src/basic/set.h index f3501d17ae..8a95fec05f 100644 --- a/src/basic/set.h +++ b/src/basic/set.h @@ -5,6 +5,14 @@ #include "hashmap.h" #include "macro.h" +#define set_free_and_replace(a, b) \ + ({ \ + set_free(a); \ + (a) = (b); \ + (b) = NULL; \ + 0; \ + }) + Set *internal_set_new(const struct hash_ops *hash_ops HASHMAP_DEBUG_PARAMS); #define set_new(ops) internal_set_new(ops HASHMAP_DEBUG_SRC_ARGS) diff --git a/src/core/manager.c b/src/core/manager.c index f15f845e81..7acbbb0b9e 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -696,7 +696,7 @@ static int manager_setup_prefix(Manager *m) { static void manager_free_unit_name_maps(Manager *m) { m->unit_id_map = hashmap_free(m->unit_id_map); m->unit_name_map = hashmap_free(m->unit_name_map); - m->unit_path_cache = set_free_free(m->unit_path_cache); + m->unit_path_cache = set_free(m->unit_path_cache); m->unit_cache_mtime = 0; } diff --git a/src/shared/unit-file.c b/src/shared/unit-file.c index 4fe2489c55..10968e18ca 100644 --- a/src/shared/unit-file.c +++ b/src/shared/unit-file.c @@ -229,9 +229,9 @@ static bool lookup_paths_mtime_good(const LookupPaths *lp, usec_t mtime) { int unit_file_build_name_map( const LookupPaths *lp, usec_t *cache_mtime, - Hashmap **ret_unit_ids_map, - Hashmap **ret_unit_names_map, - Set **ret_path_cache) { + Hashmap **unit_ids_map, + Hashmap **unit_names_map, + Set **path_cache) { /* Build two mappings: any name → main unit (i.e. the end result of symlink resolution), unit name → * all aliases (i.e. the entry for a given key is a a list of all names which point to this key). The @@ -239,7 +239,8 @@ int unit_file_build_name_map( * have a key, but it is not present in the value for itself, there was an alias pointing to it, but * the unit itself is not loadable. * - * At the same, build a cache of paths where to find units. + * At the same, build a cache of paths where to find units. The non-const parameters are for input + * and output. Existing contents will be freed before the new contents are stored. */ _cleanup_hashmap_free_ Hashmap *ids = NULL, *names = NULL; @@ -253,8 +254,8 @@ int unit_file_build_name_map( if (cache_mtime && *cache_mtime > 0 && lookup_paths_mtime_good(lp, *cache_mtime)) return 0; - if (ret_path_cache) { - paths = set_new(&path_hash_ops); + if (path_cache) { + paths = set_new(&path_hash_ops_free); if (!paths) return log_oom(); } @@ -296,7 +297,7 @@ int unit_file_build_name_map( if (!filename) return log_oom(); - if (ret_path_cache) { + if (paths) { r = set_consume(paths, filename); if (r < 0) return log_oom(); @@ -418,10 +419,11 @@ int unit_file_build_name_map( if (cache_mtime) *cache_mtime = mtime; - *ret_unit_ids_map = TAKE_PTR(ids); - *ret_unit_names_map = TAKE_PTR(names); - if (ret_path_cache) - *ret_path_cache = TAKE_PTR(paths); + + hashmap_free_and_replace(*unit_ids_map, ids); + hashmap_free_and_replace(*unit_names_map, names); + if (path_cache) + set_free_and_replace(*path_cache, paths); return 1; } From f6173cb955e85c485a2005f02651004d36c1fb57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 28 May 2020 15:25:22 +0200 Subject: [PATCH 3/4] core: define UnitDependency iterators in loops Reduced scope of variables is always nice. --- src/core/unit.c | 43 ++++++++++++++----------------------------- 1 file changed, 14 insertions(+), 29 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index bfdac7842b..e1eb6ac786 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -501,9 +501,7 @@ static void bidi_set_free(Unit *u, Hashmap *h) { /* Frees the hashmap and makes sure we are dropped from the inverse pointers */ HASHMAP_FOREACH_KEY(v, other, h, i) { - UnitDependency d; - - for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) + for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) hashmap_remove(other->dependencies[d], u); unit_add_to_gc_queue(other); @@ -599,7 +597,6 @@ static void unit_done(Unit *u) { } void unit_free(Unit *u) { - UnitDependency d; Iterator i; char *t; @@ -650,7 +647,7 @@ void unit_free(Unit *u) { job_free(j); } - for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) + for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) bidi_set_free(u, u->dependencies[d]); if (u->on_console) @@ -875,19 +872,17 @@ static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitD assert(d < _UNIT_DEPENDENCY_MAX); /* Fix backwards pointers. Let's iterate through all dependent units of the other unit. */ - HASHMAP_FOREACH_KEY(v, back, other->dependencies[d], i) { - UnitDependency k; + HASHMAP_FOREACH_KEY(v, back, other->dependencies[d], i) - /* Let's now iterate through the dependencies of that dependencies of the other units, looking for - * pointers back, and let's fix them up, to instead point to 'u'. */ - - for (k = 0; k < _UNIT_DEPENDENCY_MAX; k++) { + /* Let's now iterate through the dependencies of that dependencies of the other units, + * looking for pointers back, and let's fix them up, to instead point to 'u'. */ + for (UnitDependency k = 0; k < _UNIT_DEPENDENCY_MAX; k++) if (back == u) { /* Do not add dependencies between u and itself. */ if (hashmap_remove(back->dependencies[k], other)) maybe_warn_about_dependency(u, other_id, k); } else { - UnitDependencyInfo di_u, di_other, di_merged; + UnitDependencyInfo di_u, di_other; /* Let's drop this dependency between "back" and "other", and let's create it between * "back" and "u" instead. Let's merge the bit masks of the dependency we are moving, @@ -899,7 +894,7 @@ static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitD di_u.data = hashmap_get(back->dependencies[k], u); - di_merged = (UnitDependencyInfo) { + UnitDependencyInfo di_merged = { .origin_mask = di_u.origin_mask | di_other.origin_mask, .destination_mask = di_u.destination_mask | di_other.destination_mask, }; @@ -911,9 +906,6 @@ static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitD /* assert_se(hashmap_remove_and_replace(back->dependencies[k], other, u, di_merged.data) >= 0); */ } - } - - } /* Also do not move dependencies on u to itself */ back = hashmap_remove(other->dependencies[d], u); @@ -927,7 +919,6 @@ static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitD } int unit_merge(Unit *u, Unit *other) { - UnitDependency d; const char *other_id = NULL; int r; @@ -966,7 +957,7 @@ int unit_merge(Unit *u, Unit *other) { other_id = strdupa(other->id); /* Make reservations to ensure merge_dependencies() won't fail */ - for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) { + for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) { r = reserve_dependencies(u, other, d); /* * We don't rollback reservations if we fail. We don't have @@ -986,7 +977,7 @@ int unit_merge(Unit *u, Unit *other) { unit_ref_set(other->refs_by_target, other->refs_by_target->source, u); /* Merge dependencies */ - for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) + for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) merge_dependencies(u, other, other_id, d); other->load_state = UNIT_MERGED; @@ -1221,7 +1212,6 @@ static void print_unit_dependency_mask(FILE *f, const char *kind, UnitDependency void unit_dump(Unit *u, FILE *f, const char *prefix) { char *t, **j; - UnitDependency d; Iterator i; const char *prefix2; char timestamp[5][FORMAT_TIMESTAMP_MAX], timespan[FORMAT_TIMESPAN_MAX]; @@ -1377,7 +1367,7 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { prefix, strna(format_timestamp(timestamp[0], sizeof(timestamp[0]), u->assert_timestamp.realtime)), prefix, yes_no(u->assert_result)); - for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) { + for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) { UnitDependencyInfo di; Unit *other; @@ -1509,7 +1499,6 @@ int unit_add_default_target_dependency(Unit *u, Unit *target) { } static int unit_add_slice_dependencies(Unit *u) { - UnitDependencyMask mask; assert(u); if (!UNIT_HAS_CGROUP_CONTEXT(u)) @@ -1518,7 +1507,7 @@ static int unit_add_slice_dependencies(Unit *u) { /* Slice units are implicitly ordered against their parent slices (as this relationship is encoded in the name), while all other units are ordered based on configuration (as in their case Slice= configures the relationship). */ - mask = u->type == UNIT_SLICE ? UNIT_DEPENDENCY_IMPLICIT : UNIT_DEPENDENCY_FILE; + UnitDependencyMask mask = u->type == UNIT_SLICE ? UNIT_DEPENDENCY_IMPLICIT : UNIT_DEPENDENCY_FILE; if (UNIT_ISSET(u->slice)) return unit_add_two_dependencies(u, UNIT_AFTER, UNIT_REQUIRES, UNIT_DEREF(u->slice), true, mask); @@ -5502,8 +5491,6 @@ static void unit_update_dependency_mask(Unit *u, UnitDependency d, Unit *other, } void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) { - UnitDependency d; - assert(u); /* Removes all dependencies u has on other units marked for ownership by 'mask'. */ @@ -5511,7 +5498,7 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) { if (mask == 0) return; - for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) { + for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) { bool done; do { @@ -5522,8 +5509,6 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) { done = true; HASHMAP_FOREACH_KEY(di.data, other, u->dependencies[d], i) { - UnitDependency q; - if ((di.origin_mask & ~mask) == di.origin_mask) continue; di.origin_mask &= ~mask; @@ -5534,7 +5519,7 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) { * all dependency types on the other unit and delete all those which point to us and * have the right mask set. */ - for (q = 0; q < _UNIT_DEPENDENCY_MAX; q++) { + for (UnitDependency q = 0; q < _UNIT_DEPENDENCY_MAX; q++) { UnitDependencyInfo dj; dj.data = hashmap_get(other->dependencies[q], u); From a4ac27c1af5fa055a7c71729f71dce0b16cf5385 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 28 May 2020 18:39:27 +0200 Subject: [PATCH 4/4] manager: free the jobs hashmap after we have no jobs After a larger transaction, e.g. after bootup, we're left with an empty hashmap with hundreds of buckets. Long-term, it'd be better to size hashmaps down when they are less than 1/4 full, but even if we implement that, jobs hashmap is likely to be empty almost always, so it seems useful to deallocate it once the jobs count reaches 0. --- src/core/job.c | 4 ++++ src/core/manager.c | 9 +++++---- src/core/transaction.c | 4 ++++ 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/core/job.c b/src/core/job.c index cf3bca88d6..d518ac8969 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -263,6 +263,10 @@ int job_install_deserialized(Job *j) { return log_unit_debug_errno(j->unit, SYNTHETIC_ERRNO(EEXIST), "Unit already has a job installed. Not installing deserialized job."); + r = hashmap_ensure_allocated(&j->manager->jobs, NULL); + if (r < 0) + return r; + r = hashmap_put(j->manager->jobs, UINT32_TO_PTR(j->id), j); if (r == -EEXIST) return log_unit_debug_errno(j->unit, r, "Job ID %" PRIu32 " already used, cannot deserialize job.", j->id); diff --git a/src/core/manager.c b/src/core/manager.c index 7acbbb0b9e..3659bb0d59 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -833,10 +833,6 @@ int manager_new(UnitFileScope scope, ManagerTestRunFlags test_run_flags, Manager if (r < 0) return r; - r = hashmap_ensure_allocated(&m->jobs, NULL); - if (r < 0) - return r; - r = hashmap_ensure_allocated(&m->cgroup_unit, &path_hash_ops); if (r < 0) return r; @@ -3963,6 +3959,11 @@ void manager_check_finished(Manager *m) { return; } + /* The jobs hashmap tends to grow a lot during boot, and then it's not reused until shutdown. Let's + kill the hashmap if it is relatively large. */ + if (hashmap_buckets(m->jobs) > hashmap_size(m->units) / 10) + m->jobs = hashmap_free(m->jobs); + manager_flip_auto_status(m, false, "boot finished"); /* Notify Type=idle units that we are done now */ diff --git a/src/core/transaction.c b/src/core/transaction.c index 6f614a32dc..0fb52372f1 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -656,6 +656,10 @@ static int transaction_apply( assert(!j->transaction_prev); assert(!j->transaction_next); + r = hashmap_ensure_allocated(&m->jobs, NULL); + if (r < 0) + return r; + r = hashmap_put(m->jobs, UINT32_TO_PTR(j->id), j); if (r < 0) goto rollback;