A few functions duplicate the logic for handling threshold actions.
When a threshold is reached, these functions stop the trace, perform
actions, and restart the trace if configured to continue.

Create a new helper function, common_restart(), to centralize this
shared logic and avoid code duplication. This function now handles the
threshold actions and restarts the necessary trace instances.

Refactor the affected functions main loops to call the new helper.
This makes the code cleaner and more maintainable.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/common.c        | 65 +++++++++++++++++++-------
 tools/tracing/rtla/src/common.h        |  9 ++++
 tools/tracing/rtla/src/timerlat_hist.c | 20 ++++----
 tools/tracing/rtla/src/timerlat_top.c  | 20 ++++----
 4 files changed, 78 insertions(+), 36 deletions(-)

diff --git a/tools/tracing/rtla/src/common.c b/tools/tracing/rtla/src/common.c
index b197037fc58b3..d608ffe12e7b0 100644
--- a/tools/tracing/rtla/src/common.c
+++ b/tools/tracing/rtla/src/common.c
@@ -95,6 +95,37 @@ common_apply_config(struct osnoise_tool *tool, struct common_params *params)
 }
 
 
+/**
+ * common_restart - handle threshold actions and optionally restart tracing
+ * @tool: pointer to the osnoise_tool instance containing trace contexts
+ * @params: timerlat parameters with threshold action configuration
+ *
+ * Return:
+ *   RESTART_OK - Actions executed successfully and tracing restarted
+ *   RESTART_STOP - Actions executed but 'continue' flag not set, stop tracing
+ *   RESTART_ERROR - Failed to restart tracing after executing actions
+ */
+enum restart_result
+common_restart(const struct osnoise_tool *tool, struct common_params *params)
+{
+	actions_perform(&params->threshold_actions);
+
+	if (!params->threshold_actions.continue_flag)
+		/* continue flag not set, break */
+		return RESTART_STOP;
+
+	/* continue action reached, re-enable tracing */
+	if (tool->record && trace_instance_start(&tool->record->trace))
+		goto err;
+	if (tool->aa && trace_instance_start(&tool->aa->trace))
+		goto err;
+	return RESTART_OK;
+
+err:
+	err_msg("Error restarting trace\n");
+	return RESTART_ERROR;
+}
+
 int run_tool(struct tool_ops *ops, int argc, char *argv[])
 {
 	struct common_params *params;
@@ -272,17 +303,16 @@ int top_main_loop(struct osnoise_tool *tool)
 				/* stop tracing requested, do not perform actions */
 				return 0;
 
-			actions_perform(&params->threshold_actions);
+			enum restart_result result;
+
+			result = common_restart(tool, params);
 
-			if (!params->threshold_actions.continue_flag)
-				/* continue flag not set, break */
+			if (result == RESTART_STOP)
 				return 0;
 
-			/* continue action reached, re-enable tracing */
-			if (record)
-				trace_instance_start(&record->trace);
-			if (tool->aa)
-				trace_instance_start(&tool->aa->trace);
+			if (result == RESTART_ERROR)
+				return -1;
+
 			trace_instance_start(trace);
 		}
 
@@ -323,18 +353,17 @@ int hist_main_loop(struct osnoise_tool *tool)
 				/* stop tracing requested, do not perform actions */
 				break;
 
-			actions_perform(&params->threshold_actions);
+			enum restart_result result;
 
-			if (!params->threshold_actions.continue_flag)
-				/* continue flag not set, break */
-				break;
+			result = common_restart(tool, params);
+
+			if (result == RESTART_STOP)
+				return 0;
 
-			/* continue action reached, re-enable tracing */
-			if (tool->record)
-				trace_instance_start(&tool->record->trace);
-			if (tool->aa)
-				trace_instance_start(&tool->aa->trace);
-			trace_instance_start(&tool->trace);
+			if (result == RESTART_ERROR)
+				return -1;
+
+			trace_instance_start(trace);
 		}
 
 		/* is there still any user-threads ? */
diff --git a/tools/tracing/rtla/src/common.h b/tools/tracing/rtla/src/common.h
index 9ec2b7632c376..f2c9e21c03651 100644
--- a/tools/tracing/rtla/src/common.h
+++ b/tools/tracing/rtla/src/common.h
@@ -143,6 +143,15 @@ struct tool_ops {
 	void (*free)(struct osnoise_tool *tool);
 };
 
