diff mbox series

[wrynose] systemd: backport drop-in lookup optimization

Message ID 20260910023338.1422939-1-wenwfu@qti.qualcomm.com
State New
Headers show
Series [wrynose] systemd: backport drop-in lookup optimization | expand

Commit Message

Wenwen Fu Sept. 10, 2026, 2:33 a.m. UTC
Systemd re-derived drop-in name candidates for every lookup directory
while enumerating units, adding repeated parsing and allocations to the
boot path.

Backport the upstream optimization to the wrynose branch. Adapt it for
systemd 259.5 and its older unit path cache and configuration file APIs.

Assisted-by: Codex:GPT-5
Signed-off-by: Wenwen Fu <wenwfu@qti.qualcomm.com>
---
 ...-t-re-derive-drop-in-name-candidates.patch | 249 ++++++++++++++++++
 meta/recipes-core/systemd/systemd_259.5.bb    |   1 +
 2 files changed, 250 insertions(+)
 create mode 100644 meta/recipes-core/systemd/systemd/0001-shared-dropin-don-t-re-derive-drop-in-name-candidates.patch

Comments

Yoann Congal Sept. 10, 2026, 1:53 p.m. UTC | #1
On Thu Sep 10, 2026 at 4:33 AM CEST, Wenwen Fu via lists.openembedded.org wrote:
> Systemd re-derived drop-in name candidates for every lookup directory
> while enumerating units, adding repeated parsing and allocations to the
> boot path.
>
> Backport the upstream optimization to the wrynose branch. Adapt it for
> systemd 259.5 and its older unit path cache and configuration file APIs.
>
> Assisted-by: Codex:GPT-5
> Signed-off-by: Wenwen Fu <wenwfu@qti.qualcomm.com>
> ---
>  ...-t-re-derive-drop-in-name-candidates.patch | 249 ++++++++++++++++++
>  meta/recipes-core/systemd/systemd_259.5.bb    |   1 +
>  2 files changed, 250 insertions(+)
>  create mode 100644 meta/recipes-core/systemd/systemd/0001-shared-dropin-don-t-re-derive-drop-in-name-candidates.patch

Hello,

Optimisations are not acceptable under stable policy[0], so, I can't
merge this, sorry.

[0]: https://wiki.yoctoproject.org/wiki/Stable_Release_and_LTS#Stable/LTS_Patch_Acceptance_Policies

Regards,
diff mbox series

Patch

