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
[off-list ref] napsal:

quoted

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 <tglozar@redhat.com>
Signed-off-by: Valentin Schneider <vschneid@redhat.com>
---

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;