+enum restart_result {
+	RESTART_OK,
+	RESTART_STOP,
+	RESTART_ERROR = -1,
+};
+
+enum restart_result
+common_restart(const struct osnoise_tool *tool, struct common_params *params);
+
 int osnoise_set_cpus(struct osnoise_context *context, char *cpus);
 void osnoise_restore_cpus(struct osnoise_context *context);
 
diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c
index 226167c88c8d6..27fc98270da59 100644
--- a/tools/tracing/rtla/src/timerlat_hist.c
+++ b/tools/tracing/rtla/src/timerlat_hist.c
@@ -17,6 +17,7 @@
 #include "timerlat.h"
 #include "timerlat_aa.h"
 #include "timerlat_bpf.h"
+#include "common.h"
 
 struct timerlat_hist_cpu {
 	int			*irq;
@@ -1100,18 +1101,19 @@ static int timerlat_hist_bpf_main_loop(struct osnoise_tool *tool)
 
 		if (!stop_tracing) {
 			/* Threshold overflow, perform actions on threshold */
-			actions_perform(&params->common.threshold_actions);
+			enum restart_result result;
 
-			if (!params->common.threshold_actions.continue_flag)
-				/* continue flag not set, break */
+			result = common_restart(tool, &params->common);
+			if (result == RESTART_STOP)
 				break;
 
-			/* continue action reached, re-enable tracing */
-			if (tool->record)
-				trace_instance_start(&tool->record->trace);
-			if (tool->aa)
-				trace_instance_start(&tool->aa->trace);
-			timerlat_bpf_restart_tracing();
+			if (result == RESTART_ERROR)
+				return -1;
+
+			if (timerlat_bpf_restart_tracing()) {
+				err_msg("Error restarting BPF trace\n");
+				return -1;
+			}
 		}
 	}
 	timerlat_bpf_detach();
diff --git a/tools/tracing/rtla/src/timerlat_top.c b/tools/tracing/rtla/src/timerlat_top.c
index 31e1514a2528d..f7e85dc918ef3 100644
--- a/tools/tracing/rtla/src/timerlat_top.c
+++ b/tools/tracing/rtla/src/timerlat_top.c
@@ -18,6 +18,7 @@
 #include "timerlat.h"
 #include "timerlat_aa.h"
 #include "timerlat_bpf.h"
