tools/bootconfig: Preserve event trigger filters in ftrace2bconf.sh

ftrace2bconf.sh reads each trigger line into "action active", keeping only its first whitespace-separated word. For example, a histogram with "if common_pid > 0" is exported without its filter. Reapplying that configuration installs an unconditional histogram instead.

Read the whole trigger line and remove only the trailing histogram state marker. Quote the resulting action so that whitespace in a filter is preserved in the exported configuration.

---
Hi Masami,

This is my third submission to the kernel. Feedback is welcome.

I tested this in a temporary tracefs instance on my Ubuntu machine, running kernel 7.0.0-38-generic, with recording disabled. I installed histograms with "if common_pid > 0" and 'if comm == "a b"', exported them, then restored that instance's actions with bconf2ftrace.sh --apply. The original exporter lost both filters. With this patch, both were restored correctly. An unfiltered histogram worked in both cases.

Thanks,
Eduardo

 tools/bootconfig/scripts/ftrace2bconf.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index f8f288f90c..0a066094f5 100755
@@ -115,8 +115,10 @@ per_event_options() { # event-dir
 		done
 		DEFINED_VARS="$DEFINED_VARS "`defined_vars $evdir`
 	fi
-	grep -v "^#" $evdir/trigger | while read action active; do
-		emit_kv $PREFIX.event.$group.$event.actions += \'$action\'
+	grep -v "^#" $evdir/trigger |
+		sed 's/ \[\(active\|paused\)\]$//' |
+		while IFS= read -r action; do
+		emit_kv $PREFIX.event.$group.$event.actions += "'$action'"
 	done
 
 	if [ $GROUP_ENABLED -eq 0 ] && event_is_enabled $evdir/enable; then

base-commit: 22ab3094befd6ef37135e4a252c0da9cd755f75a

2.53.0

Hi Eduardo,

Please make your patches as a series of patches. You have sent the below patch on the similar topic. Those should be one series.

https://lore.kernel.org/all/[email protected]/

Also, the \[\(foo\|bar\)\] seems gnu extension. Can you consider using more posix style like below?

- sed 's/ \[\(active\|paused\)\]$//' |
+ sed -e 's/ \[active\]$//' -e 's/ \[paused\]$//' |

Thank you,

Masami Hiramatsu (Google) <[email protected]>

Hi Masami,

Thanks for the review. I've combined the two fixes into a series and used the two sed expressions you suggested.

The first patch keeps filters when exporting event triggers. The second handles underscores in histogram variable names, so the exporter keeps histograms that depend on them.

Changes in v2:
- Send the two previously separate patches as one series.
- Use separate sed -e expressions for [active] and [paused], avoiding GNU sed's alternation extension in patch 1.
- No code changes to patch 2.

The original submissions are here:
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/

Feedback is welcome.

Thanks,
Eduardo

Eduardo Mucelli Rezende Oliveira (2):
tools/bootconfig: Preserve event trigger filters in ftrace2bconf.sh tools/bootconfig: Recognize underscores in histogram variables

 tools/bootconfig/scripts/ftrace2bconf.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

base-commit: e28458ef30ad66ce7cd2299ec5ee8a8a016d67ff

2.53.0

ftrace2bconf.sh reads each trigger line into "action active", keeping only its first whitespace-separated word. For example, a histogram with "if common_pid > 0" is exported without its filter. Reapplying that configuration installs an unconditional histogram instead.

Read the whole trigger line and remove only the trailing histogram state marker. Quote the resulting action so that whitespace in a filter is preserved in the exported configuration.

---
I tested this in a temporary tracefs instance on my Ubuntu machine, running kernel 7.0.0-38-generic, with recording disabled. I installed histograms with "if common_pid > 0" and 'if comm == "a b"', exported them, then restored that instance's actions with bconf2ftrace.sh --apply. The original exporter lost both filters. With this patch, both were restored correctly. An unfiltered histogram worked in both cases.

 tools/bootconfig/scripts/ftrace2bconf.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index f8f288f90c74..032ac5b10a4a 100755
@@ -115,8 +115,10 @@ per_event_options() { # event-dir
 		done
 		DEFINED_VARS="$DEFINED_VARS "`defined_vars $evdir`
 	fi
-	grep -v "^#" $evdir/trigger | while read action active; do
-		emit_kv $PREFIX.event.$group.$event.actions += \'$action\'
+	grep -v "^#" $evdir/trigger |
+		sed -e 's/ \[active\]$//' -e 's/ \[paused\]$//' |
+		while IFS= read -r action; do
+		emit_kv $PREFIX.event.$group.$event.actions += "'$action'"
 	done
 
 	if [ $GROUP_ENABLED -eq 0 ] && event_is_enabled $evdir/enable; then

2.53.0

The variable dependency resolver in ftrace2bconf.sh only recognizes letters and digits in histogram variable names. A reference such as $start_ts is read as $start, and the start_ts definition is missed. The consumer is repeatedly deferred and eventually omitted from the export with an undefined-variable diagnostic.

Include underscores when matching both definitions and references so that valid histogram variables are resolved and their consumers are exported after the providers.

---
The histogram-variable documentation even uses names such as wakeup_lat and us_per_sec [1]. These are valid in tracefs, but the exporter cannot resolve references to them and leaves out the dependent histogram.

I tested this in a temporary tracefs instance on my Ubuntu machine, running kernel 7.0.0-38-generic, with recording disabled. I created a provider histogram defining start_ts and a consumer using $start_ts. The original exporter reported an undefined variable and left out the consumer. With the fix, both were exported and restored correctly. A separate pair using ts0 worked in both versions.

[1] https://docs.kernel.org/trace/histogram.html#histogram-variables

 tools/bootconfig/scripts/ftrace2bconf.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 032ac5b10a4a..b01297217cf3 100755
@@ -88,10 +88,10 @@ DEFINED_VARS=
 UNRESOLVED_EVENTS=
 
 defined_vars() { # event-dir
-	grep "^hist" $1/trigger | grep -o ':[a-zA-Z0-9]*='
+	grep "^hist" $1/trigger | grep -o ':[a-zA-Z0-9_]*='
 }
 referred_vars() {
-	grep "^hist" $1/trigger | grep -o '$[a-zA-Z0-9]*'
+	grep "^hist" $1/trigger | grep -o '$[a-zA-Z0-9_]*'
 }
 
 event_is_enabled() { # enable-file

2.53.0

These looks good to me. Let me pick it.

Thanks!

Masami Hiramatsu (Google) <[email protected]>

Thanks Masami!

Eduardo Mucelli R. Oliveira -- http://eduar.do