Re: [PATCH v4 5/6] rtla: Unconditionally clean any pre-existing filters for user-provided events

On 10/08/26 13:24, Tomas Glozar wrote:
> út 4. 8. 2026 v 19:43 odesílatel Valentin Schneider
> <[email protected]> napsal:
>>
>> A later commit will apply a filter to events recorded to the trace
>> output. To prevent any user confusion, remove pre-existing filters when
>> enabling an event provided via the '-e' command line argument.
>>
>> Suggested-by: Tomas Glozar <[email protected]>
>> Signed-off-by: Valentin Schneider <[email protected]>
>> ---
>
> I found that I missed one case when suggesting this: the user might
> supply an event twice. With this change, it will now clear the filter
> the second time the event is applied:
>
> $ rtla timerlat hist --no-aa --on-threshold trace \
>     --on-threshold shell,command="grep sched_switch timerlat_trace.txt
> | grep -Fv '[000]' | head -n1" \
>     -i 1 -e sched:sched_switch -e sched:sched_switch --filter "cpu == 0"
>
> Without this commit:
>
> ```
>  Saving trace to timerlat_trace.txt
> # RTLA timerlat histogram
> ...
> ```
>
> With this commit:
>
> ```
>  Saving trace to timerlat_trace.txt
>          <idle>-0       [002] d..2. 423185.347008: sched_switch: ...
> # RTLA timerlat histogram
> ...
> ```
>
> (Note that events are processed in opposite order to the command line.)
>
> This is unexpected and might break scripts that for some reason enable
> an event twice. So I'm not sure if my suggestion was the best
> solution.
>

Hm, didn't think of that.

I would say having the last defined event+filter override any previous
filter would make the most sense.

trace-cmd does this partially:

bash-5.3# trace-cmd record -e sched_switch -f 'CPU==0' -e sched_switch -- bash -c 'ls &>/dev/null'
CPU0 data recorded at offset=0x180000
    109 bytes in size (8192 uncompressed)
CPU1 data recorded at offset=0x181000
    0 bytes in size (0 uncompressed)
CPU2 data recorded at offset=0x181000
    0 bytes in size (0 uncompressed)
CPU3 data recorded at offset=0x181000
    0 bytes in size (0 uncompressed)

bash-5.3# trace-cmd record -e sched_switch -f 'CPU==0' -e sched_switch -f 'CPU==1' -- bash -c 'ls &>/dev/null'
CPU0 data recorded at offset=0x180000
    0 bytes in size (0 uncompressed)
CPU1 data recorded at offset=0x180000
    1428 bytes in size (237568 uncompressed)
CPU2 data recorded at offset=0x181000
    0 bytes in size (0 uncompressed)
CPU3 data recorded at offset=0x181000
    0 bytes in size (0 uncompressed)

Although I didn't realize that events were handled in reverse cmdline input
order until you pointed it out.

AIUI filers and triggers rely on the LIFO ordering to grab the
last-provided event; making the events list double-linked (but not
circular) would let us process them in FIFO order; something like the
barely tested:

---
diff --git a/tools/tracing/rtla/src/cli_p.h b/tools/tracing/rtla/src/cli_p.h
index 3c939de9abf02..4638cc317ea26 100644
--- a/tools/tracing/rtla/src/cli_p.h
+++ b/tools/tracing/rtla/src/cli_p.h
@@ -221,8 +221,10 @@ static int opt_event_cb(const struct option *opt, const char *arg, int unset)
 	if (!tevent)
 		fatal("Error alloc trace event");
 
-	if (*events)
+	if (*events) {
 		tevent->next = *events;
+		(*events)->prev = tevent;
+	}
 	*events = tevent;
 
 	return 0;
diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c
index e407447773d04..35601a2e8d0c7 100644
--- a/tools/tracing/rtla/src/trace.c
+++ b/tools/tracing/rtla/src/trace.c
@@ -440,6 +440,18 @@ static void trace_event_disable_trigger(struct trace_instance *instance,
 			tevent->event ? : "*", tevent->trigger);
 }
 
+static inline struct trace_events *trace_events_tail(struct trace_events *tevent)
+{
+	while (tevent && tevent->next)
+		tevent = tevent->next;
+
+	return tevent;
+}
+
+/* Events are stashed in LIFO order; flip that to FIFO processing */
+#define for_each_trace_event(tevent) \
+	for (tevent = trace_events_tail(tevent); tevent; tevent = tevent->prev)
+
 /*
  * trace_events_disable - disable all trace events
  */
@@ -451,7 +463,7 @@ void trace_events_disable(struct trace_instance *instance,
 	if (!events)
 		return;
 
-	while (tevent) {
+	for_each_trace_event(tevent) {
 		debug_msg("Disabling event %s:%s\n", tevent->system, tevent->event ? : "*");
 		if (tevent->enabled) {
 			trace_event_disable_filter(instance, tevent);
@@ -460,7 +472,6 @@ void trace_events_disable(struct trace_instance *instance,
 		}
 
 		tevent->enabled = 0;
-		tevent = tevent->next;
 	}
 }
 
@@ -544,7 +555,10 @@ int trace_events_enable(struct trace_instance *instance,
 	struct trace_events *tevent = events;
 	int retval;
 
-	while (tevent) {
+	if (!events)
+		return 0;
+
+	for_each_trace_event(tevent) {
 		debug_msg("Enabling event %s:%s\n", tevent->system, tevent->event ? : "*");
 		retval = tracefs_event_enable(instance->inst, tevent->system, tevent->event);
 		if (retval < 0) {
@@ -562,7 +576,6 @@ int trace_events_enable(struct trace_instance *instance,
 			return 1;
 
 		tevent->enabled = 1;
-		tevent = tevent->next;
 	}
 
 	return 0;
diff --git a/tools/tracing/rtla/src/trace.h b/tools/tracing/rtla/src/trace.h
index 95b911a2228b2..eacafc0c96b31 100644
--- a/tools/tracing/rtla/src/trace.h
+++ b/tools/tracing/rtla/src/trace.h
@@ -4,6 +4,7 @@
 
 struct trace_events {
 	struct trace_events *next;
+	struct trace_events *prev;
 	char *system;
 	char *event;
 	char *filter;