+#include "common.h"
 
 struct timerlat_top_cpu {
 	unsigned long long	irq_count;
@@ -869,18 +870,19 @@ timerlat_top_bpf_main_loop(struct osnoise_tool *tool)
 
 		if (wait_retval != 0) {
 			/* Stopping requested by tracer */
-			actions_perform(&params->common.threshold_actions);
+			enum restart_result result;
 
-			if (!params->common.threshold_actions.continue_flag)
-				/* continue flag not set, break */
+			result = common_restart(tool, &params->common);
+			if (result == RESTART_STOP)
 				break;
 
-			/* continue action reached, re-enable tracing */
-			if (tool->record)
-				trace_instance_start(&tool->record->trace);
-			if (tool->aa)
-				trace_instance_start(&tool->aa->trace);
-			timerlat_bpf_restart_tracing();
+			if (result == RESTART_ERROR)
+				return -1;
+
+			if (timerlat_bpf_restart_tracing()) {
+				err_msg("Error restarting BPF trace\n");
+				return -1;
+			}
 		}
 
 		/* is there still any user-threads ? */
-- 
2.52.0

út 6. 1. 2026 v 14:42 odesílatel Wander Lairson Costa
<wander@redhat.com> napsal:
>
> A few functions duplicate the logic for handling threshold actions.
> When a threshold is reached, these functions stop the trace, perform
> actions, and restart the trace if configured to continue.
>
> Create a new helper function, common_restart(), to centralize this
> shared logic and avoid code duplication. This function now handles the
> threshold actions and restarts the necessary trace instances.
>
> Refactor the affected functions main loops to call the new helper.
> This makes the code cleaner and more maintainable.
>

The deduplication idea is good, but I find the name of the helper
quite confusing. The main function of the helper is not to restart
tracing, it is to handle a latency threshold overflow - restarting
tracing is only one of possible effects, and one that is only applied
when using --on-threshold continue which is not the most common use
case. Could something like common_handle_stop_tracing() perhaps be
better?

> +enum restart_result {
> +       RESTART_OK,
> +       RESTART_STOP,
> +       RESTART_ERROR = -1,
> +};

Do we really need a separate return value enum just for this one helper?

Tomas

On Wed, Jan 7, 2026 at 9:03 AM Tomas Glozar <tglozar@redhat.com> wrote:
>
> út 6. 1. 2026 v 14:42 odesílatel Wander Lairson Costa
> <wander@redhat.com> napsal:
> >
> > A few functions duplicate the logic for handling threshold actions.
> > When a threshold is reached, these functions stop the trace, perform
> > actions, and restart the trace if configured to continue.
> >
> > Create a new helper function, common_restart(), to centralize this
> > shared logic and avoid code duplication. This function now handles the
> > threshold actions and restarts the necessary trace instances.
> >
> > Refactor the affected functions main loops to call the new helper.
> > This makes the code cleaner and more maintainable.
> >
>
> The deduplication idea is good, but I find the name of the helper
> quite confusing. The main function of the helper is not to restart
> tracing, it is to handle a latency threshold overflow - restarting
> tracing is only one of possible effects, and one that is only applied
> when using --on-threshold continue which is not the most common use
> case. Could something like common_handle_stop_tracing() perhaps be
> better?
>

Sure, I will change the name in v3.

> > +enum restart_result {
> > +       RESTART_OK,
> > +       RESTART_STOP,
> > +       RESTART_ERROR = -1,
> > +};
>
> Do we really need a separate return value enum just for this one helper?
>

If it was success/failure type of return value, we wouldn't need.
However, a three state code, I think it is worth for code readiness.
Do you have something else in mind?

> Tomas
>

st 7. 1. 2026 v 13:43 odesílatel Wander Lairson Costa
<wander@redhat.com> napsal:
> >
> > The deduplication idea is good, but I find the name of the helper
> > quite confusing. The main function of the helper is not to restart
> > tracing, it is to handle a latency threshold overflow - restarting
> > tracing is only one of possible effects, and one that is only applied
> > when using --on-threshold continue which is not the most common use
> > case. Could something like common_handle_stop_tracing() perhaps be
> > better?
> >
>
> Sure, I will change the name in v3.
>

Thanks.

> > > +enum restart_result {
> > > +       RESTART_OK,
> > > +       RESTART_STOP,
> > > +       RESTART_ERROR = -1,
> > > +};
> >
> > Do we really need a separate return value enum just for this one helper?
> >
>
> If it was success/failure type of return value, we wouldn't need.
> However, a three state code, I think it is worth for code readiness.
> Do you have something else in mind?
>

The main loop can simply use the continue flag, just like in the old
version, no need to duplicate that information into the return value
of common_restart().

Tomas

On Wed, Jan 7, 2026 at 10:47 AM Tomas Glozar <tglozar@redhat.com> wrote:
>
> st 7. 1. 2026 v 13:43 odesílatel Wander Lairson Costa
> <wander@redhat.com> napsal:
> > >
> > > The deduplication idea is good, but I find the name of the helper
> > > quite confusing. The main function of the helper is not to restart
> > > tracing, it is to handle a latency threshold overflow - restarting
> > > tracing is only one of possible effects, and one that is only applied
> > > when using --on-threshold continue which is not the most common use
> > > case. Could something like common_handle_stop_tracing() perhaps be
> > > better?
> > >
> >
> > Sure, I will change the name in v3.
> >
>
> Thanks.
>
> > > > +enum restart_result {
> > > > +       RESTART_OK,
> > > > +       RESTART_STOP,
> > > > +       RESTART_ERROR = -1,
> > > > +};
> > >
> > > Do we really need a separate return value enum just for this one helper?
> > >
> >
> > If it was success/failure type of return value, we wouldn't need.
> > However, a three state code, I think it is worth for code readiness.
> > Do you have something else in mind?
> >
>
> The main loop can simply use the continue flag, just like in the old
> version, no need to duplicate that information into the return value
> of common_restart().
>

Ok, I will change it in v3.

> Tomas
>