diff --git a/meta/recipes-core/systemd/systemd/0001-shared-dropin-don-t-re-derive-drop-in-name-candidates.patch b/meta/recipes-core/systemd/systemd/0001-shared-dropin-don-t-re-derive-drop-in-name-candidates.patch
new file mode 100644
index 0000000..95a275d
--- /dev/null
+++ b/meta/recipes-core/systemd/systemd/0001-shared-dropin-don-t-re-derive-drop-in-name-candidates.patch
@@ -0,0 +1,249 @@ 
+From 5ef2510988829c12cfbbe60e614bff3e3b0f97f9 Mon Sep 17 00:00:00 2001
+From: Eric Curtin <eric.curtin@docker.com>
+Date: Thu, 2 Jul 2026 13:37:06 +0100
+Subject: [PATCH] shared/dropin: don't re-derive drop-in name candidates per
+ lookup dir
+
+unit_file_find_dirs() is called once for every (unit name or alias,
+lookup directory, drop-in suffix) combination while enumerating units
+at boot, to check whether that unit has a ".d", ".wants", ".requires"
+or ".upholds" drop-in directory in that particular lookup path. On a
+typical system with ~270 loaded units and ~12 directories in the unit
+search path, this adds up to tens of thousands of calls.
+
+For every one of those calls, the function used to independently
+re-derive the full chain of candidate unit names to check for that one
+directory: the name itself, its template if it is a template instance,
+and its "-" prefix chain (e.g. for "foo-bar-waldo.service" also
+"foo-bar-.service" and "foo-.service"), recursively expanding further
+where applicable. That derivation only depends on the unit name itself
+and does not involve the lookup directory at all, so it produces the
+exact same list of candidate names regardless of which of the 12
+lookup directories is currently being checked. Despite this, it was
+being fully recomputed for every single directory, doing several small
+allocations and unit-name parsing calls (unit_name_template(),
+unit_name_to_prefix(), unit_name_build_from_type(), ...) each time.
+
+Split the name-derivation logic out into its own function,
+unit_file_expand_dropin_names(), and compute it once per unit
+name/alias, then reuse the resulting candidate list across all lookup
+directories instead of re-deriving it for each of them. The order in
+which candidate directories end up being added is unchanged, so this
+is not expected to alter drop-in resolution behaviour: I confirmed this
+by comparing the sorted unit load state, fragment path and drop-in path
+output of "systemd --test --system" before and after this change on the
+same unit tree, which is byte-for-byte identical.
+
+I measured the effect by instrumenting manager_enumerate() with
+CLOCK_MONOTONIC timestamps and running systemd, built from this exact
+tree, as actual PID 1 in a container with ~270 real units loaded, 50
+runs each before and after this change:
+
+  before: mean 45.35ms (stddev 0.60ms)
+  after:  mean 38.81ms (stddev 0.81ms)
+
+a ~14% reduction with about 8 standard deviations of separation between
+the two distributions, i.e. well outside of run-to-run noise.
+
+unit_file_expand_dropin_names()'s out parameter is renamed from
+ret_names to names, since it is appended to (including recursively)
+rather than only being populated on success, matching the ret_ naming
+convention used elsewhere for output-only parameters. Also, a failure
+partway through expanding a name's candidates (e.g. OOM) no longer
+discards the candidates already derived before the failure, keeping
+unit_file_find_dirs() closer to the original recursive
+implementation's error handling.
+
+unit_file_add_dir_if_exists(), which builds the path to check for each
+(lookup directory, candidate name) pair, is now the hottest remaining
+part of this code: with the per-directory re-derivation gone, it is
+called once for every directory/candidate combination instead of once
+per candidate. It used to build that path with strjoin(name, suffix)
+followed by path_join(unit_path, name_and_suffix), i.e. two heap
+allocations plus path_join()'s normalization pass. Lookup paths are
+already normalized (path_simplify() + strv_uniq()), so a single
+strjoin(unit_path, "/", name, suffix) produces the same string while
+halving the allocations and skipping the redundant normalization.
+
+Upstream-Status: Backport [https://github.com/systemd/systemd/commit/5ef2510988829c12cfbbe60e614bff3e3b0f97f9]
+
+---
+index f540f48..730cf3f 100644
+--- a/src/shared/dropin.c
++++ b/src/shared/dropin.c
+@@ -140,32 +140,30 @@ static int unit_file_add_dir(
+         return 0;
+ }
+ 
+-static int unit_file_find_dirs(
+-                const char *original_root,
+-                Set *unit_path_cache,
+-                const char *unit_path,
+-                const char *name,
+-                const char *suffix,
+-                char ***dirs) {
+-
++/* Expands a unit name into the ordered list of candidate unit names whose drop-in directories should be
++ * considered for it: the name itself, its template (if it's an instance), and its "-" prefix chain (e.g.
++ * given "foo-bar-waldo.service" also "foo-bar-.service" and "foo-.service"), each of which may again expand
++ * further via the same rules (e.g. a templated prefix).
++ *
++ * This only depends on the unit name itself, not on any particular lookup directory. Search paths usually
++ * contain several directories, and each unit is looked up under several different suffixes (".d", ".wants",
++ * ".requires", ".upholds"), so callers are expected to compute this list once per (name, aliases) and reuse
++ * it across every directory/suffix combination, rather than re-deriving it (which requires several small
++ * allocations and unit-name parsing calls) again for each one. */
++static int unit_file_expand_dropin_names(const char *name, char ***names) {
+         _cleanup_free_ char *prefix = NULL, *instance = NULL, *built = NULL;
+         bool is_instance, chopped;
+         const char *dash;
+         UnitType type;
+-        char *path;
+         size_t n;
+         int r;
+ 
+-        assert(unit_path);
+         assert(name);
+-        assert(suffix);
++        assert(names);
+ 
+-        path = strjoina(unit_path, "/", name, suffix);
+-        if (!unit_path_cache || set_get(unit_path_cache, path)) {
+-                r = unit_file_add_dir(original_root, path, dirs);
+-                if (r < 0)
+-                        return r;
+-        }
++        r = strv_extend(names, name);
++        if (r < 0)
++                return log_oom();
+ 
+         is_instance = unit_name_is_valid(name, UNIT_NAME_INSTANCE);
+         if (is_instance) { /* Also try the template dir */
+@@ -175,7 +173,7 @@ static int unit_file_find_dirs(
+                 if (r < 0)
+                         return log_error_errno(r, "Failed to generate template from unit name: %m");
+ 
+-                r = unit_file_find_dirs(original_root, unit_path_cache, unit_path, template, suffix, dirs);
++                r = unit_file_expand_dropin_names(template, names);
+                 if (r < 0)
+                         return r;
+         }
+@@ -234,7 +232,72 @@ static int unit_file_find_dirs(
+         if (r < 0)
+                 return log_error_errno(r, "Failed to build prefix unit name: %m");
+ 
+-        return unit_file_find_dirs(original_root, unit_path_cache, unit_path, built, suffix, dirs);
++        return unit_file_expand_dropin_names(built, names);
++}
++
++/* Checks whether [original_root]/unit_path/name+suffix exists (consulting unit_path_cache first, if
++ * given), and if so adds it to dirs. */
++static int unit_file_add_dir_if_exists(
++                const char *original_root,
++                Set *unit_path_cache,
++                const char *unit_path,
++                const char *name,
++                const char *suffix,
++                char ***dirs) {
++
++        _cleanup_free_ char *path = NULL;
++
++        assert(unit_path);
++        assert(name);
++        assert(suffix);
++
++        /* Lookup paths are already normalized (path_simplify() + strv_uniq()), so a plain concatenation
++         * here produces the same string path_join() would, without the extra allocation and
++         * normalization pass. */
++        path = strjoin(unit_path, "/", name, suffix);
++        if (!path)
++                return log_oom();
++
++        if (unit_path_cache && !set_contains(unit_path_cache, path))
++                return 0;
++
++        return unit_file_add_dir(original_root, path, dirs);
++}
++
++static int unit_file_find_dirs(
++                const char *original_root,
++                Set *unit_path_cache,
++                char **lookup_path,
++                const char *name,
++                const char *suffix,
++                char ***dirs) {
++
++        _cleanup_strv_free_ char **candidates = NULL;
++        int r;
++
++        assert(name);
++        assert(suffix);
++
++        r = unit_file_expand_dropin_names(name, &candidates);
++        if (r < 0 && strv_isempty(candidates))
++                return r;
++
++        /* Even if expansion failed partway through (e.g. OOM, or a malformed candidate derived from the
++         * unit name), still use whatever candidates were already derived before the failure, rather than
++         * discarding all of them: this keeps us closer to the original recursive implementation, where a
++         * failure deriving a less specific candidate didn't undo drop-ins already found for more specific
++         * ones.
++         *
++         * Similarly, a failure while checking one candidate directory (e.g. OOM, or an unexpected chase()
++         * error) only aborts the (increasingly less specific) remaining candidates for the lookup
++         * directory we were currently looking at, matching the original recursive implementation's error
++         * handling. Other lookup directories are still tried independently. */
++        STRV_FOREACH(p, lookup_path)
++                STRV_FOREACH(c, candidates)
++                        if (unit_file_add_dir_if_exists(original_root, unit_path_cache, *p, *c, suffix, dirs) < 0)
++                                break;
++
++        return 0;
+ }
+ 
+ int unit_file_find_dropin_paths(
+@@ -254,12 +317,10 @@ int unit_file_find_dropin_paths(
+         assert(ret);
+ 
+         if (name)
+-                STRV_FOREACH(p, lookup_path)
+-                        (void) unit_file_find_dirs(original_root, unit_path_cache, *p, name, dir_suffix, &dirs);
++                (void) unit_file_find_dirs(original_root, unit_path_cache, lookup_path, name, dir_suffix, &dirs);
+ 
+         SET_FOREACH(n, aliases)
+-                STRV_FOREACH(p, lookup_path)
+-                        (void) unit_file_find_dirs(original_root, unit_path_cache, *p, n, dir_suffix, &dirs);
++                (void) unit_file_find_dirs(original_root, unit_path_cache, lookup_path, n, dir_suffix, &dirs);
+ 
+         /* All the names in the unit are of the same type so just grab one. */
+         n = name ?: (const char*) set_first(aliases);
+@@ -273,13 +334,12 @@ int unit_file_find_dropin_paths(
+ 
+                 /* Special top level drop in for "<unit type>.<suffix>". Add this last as it's the most generic
+                  * and should be able to be overridden by more specific drop-ins. */
+-                STRV_FOREACH(p, lookup_path)
+-                        (void) unit_file_find_dirs(original_root,
+-                                                   unit_path_cache,
+-                                                   *p,
+-                                                   unit_type_to_string(type),
+-                                                   dir_suffix,
+-                                                   &dirs);
++                (void) unit_file_find_dirs(original_root,
++                                           unit_path_cache,
++                                           lookup_path,
++                                           unit_type_to_string(type),
++                                           dir_suffix,
++                                           &dirs);
+         }
+ 
+         if (strv_isempty(dirs)) {
+@@ -287,7 +347,7 @@ int unit_file_find_dropin_paths(
+                 return 0;
+         }
+ 
+-        r = conf_files_list_strv(ret, file_suffix, NULL, 0, (const char**) dirs);
++        r = conf_files_list_strv(ret, file_suffix, /* root= */ NULL, CONF_FILES_WARN, (const char**) dirs);
+         if (r < 0)
+                 return log_warning_errno(r, "Failed to create the list of configuration files: %m");
+ 
+
diff --git a/meta/recipes-core/systemd/systemd_259.5.bb b/meta/recipes-core/systemd/systemd_259.5.bb
index f3ec0ed..d3f9a1e 100644
--- a/meta/recipes-core/systemd/systemd_259.5.bb
+++ b/meta/recipes-core/systemd/systemd_259.5.bb
@@ -35,6 +35,7 @@  SRC_URI += " \
            file://0001-meson-use-libfido2_cflags-dependency.patch \
            file://0018-shared-fdset-add-detailed-debug-logging-to-fdset_new.patch \
            file://0004-tpm2-util-fix-PCR-bank-guessing-without-EFI.patch \
+           file://0001-shared-dropin-don-t-re-derive-drop-in-name-candidates.patch \
            "
 
 PAM_PLUGINS = " \