kdump-lib-initramfs: Fix performance regression in kdump_get_conf_val

[prudo1]

Rewriting kdump_get_conf_val in Bash lead to a massive performance regression. On my test system starting the kdump service took

$ time kdumpctl start
real 0m13.134s
user 0m8.828s
sys 0m7.450s

which is ~20 times slower compared to kdump-utils-1.0.59-1.fc44 with

$ time kdumpctl start
real 0m0.641s
user 0m0.208s
sys 0m0.538s

Looking at the traces shows that this is caused because Bash now has to handle the whole kdump.conf, including the extensive comment at the start, every time kdump_get_conf_val is called. This is done multiple times when starting the kdump service and is often cloaked by other functions, e.g. is_ssh_dump_target() or get_save_path().

To fix the issue remove comments and empty lines in a regex again so that the Bash code only has to handle valid config entries. With this change alone the performance is almost as good as the original version with

$ time kdumpctl start
real 0m0.780s
user 0m0.330s
sys 0m0.604s

In the long run it would make sense to also reduce the number of calls to kdump_get_conf_val.

This patch also fixes the issue that subsequent blanks are replaced by a single space. Usually this is not an issue but there are corner cases, e.g. in printf-like format strings passed as an argument, where the new behaviour is undesirable.

Fixes: d81109c ("kdump-lib-initramfs: rewrite kdump_get_conf_val")

[gemini-code-assist]

Summary of Changes

Hello @prudo1, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a critical performance bottleneck in the kdump_get_conf_val function, which was causing the kdump service to start very slowly. The core issue stemmed from inefficient Bash processing of the entire configuration file, including extensive comments. The solution involves optimizing the parsing logic by pre-filtering irrelevant lines, leading to a substantial speedup. Additionally, it rectifies an unintended modification of blank spaces, ensuring correct handling of sensitive string formats.

Highlights

• Performance Regression Fix: Addresses a significant performance regression in the kdump_get_conf_val function, which was introduced by a previous Bash rewrite. The kdumpctl start command, which previously took ~13 seconds, now completes in ~0.78 seconds, closely matching the original performance of ~0.64 seconds.
• Optimized Configuration Parsing: The performance improvement is achieved by pre-filtering comments and empty lines from the kdump.conf file using grep -Ev before the Bash script processes the configuration entries. This drastically reduces the amount of data the Bash loop has to handle.
• Fix for Blank Space Handling: Corrects an issue where subsequent blank spaces were inadvertently replaced by a single space. This fix is crucial for corner cases, such as printf-like format strings, where preserving multiple spaces is necessary.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request effectively addresses a significant performance regression in kdump_get_conf_val by pre-filtering the configuration file using grep. The implementation is solid, including the correct handling of subshells in pipelines. My review includes a couple of suggestions for further micro-optimizations within the loop to enhance performance by replacing external process calls with shell built-ins, which is in line with the goal of this PR.

[prudo1]

Added first suggestion from Gemini. Even though the expected performance impact should be tiny (comments at the end of the line should be rare) it's better to use the shell build-in functionality if possible.

The second suggestion of Gemini cannot be included as it is not POSIX compliant.

[licliu]

Thanks, @prudo1!
This patch looks good to me. Just a small nit: "\s" isn't strictly POSIX-compliant, though I don't think it's an issue here.

[coiby]

Hi @prudo1,

I'm surprised to find that kdump_get_conf_val can be called so many times,

# debug=1 bash -x /usr/bin/kdumpctl restart 2>&1 | grep -c "kdump_get_conf_val "
52

Currently the patch only filters out the comment lines. If your test only covers the default case which mean there are only 3 non-comment lines in the default kdump.conf, maybe you can add more options to /etc/kdump.conf to see how it performs. One interesting thing I notice if I add force_no_rebuild 1 to kdump.conf, it can significantly reduce the time of kdumpctl restart (1.1s vs 0.4s). Meanwhile calls of kdump_get_conf_val drops from 52 to 11.

Another issue with kdump_get_conf_val is it parses kdump.conf again and again whereas parse_config already parses the configs into the OPT array. parse_config is partially duplicating kdump_get_conf_val and I notice some inconsistencies between parse_config and kdump_get_conf_val, for example,

1. kdump_get_conf_val allows duplicated kdump config option but parse_config forbids it
2. kdump_get_conf_val reserve multiple spaces in a value but parse_config will squash multiple spaces into one

So ideally we should achieve the goal of only parsing once and to achieve this goal bash associative array is necessary. Unfortunately associative array is not POSIX. Personally I think we can switch to bash for kdump-lib-initramfs.sh because Fedora defaults to bash and I notice even changing the root's shell to dash, the kdump initramfs still uses bash. But if we can't make a quick decision and we need better performance, maybe something as follows can be useful,

--- a/kdump-lib-initramfs.sh
+++ b/kdump-lib-initramfs.sh
@@ -29,6 +29,11 @@ kdump_get_conf_val()
        _to_find="$1"
        _found=""
 
+        if ! $POSIX && ((${#OPT[@]})); then
+                echo -n "${OPT[$1]}"
+                return
+        fi

By the way I notice a problem (not caused by this patch) is cases like extra_bins "/usr/local/bin/my binary" or extra_bins "/usr/local/two spaces/executable" will fail.

[prudo1]

Hi @licliu,

Thanks, @prudo1! This patch looks good to me. Just a small nit: "\s" isn't strictly POSIX-compliant, though I don't think it's an issue here.

true, \s isn't POSIX-compliant. But [:space:] et al. are. They are described in section 9.3.5 "RE Bracket Expression" (in the draft for version 7 I'm using).

[prudo1]

Hi @coiby,

thanks for taking a look!

Let's start with the important things. I agree with you that more needs to be done. This patch isn't perfect though I deem it good enough for the moment. The only alternative I see is to revert d81109c ("kdump-lib-initramfs: rewrite kdump_get_conf_val") and re-add 1a73387 ("Strip surrounding quotes from configuration values"). But that will leave us with a rather nasty regex to maintain. So I'd prefer to keep this fix.

Either way, we should decide quickly. The current state is almost unusable. Taking into account that we also need to backport the fix to RHEL and the merge window closes soon. It would be best when the backport is done before the Chinese New Year holidays.

More below.

Hi @prudo1,

I'm surprised to find that kdump_get_conf_val can be called so many times,

# debug=1 bash -x /usr/bin/kdumpctl restart 2>&1 | grep -c "kdump_get_conf_val "
52

Currently the patch only filters out the comment lines. If your test only covers the default case which mean there are only 3 non-comment lines in the default kdump.conf, maybe you can add more options to /etc/kdump.conf to see how it performs. One interesting thing I notice if I add force_no_rebuild 1 to kdump.conf, it can significantly reduce the time of kdumpctl restart (1.1s vs 0.4s). Meanwhile calls of kdump_get_conf_val drops from 52 to 11.

On my test system I even found 63 calls to kdump_get_conf_val when the initrd had to be rebuild...

I've tried enabling more options in kdump.conf. The problem is that this will change the code that is executed. So comparing the times won't tell you much about how long kdump_get_conf_val took. I've also tried enabling more options with force_no_rebuild enabled. But that didn't change much. The run times I got were all within the variance you get at every run...

Another issue with kdump_get_conf_val is it parses kdump.conf again and again whereas parse_config already parses the configs into the OPT array. parse_config is partially duplicating kdump_get_conf_val and I notice some inconsistencies between parse_config and kdump_get_conf_val, for example,

1. kdump_get_conf_val allows duplicated kdump config option but parse_config forbids it

True, that's an inconsistency we should remove. No idea on what is the right behavior though...

2. kdump_get_conf_val reserve multiple spaces in a value but parse_config will squash multiple spaces into one

That's not entirely true. parse_config will only squash multiple spaces between the option and its value. But it will preserve multiple spaces within the value. For example in

core_collector makedumpfile <...>

the spaces are squashed but in

core_collector makedumpfile <...>

the spaces are preserved.

So ideally we should achieve the goal of only parsing once and to achieve this goal bash associative array is necessary. Unfortunately associative array is not POSIX. Personally I think we can switch to bash for kdump-lib-initramfs.sh because Fedora defaults to bash and I notice even changing the root's shell to dash, the kdump initramfs still uses bash. But if we can't make a quick decision and we need better performance, maybe something as follows can be useful,

--- a/kdump-lib-initramfs.sh
+++ b/kdump-lib-initramfs.sh
@@ -29,6 +29,11 @@ kdump_get_conf_val()
        _to_find="$1"
        _found=""
 
+        if ! $POSIX && ((${#OPT[@]})); then
+                echo -n "${OPT[$1]}"
+                return
+        fi

I agree with you that we should aim for only parsing the config once. Although my idea to achieve this is different. I've opened issue #139 for this. As I believe this will be a larger task I suggest we move the discussion there.

I've also opened #140 and #141, that should at least help us remove some calls to kdump_get_conf_val.

By the way I notice a problem (not caused by this patch) is cases like extra_bins "/usr/local/bin/my binary" or extra_bins "/usr/local/two spaces/executable" will fail.

True... I never thought about spaces in file names...

This makes me think that the 'problem' that was fixed with 1a73387 ("Strip surrounding quotes from configuration values") is actually the desired behavior.... I need to think about this more...

Thanks
Philipp

[coiby]

Hi @prudo1 Thanks for creating the new issues. Since the main problem is performance issue and I think PR fixes the issue without introducing any new problem, I'll merge the PR to give us enough time to come up with a long-term solution.