* [PATCH] rtla: Simplify osnoise tracer option setting code
@ 2026-06-12 11:51 Tomas Glozar
2026-06-12 17:54 ` Crystal Wood
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Glozar @ 2026-06-12 11:51 UTC (permalink / raw)
To: Steven Rostedt, Tomas Glozar
Cc: John Kacur, Luis Goncalves, Crystal Wood, Costa Shulyupin,
Wander Lairson Costa, LKML, linux-trace-kernel
Each osnoise tracer option (in /sys/kernel/tracing/osnoise) used by RTLA
requires four functions to be defined:
- static osnoise_get_<opt>() - to get the current value of the option
and save it into struct osnoise_context's orig_<opt> field,
- osnoise_set_<opt>() - to set the value of the option requested by the
user after reading and saving the original with osnoise_get_<opt>(),
and save it into <opt> field of struct osnoise_context,
- osnoise_restore_<opt>() - restore the value recorded in orig_<opt>,
- static osnoise_put_<opt>() - restore the value recorded in orig_<opt>
and update <opt> to reflect that.
The logic is duplicated for all the options, except for cpus (which is
the only string option) and period/runtime (which are handled together
and feature extra checks).
Deduplicate the logic using a set of macros featuring the X macro
pattern, defined in src/common.h:
- OSNOISE_LL_OPTIONS, which invokes OSNOISE_LL_OPTION macro for all
"long long" options,
- OSNOISE_FLAG_OPTIONS, which invokes OSNOISE_FLAG_OPTION macro for all
flag (boolean values in osnoise/options file) options.
The list macros are then invoked in four places:
- for struct osnoise_context fields in src/common.h,
- for function declarations, moved into src/common.h from
src/osnoise.h,
- for function definitions in src/osnoise.c,
- for context initialization and restoration, in osnoise_context_alloc()
and osnoise_put_context(), both in src/osnoise.c.
OSNOISE_LL_OPTIONS takes three options: name - struct osnoise_context
field name (written "<opt>" above), path - filename inside
/sys/kernel/tracing/osnoise passed to libtracefs, and init_val - initial
value of struct fields, corresponding to an otherwise invalid option
(some options use OSNOISE_OPTION_INIT_VAL = -1, some use
OSNOISE_TIME_INIT_VAL = 0).
OSNOISE_FLAG_OPTION is similar, but instead of path, it takes the option
string inside /sys/kernel/tracing/osnoise/options (opt_string), and no
init_val, as it is purely boolean (0 or 1).
Previously, for options timerlat_align and osnoise_workload, the return
value of osnoise_set_<opt>() distinguished between -2 (option cannot be
set) and -1 (option not present). This distinction is expanded for all
options for consistency; for most options, it is currently not used,
only osnoise_workload is implemented to avoid error on -1 on older RTLA
versions.
The change overall has two main benefits: it makes it much simpler to
add a new option, as well as to change existing logic consistently for
all of them. It also makes the code shorter by a bit over 500 lines.
There is no intentional user-visible change coming from the refactoring.
osnoise_restore_<opt>() for flag options now sets <opt> instead of
orig_<opt>. As the latter is also set by osnoise_put_<opt>(), plus long
long options set <opt> in both the old and new implementation, the old
behavior was likely a mistake, and should not matter for now, as the
options are only restored once at the end of tracing and neither <opt>
nor orig_<opt> field is read again.
Assisted-by: Claude:claude-opus-4-6
Signed-off-by: Tomas Glozar <tglozar@redhat.com>
---
tools/tracing/rtla/src/common.h | 79 +--
tools/tracing/rtla/src/osnoise.c | 836 ++++++-------------------------
tools/tracing/rtla/src/osnoise.h | 22 -
3 files changed, 188 insertions(+), 749 deletions(-)
diff --git a/tools/tracing/rtla/src/common.h b/tools/tracing/rtla/src/common.h
index 04b287a03f6d..47233b0781c7 100644
--- a/tools/tracing/rtla/src/common.h
+++ b/tools/tracing/rtla/src/common.h
@@ -6,9 +6,35 @@
#include "trace.h"
#include "utils.h"
+/*
+ * OSNOISE_LL_OPTIONS - list of long long options backed by tracefs files.
+ * OSNOISE_LL_OPTION(field_name, tracefs_path, init_value)
+ *
+ * OSNOISE_FLAG_OPTIONS - list of boolean options backed by osnoise/options.
+ * OSNOISE_FLAG_OPTION(field_name, option_string)
+ */
+#define OSNOISE_LL_OPTIONS \
+ OSNOISE_LL_OPTION(stop_us, "osnoise/stop_tracing_us", OSNOISE_OPTION_INIT_VAL) \
+ OSNOISE_LL_OPTION(stop_total_us, "osnoise/stop_tracing_total_us", OSNOISE_OPTION_INIT_VAL) \
+ OSNOISE_LL_OPTION(print_stack, "osnoise/print_stack", OSNOISE_OPTION_INIT_VAL) \
+ OSNOISE_LL_OPTION(tracing_thresh, "tracing_thresh", OSNOISE_OPTION_INIT_VAL) \
+ OSNOISE_LL_OPTION(timerlat_period_us, "osnoise/timerlat_period_us", OSNOISE_TIME_INIT_VAL) \
+ OSNOISE_LL_OPTION(timerlat_align_us, "osnoise/timerlat_align_us", OSNOISE_OPTION_INIT_VAL)
+
+#define OSNOISE_FLAG_OPTIONS \
+ OSNOISE_FLAG_OPTION(irq_disable, "OSNOISE_IRQ_DISABLE") \
+ OSNOISE_FLAG_OPTION(workload, "OSNOISE_WORKLOAD") \
+ OSNOISE_FLAG_OPTION(timerlat_align, "TIMERLAT_ALIGN")
+
/*
* osnoise_context - read, store, write, restore osnoise configs.
*/
+#define OSNOISE_LL_OPTION(name, path, init_val) \
+ long long orig_##name; \
+ long long name;
+#define OSNOISE_FLAG_OPTION(name, option_str) \
+ int orig_opt_##name; \
+ int opt_##name;
struct osnoise_context {
int flags;
int ref;
@@ -24,42 +50,11 @@ struct osnoise_context {
unsigned long long orig_period_us;
unsigned long long period_us;
- /* 0 as init value */
- long long orig_timerlat_period_us;
- long long timerlat_period_us;
-
- /* 0 as init value */
- long long orig_tracing_thresh;
- long long tracing_thresh;
-
- /* -1 as init value because 0 is disabled */
- long long orig_stop_us;
- long long stop_us;
-
- /* -1 as init value because 0 is disabled */
- long long orig_stop_total_us;
- long long stop_total_us;
-
- /* -1 as init value because 0 is disabled */
- long long orig_print_stack;
- long long print_stack;
-
- /* -1 as init value because 0 is off */
- int orig_opt_irq_disable;
- int opt_irq_disable;
-
- /* -1 as init value because 0 is off */
- int orig_opt_workload;
- int opt_workload;
-
- /* -1 as init value because 0 is off */
- int orig_opt_timerlat_align;
- int opt_timerlat_align;
-
- /* 0 as init value */
- unsigned long long orig_timerlat_align_us;
- unsigned long long timerlat_align_us;
+ OSNOISE_LL_OPTIONS
+ OSNOISE_FLAG_OPTIONS
};
+#undef OSNOISE_LL_OPTION
+#undef OSNOISE_FLAG_OPTION
extern volatile int stop_tracing;
@@ -173,15 +168,21 @@ common_threshold_handler(const struct osnoise_tool *tool);
int osnoise_set_cpus(struct osnoise_context *context, char *cpus);
void osnoise_restore_cpus(struct osnoise_context *context);
-int osnoise_set_workload(struct osnoise_context *context, bool onoff);
+#define OSNOISE_LL_OPTION(name, path, init_val) \
+ int osnoise_set_##name(struct osnoise_context *context, long long name); \
+ void osnoise_restore_##name(struct osnoise_context *context);
+#define OSNOISE_FLAG_OPTION(name, option_str) \
+ int osnoise_set_##name(struct osnoise_context *context, bool onoff); \
+ void osnoise_restore_##name(struct osnoise_context *context);
+OSNOISE_LL_OPTIONS
+OSNOISE_FLAG_OPTIONS
+#undef OSNOISE_LL_OPTION
+#undef OSNOISE_FLAG_OPTION
void osnoise_destroy_tool(struct osnoise_tool *top);
struct osnoise_tool *osnoise_init_tool(char *tool_name);
struct osnoise_tool *osnoise_init_trace_tool(const char *tracer);
bool osnoise_trace_is_off(struct osnoise_tool *tool, struct osnoise_tool *record);
-int osnoise_set_stop_us(struct osnoise_context *context, long long stop_us);
-int osnoise_set_stop_total_us(struct osnoise_context *context,
- long long stop_total_us);
int common_apply_config(struct osnoise_tool *tool, struct common_params *params);
int top_main_loop(struct osnoise_tool *tool);
diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c
index 4ff5dad013b1..7f15d00b431e 100644
--- a/tools/tracing/rtla/src/osnoise.c
+++ b/tools/tracing/rtla/src/osnoise.c
@@ -345,480 +345,73 @@ void osnoise_put_runtime_period(struct osnoise_context *context)
}
/*
- * osnoise_get_timerlat_period_us - read and save the original "timerlat_period_us"
- */
-static long long
-osnoise_get_timerlat_period_us(struct osnoise_context *context)
-{
- long long timerlat_period_us;
-
- if (context->timerlat_period_us != OSNOISE_TIME_INIT_VAL)
- return context->timerlat_period_us;
-
- if (context->orig_timerlat_period_us != OSNOISE_TIME_INIT_VAL)
- return context->orig_timerlat_period_us;
-
- timerlat_period_us = osnoise_read_ll_config("osnoise/timerlat_period_us");
- if (timerlat_period_us < 0)
- goto out_err;
-
- context->orig_timerlat_period_us = timerlat_period_us;
- return timerlat_period_us;
-
-out_err:
- return OSNOISE_TIME_INIT_VAL;
-}
-
-/*
- * osnoise_set_timerlat_period_us - set "timerlat_period_us"
- */
-int osnoise_set_timerlat_period_us(struct osnoise_context *context, long long timerlat_period_us)
-{
- long long curr_timerlat_period_us = osnoise_get_timerlat_period_us(context);
- int retval;
-
- if (curr_timerlat_period_us == OSNOISE_TIME_INIT_VAL)
- return -1;
-
- retval = osnoise_write_ll_config("osnoise/timerlat_period_us", timerlat_period_us);
- if (retval < 0)
- return -1;
-
- context->timerlat_period_us = timerlat_period_us;
-
- return 0;
-}
-
-/*
- * osnoise_restore_timerlat_period_us - restore "timerlat_period_us"
- */
-void osnoise_restore_timerlat_period_us(struct osnoise_context *context)
-{
- int retval;
-
- if (context->orig_timerlat_period_us == OSNOISE_TIME_INIT_VAL)
- return;
-
- if (context->orig_timerlat_period_us == context->timerlat_period_us)
- goto out_done;
-
- retval = osnoise_write_ll_config("osnoise/timerlat_period_us", context->orig_timerlat_period_us);
- if (retval < 0)
- err_msg("Could not restore original osnoise timerlat_period_us\n");
-
-out_done:
- context->timerlat_period_us = OSNOISE_TIME_INIT_VAL;
-}
-
-/*
- * osnoise_put_timerlat_period_us - restore original values and cleanup data
- */
-void osnoise_put_timerlat_period_us(struct osnoise_context *context)
-{
- osnoise_restore_timerlat_period_us(context);
-
- if (context->orig_timerlat_period_us == OSNOISE_TIME_INIT_VAL)
- return;
-
- context->orig_timerlat_period_us = OSNOISE_TIME_INIT_VAL;
-}
-
-/*
- * osnoise_get_timerlat_align_us - read and save the original "timerlat_align_us"
- */
-static long long
-osnoise_get_timerlat_align_us(struct osnoise_context *context)
-{
- long long timerlat_align_us;
-
- if (context->timerlat_align_us != OSNOISE_OPTION_INIT_VAL)
- return context->timerlat_align_us;
-
- if (context->orig_timerlat_align_us != OSNOISE_OPTION_INIT_VAL)
- return context->orig_timerlat_align_us;
-
- timerlat_align_us = osnoise_read_ll_config("osnoise/timerlat_align_us");
- if (timerlat_align_us < 0)
- goto out_err;
-
- context->orig_timerlat_align_us = timerlat_align_us;
- return timerlat_align_us;
-
-out_err:
- return OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_set_timerlat_align_us - set "timerlat_align_us"
- */
-int osnoise_set_timerlat_align_us(struct osnoise_context *context, long long timerlat_align_us)
-{
- long long curr_timerlat_align_us = osnoise_get_timerlat_align_us(context);
- int retval;
-
- if (curr_timerlat_align_us == OSNOISE_OPTION_INIT_VAL)
- return -1;
-
- retval = osnoise_write_ll_config("osnoise/timerlat_align_us", timerlat_align_us);
- if (retval < 0)
- return -1;
-
- context->timerlat_align_us = timerlat_align_us;
-
- return 0;
-}
-
-/*
- * osnoise_restore_timerlat_align_us - restore "timerlat_align_us"
- */
-void osnoise_restore_timerlat_align_us(struct osnoise_context *context)
-{
- int retval;
-
- if (context->orig_timerlat_align_us == OSNOISE_OPTION_INIT_VAL)
- return;
-
- if (context->orig_timerlat_align_us == context->timerlat_align_us)
- goto out_done;
-
- retval = osnoise_write_ll_config("osnoise/timerlat_align_us",
- context->orig_timerlat_align_us);
- if (retval < 0)
- err_msg("Could not restore original osnoise timerlat_align_us\n");
-
-out_done:
- context->timerlat_align_us = OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_put_timerlat_align_us - restore original values and cleanup data
- */
-void osnoise_put_timerlat_align_us(struct osnoise_context *context)
-{
- osnoise_restore_timerlat_align_us(context);
-
- if (context->orig_timerlat_align_us == OSNOISE_OPTION_INIT_VAL)
- return;
-
- context->orig_timerlat_align_us = OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_get_stop_us - read and save the original "stop_tracing_us"
- */
-static long long
-osnoise_get_stop_us(struct osnoise_context *context)
-{
- long long stop_us;
-
- if (context->stop_us != OSNOISE_OPTION_INIT_VAL)
- return context->stop_us;
-
- if (context->orig_stop_us != OSNOISE_OPTION_INIT_VAL)
- return context->orig_stop_us;
-
- stop_us = osnoise_read_ll_config("osnoise/stop_tracing_us");
- if (stop_us < 0)
- goto out_err;
-
- context->orig_stop_us = stop_us;
- return stop_us;
-
-out_err:
- return OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_set_stop_us - set "stop_tracing_us"
- */
-int osnoise_set_stop_us(struct osnoise_context *context, long long stop_us)
-{
- long long curr_stop_us = osnoise_get_stop_us(context);
- int retval;
-
- if (curr_stop_us == OSNOISE_OPTION_INIT_VAL)
- return -1;
-
- retval = osnoise_write_ll_config("osnoise/stop_tracing_us", stop_us);
- if (retval < 0)
- return -1;
-
- context->stop_us = stop_us;
-
- return 0;
-}
-
-/*
- * osnoise_restore_stop_us - restore the original "stop_tracing_us"
- */
-void osnoise_restore_stop_us(struct osnoise_context *context)
-{
- int retval;
-
- if (context->orig_stop_us == OSNOISE_OPTION_INIT_VAL)
- return;
-
- if (context->orig_stop_us == context->stop_us)
- goto out_done;
-
- retval = osnoise_write_ll_config("osnoise/stop_tracing_us", context->orig_stop_us);
- if (retval < 0)
- err_msg("Could not restore original osnoise stop_us\n");
-
-out_done:
- context->stop_us = OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_put_stop_us - restore original values and cleanup data
- */
-void osnoise_put_stop_us(struct osnoise_context *context)
-{
- osnoise_restore_stop_us(context);
-
- if (context->orig_stop_us == OSNOISE_OPTION_INIT_VAL)
- return;
-
- context->orig_stop_us = OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_get_stop_total_us - read and save the original "stop_tracing_total_us"
- */
-static long long
-osnoise_get_stop_total_us(struct osnoise_context *context)
-{
- long long stop_total_us;
-
- if (context->stop_total_us != OSNOISE_OPTION_INIT_VAL)
- return context->stop_total_us;
-
- if (context->orig_stop_total_us != OSNOISE_OPTION_INIT_VAL)
- return context->orig_stop_total_us;
-
- stop_total_us = osnoise_read_ll_config("osnoise/stop_tracing_total_us");
- if (stop_total_us < 0)
- goto out_err;
-
- context->orig_stop_total_us = stop_total_us;
- return stop_total_us;
-
-out_err:
- return OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_set_stop_total_us - set "stop_tracing_total_us"
- */
-int osnoise_set_stop_total_us(struct osnoise_context *context, long long stop_total_us)
-{
- long long curr_stop_total_us = osnoise_get_stop_total_us(context);
- int retval;
-
- if (curr_stop_total_us == OSNOISE_OPTION_INIT_VAL)
- return -1;
-
- retval = osnoise_write_ll_config("osnoise/stop_tracing_total_us", stop_total_us);
- if (retval < 0)
- return -1;
-
- context->stop_total_us = stop_total_us;
-
- return 0;
-}
-
-/*
- * osnoise_restore_stop_total_us - restore the original "stop_tracing_total_us"
- */
-void osnoise_restore_stop_total_us(struct osnoise_context *context)
-{
- int retval;
-
- if (context->orig_stop_total_us == OSNOISE_OPTION_INIT_VAL)
- return;
-
- if (context->orig_stop_total_us == context->stop_total_us)
- goto out_done;
-
- retval = osnoise_write_ll_config("osnoise/stop_tracing_total_us",
- context->orig_stop_total_us);
- if (retval < 0)
- err_msg("Could not restore original osnoise stop_total_us\n");
-
-out_done:
- context->stop_total_us = OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_put_stop_total_us - restore original values and cleanup data
- */
-void osnoise_put_stop_total_us(struct osnoise_context *context)
-{
- osnoise_restore_stop_total_us(context);
-
- if (context->orig_stop_total_us == OSNOISE_OPTION_INIT_VAL)
- return;
-
- context->orig_stop_total_us = OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_get_print_stack - read and save the original "print_stack"
- */
-static long long
-osnoise_get_print_stack(struct osnoise_context *context)
-{
- long long print_stack;
-
- if (context->print_stack != OSNOISE_OPTION_INIT_VAL)
- return context->print_stack;
-
- if (context->orig_print_stack != OSNOISE_OPTION_INIT_VAL)
- return context->orig_print_stack;
-
- print_stack = osnoise_read_ll_config("osnoise/print_stack");
- if (print_stack < 0)
- goto out_err;
-
- context->orig_print_stack = print_stack;
- return print_stack;
-
-out_err:
- return OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_set_print_stack - set "print_stack"
- */
-int osnoise_set_print_stack(struct osnoise_context *context, long long print_stack)
-{
- long long curr_print_stack = osnoise_get_print_stack(context);
- int retval;
-
- if (curr_print_stack == OSNOISE_OPTION_INIT_VAL)
- return -1;
-
- retval = osnoise_write_ll_config("osnoise/print_stack", print_stack);
- if (retval < 0)
- return -1;
-
- context->print_stack = print_stack;
-
- return 0;
-}
-
-/*
- * osnoise_restore_print_stack - restore the original "print_stack"
- */
-void osnoise_restore_print_stack(struct osnoise_context *context)
-{
- int retval;
-
- if (context->orig_print_stack == OSNOISE_OPTION_INIT_VAL)
- return;
-
- if (context->orig_print_stack == context->print_stack)
- goto out_done;
-
- retval = osnoise_write_ll_config("osnoise/print_stack", context->orig_print_stack);
- if (retval < 0)
- err_msg("Could not restore original osnoise print_stack\n");
-
-out_done:
- context->print_stack = OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_put_print_stack - restore original values and cleanup data
- */
-void osnoise_put_print_stack(struct osnoise_context *context)
-{
- osnoise_restore_print_stack(context);
-
- if (context->orig_print_stack == OSNOISE_OPTION_INIT_VAL)
- return;
-
- context->orig_print_stack = OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_get_tracing_thresh - read and save the original "tracing_thresh"
- */
-static long long
-osnoise_get_tracing_thresh(struct osnoise_context *context)
-{
- long long tracing_thresh;
-
- if (context->tracing_thresh != OSNOISE_OPTION_INIT_VAL)
- return context->tracing_thresh;
-
- if (context->orig_tracing_thresh != OSNOISE_OPTION_INIT_VAL)
- return context->orig_tracing_thresh;
-
- tracing_thresh = osnoise_read_ll_config("tracing_thresh");
- if (tracing_thresh < 0)
- goto out_err;
-
- context->orig_tracing_thresh = tracing_thresh;
- return tracing_thresh;
-
-out_err:
- return OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_set_tracing_thresh - set "tracing_thresh"
- */
-int osnoise_set_tracing_thresh(struct osnoise_context *context, long long tracing_thresh)
-{
- long long curr_tracing_thresh = osnoise_get_tracing_thresh(context);
- int retval;
-
- if (curr_tracing_thresh == OSNOISE_OPTION_INIT_VAL)
- return -1;
-
- retval = osnoise_write_ll_config("tracing_thresh", tracing_thresh);
- if (retval < 0)
- return -1;
-
- context->tracing_thresh = tracing_thresh;
-
- return 0;
-}
-
-/*
- * osnoise_restore_tracing_thresh - restore the original "tracing_thresh"
- */
-void osnoise_restore_tracing_thresh(struct osnoise_context *context)
-{
- int retval;
-
- if (context->orig_tracing_thresh == OSNOISE_OPTION_INIT_VAL)
- return;
-
- if (context->orig_tracing_thresh == context->tracing_thresh)
- goto out_done;
-
- retval = osnoise_write_ll_config("tracing_thresh", context->orig_tracing_thresh);
- if (retval < 0)
- err_msg("Could not restore original tracing_thresh\n");
-
-out_done:
- context->tracing_thresh = OSNOISE_OPTION_INIT_VAL;
-}
-
-/*
- * osnoise_put_tracing_thresh - restore original values and cleanup data
- */
-void osnoise_put_tracing_thresh(struct osnoise_context *context)
-{
- osnoise_restore_tracing_thresh(context);
-
- if (context->orig_tracing_thresh == OSNOISE_OPTION_INIT_VAL)
- return;
-
- context->orig_tracing_thresh = OSNOISE_OPTION_INIT_VAL;
-}
+ * Long long option get/set/restore/put functions, generated from OSNOISE_LL_OPTIONS.
+ */
+#define OSNOISE_LL_OPTION(name, path, init_val) \
+static long long \
+osnoise_get_##name(struct osnoise_context *context) \
+{ \
+ long long name; \
+ \
+ if (context->name != (init_val)) \
+ return context->name; \
+ \
+ if (context->orig_##name != (init_val)) \
+ return context->orig_##name; \
+ \
+ name = osnoise_read_ll_config(path); \
+ if (name < 0) \
+ return (init_val); \
+ \
+ context->orig_##name = name; \
+ return name; \
+} \
+ \
+int osnoise_set_##name(struct osnoise_context *context, long long name) \
+{ \
+ long long curr = osnoise_get_##name(context); \
+ int retval; \
+ \
+ if (curr == (init_val)) \
+ return -1; \
+ \
+ retval = osnoise_write_ll_config(path, name); \
+ if (retval < 0) \
+ return -2; \
+ \
+ context->name = name; \
+ return 0; \
+} \
+ \
+void osnoise_restore_##name(struct osnoise_context *context) \
+{ \
+ int retval; \
+ \
+ if (context->orig_##name == (init_val)) \
+ return; \
+ \
+ if (context->orig_##name == context->name) \
+ goto out_done_##name; \
+ \
+ retval = osnoise_write_ll_config(path, context->orig_##name); \
+ if (retval < 0) \
+ err_msg("Could not restore original " #name "\n"); \
+ \
+out_done_##name: \
+ context->name = (init_val); \
+} \
+ \
+static void osnoise_put_##name(struct osnoise_context *context) \
+{ \
+ osnoise_restore_##name(context); \
+ \
+ if (context->orig_##name == (init_val)) \
+ return; \
+ \
+ context->orig_##name = (init_val); \
+}
+OSNOISE_LL_OPTIONS
+#undef OSNOISE_LL_OPTION
static int osnoise_options_get_option(char *option)
{
@@ -866,188 +459,70 @@ static int osnoise_options_set_option(char *option, bool onoff)
return tracefs_instance_file_write(NULL, "osnoise/options", no_option);
}
-static int osnoise_get_irq_disable(struct osnoise_context *context)
-{
- if (context->opt_irq_disable != OSNOISE_OPTION_INIT_VAL)
- return context->opt_irq_disable;
-
- if (context->orig_opt_irq_disable != OSNOISE_OPTION_INIT_VAL)
- return context->orig_opt_irq_disable;
-
- context->orig_opt_irq_disable = osnoise_options_get_option("OSNOISE_IRQ_DISABLE");
-
- return context->orig_opt_irq_disable;
-}
-
-int osnoise_set_irq_disable(struct osnoise_context *context, bool onoff)
-{
- int opt_irq_disable = osnoise_get_irq_disable(context);
- int retval;
-
- if (opt_irq_disable == OSNOISE_OPTION_INIT_VAL)
- return -1;
-
- if (opt_irq_disable == onoff)
- return 0;
-
- retval = osnoise_options_set_option("OSNOISE_IRQ_DISABLE", onoff);
- if (retval < 0)
- return -1;
-
- context->opt_irq_disable = onoff;
-
- return 0;
-}
-
-static void osnoise_restore_irq_disable(struct osnoise_context *context)
-{
- int retval;
-
- if (context->orig_opt_irq_disable == OSNOISE_OPTION_INIT_VAL)
- return;
-
- if (context->orig_opt_irq_disable == context->opt_irq_disable)
- goto out_done;
-
- retval = osnoise_options_set_option("OSNOISE_IRQ_DISABLE", context->orig_opt_irq_disable);
- if (retval < 0)
- err_msg("Could not restore original OSNOISE_IRQ_DISABLE option\n");
-
-out_done:
- context->orig_opt_irq_disable = OSNOISE_OPTION_INIT_VAL;
-}
-
-static void osnoise_put_irq_disable(struct osnoise_context *context)
-{
- osnoise_restore_irq_disable(context);
-
- if (context->orig_opt_irq_disable == OSNOISE_OPTION_INIT_VAL)
- return;
-
- context->orig_opt_irq_disable = OSNOISE_OPTION_INIT_VAL;
-}
-
-static int osnoise_get_workload(struct osnoise_context *context)
-{
- if (context->opt_workload != OSNOISE_OPTION_INIT_VAL)
- return context->opt_workload;
-
- if (context->orig_opt_workload != OSNOISE_OPTION_INIT_VAL)
- return context->orig_opt_workload;
-
- context->orig_opt_workload = osnoise_options_get_option("OSNOISE_WORKLOAD");
-
- return context->orig_opt_workload;
-}
-
-int osnoise_set_workload(struct osnoise_context *context, bool onoff)
-{
- int opt_workload = osnoise_get_workload(context);
- int retval;
-
- if (opt_workload == OSNOISE_OPTION_INIT_VAL)
- return -1;
-
- if (opt_workload == onoff)
- return 0;
-
- retval = osnoise_options_set_option("OSNOISE_WORKLOAD", onoff);
- if (retval < 0)
- return -2;
-
- context->opt_workload = onoff;
-
- return 0;
-}
-
-static void osnoise_restore_workload(struct osnoise_context *context)
-{
- int retval;
-
- if (context->orig_opt_workload == OSNOISE_OPTION_INIT_VAL)
- return;
-
- if (context->orig_opt_workload == context->opt_workload)
- goto out_done;
-
- retval = osnoise_options_set_option("OSNOISE_WORKLOAD", context->orig_opt_workload);
- if (retval < 0)
- err_msg("Could not restore original OSNOISE_WORKLOAD option\n");
-
-out_done:
- context->orig_opt_workload = OSNOISE_OPTION_INIT_VAL;
-}
-
-static void osnoise_put_workload(struct osnoise_context *context)
-{
- osnoise_restore_workload(context);
-
- if (context->orig_opt_workload == OSNOISE_OPTION_INIT_VAL)
- return;
-
- context->orig_opt_workload = OSNOISE_OPTION_INIT_VAL;
-}
-
-static int osnoise_get_timerlat_align(struct osnoise_context *context)
-{
- if (context->opt_timerlat_align != OSNOISE_OPTION_INIT_VAL)
- return context->opt_timerlat_align;
-
- if (context->orig_opt_timerlat_align != OSNOISE_OPTION_INIT_VAL)
- return context->orig_opt_timerlat_align;
-
- context->orig_opt_timerlat_align = osnoise_options_get_option("TIMERLAT_ALIGN");
-
- return context->orig_opt_timerlat_align;
-}
-
-int osnoise_set_timerlat_align(struct osnoise_context *context, bool onoff)
-{
- int opt_timerlat_align = osnoise_get_timerlat_align(context);
- int retval;
-
- if (opt_timerlat_align == OSNOISE_OPTION_INIT_VAL)
- return -1;
-
- if (opt_timerlat_align == onoff)
- return 0;
-
- retval = osnoise_options_set_option("TIMERLAT_ALIGN", onoff);
- if (retval < 0)
- return -2;
-
- context->opt_timerlat_align = onoff;
-
- return 0;
-}
-
-static void osnoise_restore_timerlat_align(struct osnoise_context *context)
-{
- int retval;
-
- if (context->orig_opt_timerlat_align == OSNOISE_OPTION_INIT_VAL)
- return;
-
- if (context->orig_opt_timerlat_align == context->opt_timerlat_align)
- goto out_done;
-
- retval = osnoise_options_set_option("TIMERLAT_ALIGN", context->orig_opt_timerlat_align);
- if (retval < 0)
- err_msg("Could not restore original TIMERLAT_ALIGN option\n");
-
-out_done:
- context->orig_opt_timerlat_align = OSNOISE_OPTION_INIT_VAL;
-}
-
-static void osnoise_put_timerlat_align(struct osnoise_context *context)
-{
- osnoise_restore_timerlat_align(context);
-
- if (context->orig_opt_timerlat_align == OSNOISE_OPTION_INIT_VAL)
- return;
-
- context->orig_opt_timerlat_align = OSNOISE_OPTION_INIT_VAL;
-}
+/*
+ * Flag option get/set/restore/put functions, generated from OSNOISE_FLAG_OPTIONS.
+ */
+#define OSNOISE_FLAG_OPTION(name, option_str) \
+static int osnoise_get_##name(struct osnoise_context *context) \
+{ \
+ if (context->opt_##name != OSNOISE_OPTION_INIT_VAL) \
+ return context->opt_##name; \
+ \
+ if (context->orig_opt_##name != OSNOISE_OPTION_INIT_VAL) \
+ return context->orig_opt_##name; \
+ \
+ context->orig_opt_##name = osnoise_options_get_option(option_str); \
+ return context->orig_opt_##name; \
+} \
+ \
+int osnoise_set_##name(struct osnoise_context *context, bool onoff) \
+{ \
+ int val = osnoise_get_##name(context); \
+ int retval; \
+ \
+ if (val == OSNOISE_OPTION_INIT_VAL) \
+ return -1; \
+ \
+ if (val == onoff) \
+ return 0; \
+ \
+ retval = osnoise_options_set_option(option_str, onoff); \
+ if (retval < 0) \
+ return -2; \
+ \
+ context->opt_##name = onoff; \
+ return 0; \
+} \
+ \
+void osnoise_restore_##name(struct osnoise_context *context) \
+{ \
+ int retval; \
+ \
+ if (context->orig_opt_##name == OSNOISE_OPTION_INIT_VAL) \
+ return; \
+ \
+ if (context->orig_opt_##name == context->opt_##name) \
+ goto out_done_##name; \
+ \
+ retval = osnoise_options_set_option(option_str, context->orig_opt_##name); \
+ if (retval < 0) \
+ err_msg("Could not restore original " option_str " option\n"); \
+ \
+out_done_##name: \
+ context->opt_##name = OSNOISE_OPTION_INIT_VAL; \
+} \
+ \
+static void osnoise_put_##name(struct osnoise_context *context) \
+{ \
+ osnoise_restore_##name(context); \
+ \
+ if (context->orig_opt_##name == OSNOISE_OPTION_INIT_VAL) \
+ return; \
+ \
+ context->orig_opt_##name = OSNOISE_OPTION_INIT_VAL; \
+}
+OSNOISE_FLAG_OPTIONS
+#undef OSNOISE_FLAG_OPTION
enum {
FLAG_CONTEXT_NEWLY_CREATED = (1 << 0),
@@ -1083,29 +558,16 @@ struct osnoise_context *osnoise_context_alloc(void)
context = calloc_fatal(1, sizeof(*context));
- context->orig_stop_us = OSNOISE_OPTION_INIT_VAL;
- context->stop_us = OSNOISE_OPTION_INIT_VAL;
-
- context->orig_stop_total_us = OSNOISE_OPTION_INIT_VAL;
- context->stop_total_us = OSNOISE_OPTION_INIT_VAL;
-
- context->orig_print_stack = OSNOISE_OPTION_INIT_VAL;
- context->print_stack = OSNOISE_OPTION_INIT_VAL;
-
- context->orig_tracing_thresh = OSNOISE_OPTION_INIT_VAL;
- context->tracing_thresh = OSNOISE_OPTION_INIT_VAL;
-
- context->orig_opt_irq_disable = OSNOISE_OPTION_INIT_VAL;
- context->opt_irq_disable = OSNOISE_OPTION_INIT_VAL;
-
- context->orig_opt_workload = OSNOISE_OPTION_INIT_VAL;
- context->opt_workload = OSNOISE_OPTION_INIT_VAL;
-
- context->orig_opt_timerlat_align = OSNOISE_OPTION_INIT_VAL;
- context->opt_timerlat_align = OSNOISE_OPTION_INIT_VAL;
-
- context->orig_timerlat_align_us = OSNOISE_OPTION_INIT_VAL;
- context->timerlat_align_us = OSNOISE_OPTION_INIT_VAL;
+#define OSNOISE_LL_OPTION(name, path, init_val) \
+ context->orig_##name = (init_val); \
+ context->name = (init_val);
+#define OSNOISE_FLAG_OPTION(name, option_str) \
+ context->orig_opt_##name = OSNOISE_OPTION_INIT_VAL; \
+ context->opt_##name = OSNOISE_OPTION_INIT_VAL;
+ OSNOISE_LL_OPTIONS
+ OSNOISE_FLAG_OPTIONS
+#undef OSNOISE_LL_OPTION
+#undef OSNOISE_FLAG_OPTION
osnoise_get_context(context);
@@ -1128,15 +590,13 @@ void osnoise_put_context(struct osnoise_context *context)
osnoise_put_cpus(context);
osnoise_put_runtime_period(context);
- osnoise_put_stop_us(context);
- osnoise_put_stop_total_us(context);
- osnoise_put_timerlat_period_us(context);
- osnoise_put_print_stack(context);
- osnoise_put_tracing_thresh(context);
- osnoise_put_irq_disable(context);
- osnoise_put_workload(context);
- osnoise_put_timerlat_align(context);
- osnoise_put_timerlat_align_us(context);
+
+#define OSNOISE_LL_OPTION(name, path, init_val) osnoise_put_##name(context);
+#define OSNOISE_FLAG_OPTION(name, option_str) osnoise_put_##name(context);
+ OSNOISE_LL_OPTIONS
+ OSNOISE_FLAG_OPTIONS
+#undef OSNOISE_LL_OPTION
+#undef OSNOISE_FLAG_OPTION
free(context);
}
diff --git a/tools/tracing/rtla/src/osnoise.h b/tools/tracing/rtla/src/osnoise.h
index 340ff5a64e6e..3d1852bffed8 100644
--- a/tools/tracing/rtla/src/osnoise.h
+++ b/tools/tracing/rtla/src/osnoise.h
@@ -34,28 +34,6 @@ int osnoise_set_runtime_period(struct osnoise_context *context,
unsigned long long period);
void osnoise_restore_runtime_period(struct osnoise_context *context);
-void osnoise_restore_stop_us(struct osnoise_context *context);
-void osnoise_restore_stop_total_us(struct osnoise_context *context);
-
-int osnoise_set_timerlat_period_us(struct osnoise_context *context,
- long long timerlat_period_us);
-void osnoise_restore_timerlat_period_us(struct osnoise_context *context);
-
-int osnoise_set_tracing_thresh(struct osnoise_context *context,
- long long tracing_thresh);
-void osnoise_restore_tracing_thresh(struct osnoise_context *context);
-
-void osnoise_restore_print_stack(struct osnoise_context *context);
-int osnoise_set_print_stack(struct osnoise_context *context,
- long long print_stack);
-
-int osnoise_set_timerlat_align_us(struct osnoise_context *context,
- long long timerlat_align_us);
-void osnoise_restore_timerlat_align_us(struct osnoise_context *context);
-
-int osnoise_set_timerlat_align(struct osnoise_context *context, bool onoff);
-
-int osnoise_set_irq_disable(struct osnoise_context *context, bool onoff);
void osnoise_report_missed_events(struct osnoise_tool *tool);
int osnoise_apply_config(struct osnoise_tool *tool, struct osnoise_params *params);
--
2.54.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] rtla: Simplify osnoise tracer option setting code
2026-06-12 11:51 [PATCH] rtla: Simplify osnoise tracer option setting code Tomas Glozar
@ 2026-06-12 17:54 ` Crystal Wood
2026-07-03 12:11 ` Tomas Glozar
0 siblings, 1 reply; 5+ messages in thread
From: Crystal Wood @ 2026-06-12 17:54 UTC (permalink / raw)
To: Tomas Glozar, Steven Rostedt
Cc: John Kacur, Luis Goncalves, Costa Shulyupin, Wander Lairson Costa,
LKML, linux-trace-kernel
On Fri, 2026-06-12 at 13:51 +0200, Tomas Glozar wrote:
> Each osnoise tracer option (in /sys/kernel/tracing/osnoise) used by RTLA
> requires four functions to be defined:
>
> - static osnoise_get_<opt>() - to get the current value of the option
> and save it into struct osnoise_context's orig_<opt> field,
> - osnoise_set_<opt>() - to set the value of the option requested by the
> user after reading and saving the original with osnoise_get_<opt>(),
> and save it into <opt> field of struct osnoise_context,
> - osnoise_restore_<opt>() - restore the value recorded in orig_<opt>,
> - static osnoise_put_<opt>() - restore the value recorded in orig_<opt>
> and update <opt> to reflect that.
>
> The logic is duplicated for all the options, except for cpus (which is
> the only string option) and period/runtime (which are handled together
> and feature extra checks).
Thanks for taking this on... this is one of the things that was bugging me
during consolidation work that I didn't get to.
> Deduplicate the logic using a set of macros featuring the X macro
> pattern, defined in src/common.h:
>
> - OSNOISE_LL_OPTIONS, which invokes OSNOISE_LL_OPTION macro for all
> "long long" options,
> - OSNOISE_FLAG_OPTIONS, which invokes OSNOISE_FLAG_OPTION macro for all
> flag (boolean values in osnoise/options file) options.
>
> The list macros are then invoked in four places:
>
> - for struct osnoise_context fields in src/common.h,
> - for function declarations, moved into src/common.h from
> src/osnoise.h,
> - for function definitions in src/osnoise.c,
> - for context initialization and restoration, in osnoise_context_alloc()
> and osnoise_put_context(), both in src/osnoise.c.
>
> OSNOISE_LL_OPTIONS takes three options: name - struct osnoise_context
> field name (written "<opt>" above), path - filename inside
> /sys/kernel/tracing/osnoise passed to libtracefs, and init_val - initial
> value of struct fields, corresponding to an otherwise invalid option
> (some options use OSNOISE_OPTION_INIT_VAL = -1, some use
> OSNOISE_TIME_INIT_VAL = 0).
Can we simplify by always using -1? Especially since that's already
treated as the universal "invalid" by osnoise_read_ll_config().
FWIW using "init val" to mean "invalid" rather than "default" is a bit
unintuitive.
> OSNOISE_FLAG_OPTION is similar, but instead of path, it takes the option
> string inside /sys/kernel/tracing/osnoise/options (opt_string), and no
> init_val, as it is purely boolean (0 or 1).
>
> Previously, for options timerlat_align and osnoise_workload, the return
> value of osnoise_set_<opt>() distinguished between -2 (option cannot be
> set) and -1 (option not present). This distinction is expanded for all
> options for consistency; for most options, it is currently not used,
> only osnoise_workload is implemented to avoid error on -1 on older RTLA
> versions.
"on -1 on"?
> The change overall has two main benefits: it makes it much simpler to
> add a new option, as well as to change existing logic consistently for
> all of them. It also makes the code shorter by a bit over 500 lines.
>
> There is no intentional user-visible change coming from the refactoring.
> osnoise_restore_<opt>() for flag options now sets <opt> instead of
> orig_<opt>. As the latter is also set by osnoise_put_<opt>(), plus long
> long options set <opt> in both the old and new implementation, the old
> behavior was likely a mistake, and should not matter for now, as the
> options are only restored once at the end of tracing and neither <opt>
> nor orig_<opt> field is read again.
>
> Assisted-by: Claude:claude-opus-4-6
> Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> ---
> tools/tracing/rtla/src/common.h | 79 +--
> tools/tracing/rtla/src/osnoise.c | 836 ++++++-------------------------
> tools/tracing/rtla/src/osnoise.h | 22 -
> 3 files changed, 188 insertions(+), 749 deletions(-)
While we're at it, can we move this code to common.c, and drop
"osnoise" from the names, to move closer to using that only for the
actual osnoise mode?
Or if we really want to namespace things that are specific to the
osnoise subsystem (i.e. everything implemented in trace_osnoise.c) but
not specific with respect to the osnoise/timerlat split, I'd suggest
something different like "osn_".
> + * Long long option get/set/restore/put functions, generated from OSNOISE_LL_OPTIONS.
> + */
> +#define OSNOISE_LL_OPTION(name, path, init_val) \
> +static long long \
> +osnoise_get_##name(struct osnoise_context *context) \
> +{ \
> + long long name; \
> + \
> + if (context->name != (init_val)) \
> + return context->name; \
> + \
> + if (context->orig_##name != (init_val)) \
> + return context->orig_##name; \
> + \
> + name = osnoise_read_ll_config(path); \
> + if (name < 0) \
> + return (init_val); \
> + \
> + context->orig_##name = name; \
> + return name; \
> +} \
> + \
> +int osnoise_set_##name(struct osnoise_context *context, long long name) \
> +{ \
> + long long curr = osnoise_get_##name(context); \
> + int retval; \
> + \
> + if (curr == (init_val)) \
> + return -1; \
> + \
> + retval = osnoise_write_ll_config(path, name); \
> + if (retval < 0) \
> + return -2; \
> + \
> + context->name = name; \
> + return 0; \
> +} \
Using "name" for the value is confusing... "val" would be better.
> + \
> +void osnoise_restore_##name(struct osnoise_context *context) \
> +{ \
> + int retval; \
> + \
> + if (context->orig_##name == (init_val)) \
> + return; \
> + \
> + if (context->orig_##name == context->name) \
> + goto out_done_##name; \
> + \
> + retval = osnoise_write_ll_config(path, context->orig_##name); \
> + if (retval < 0) \
> + err_msg("Could not restore original " #name "\n"); \
> + \
> +out_done_##name: \
> + context->name = (init_val); \
> +} \
Why does the label need to have ##name in it?
> + \
> +static void osnoise_put_##name(struct osnoise_context *context) \
> +{ \
> + osnoise_restore_##name(context); \
> + \
> + if (context->orig_##name == (init_val)) \
> + return; \
> + \
> + context->orig_##name = (init_val); \
> +}
[snip]
> +/*
> + * Flag option get/set/restore/put functions, generated from OSNOISE_FLAG_OPTIONS.
> + */
> +#define OSNOISE_FLAG_OPTION(name, option_str) \
> +static int osnoise_get_##name(struct osnoise_context *context) \
> +{ \
> + if (context->opt_##name != OSNOISE_OPTION_INIT_VAL) \
> + return context->opt_##name; \
> + \
> + if (context->orig_opt_##name != OSNOISE_OPTION_INIT_VAL) \
> + return context->orig_opt_##name; \
> + \
> + context->orig_opt_##name = osnoise_options_get_option(option_str); \
> + return context->orig_opt_##name; \
> +} \
> + \
> +int osnoise_set_##name(struct osnoise_context *context, bool onoff) \
> +{ \
> + int val = osnoise_get_##name(context); \
> + int retval; \
> + \
> + if (val == OSNOISE_OPTION_INIT_VAL) \
> + return -1; \
> + \
> + if (val == onoff) \
> + return 0; \
> + \
> + retval = osnoise_options_set_option(option_str, onoff); \
> + if (retval < 0) \
> + return -2; \
> + \
> + context->opt_##name = onoff; \
> + return 0; \
> +} \
> + \
> +void osnoise_restore_##name(struct osnoise_context *context) \
> +{ \
> + int retval; \
> + \
> + if (context->orig_opt_##name == OSNOISE_OPTION_INIT_VAL) \
> + return; \
> + \
> + if (context->orig_opt_##name == context->opt_##name) \
> + goto out_done_##name; \
> + \
> + retval = osnoise_options_set_option(option_str, context->orig_opt_##name); \
> + if (retval < 0) \
> + err_msg("Could not restore original " option_str " option\n"); \
> + \
> +out_done_##name: \
> + context->opt_##name = OSNOISE_OPTION_INIT_VAL; \
> +} \
> + \
> +static void osnoise_put_##name(struct osnoise_context *context) \
> +{ \
> + osnoise_restore_##name(context); \
> + \
> + if (context->orig_opt_##name == OSNOISE_OPTION_INIT_VAL) \
> + return; \
> + \
> + context->orig_opt_##name = OSNOISE_OPTION_INIT_VAL; \
> +}
Can we reduce the amount of code we put in macros by moving some of the
logic to osnoise_read/write_ll_config() and osnoise_get/set_optino()?
Or a non-macro wrapper around them if there are other callers that need
the current behavior.
Something like (assuming universal -1 invalid):
static int osn_read_ll_config(const char *rel_path, long long *val, long long *orig)
static int osn_write_ll_config(const char *rel_path, long long *val, long long *orig)
static int osn_get_option(const char *name, int *val, int *orig)
static int osn_set_option(const char *name, int *val, int *orig)
-Crystal (who wishes we were using a modern language that didn't require all
this macro stuff)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rtla: Simplify osnoise tracer option setting code
2026-06-12 17:54 ` Crystal Wood
@ 2026-07-03 12:11 ` Tomas Glozar
2026-07-03 14:09 ` Tomas Glozar
2026-08-18 22:31 ` Crystal Wood
0 siblings, 2 replies; 5+ messages in thread
From: Tomas Glozar @ 2026-07-03 12:11 UTC (permalink / raw)
To: Crystal Wood
Cc: Steven Rostedt, John Kacur, Luis Goncalves, Costa Shulyupin,
Wander Lairson Costa, LKML, linux-trace-kernel
pá 12. 6. 2026 v 19:55 Crystal Wood <crwood@redhat.com> napsala:
> > OSNOISE_LL_OPTIONS takes three options: name - struct osnoise_context
> > field name (written "<opt>" above), path - filename inside
> > /sys/kernel/tracing/osnoise passed to libtracefs, and init_val - initial
> > value of struct fields, corresponding to an otherwise invalid option
> > (some options use OSNOISE_OPTION_INIT_VAL = -1, some use
> > OSNOISE_TIME_INIT_VAL = 0).
>
> Can we simplify by always using -1? Especially since that's already
> treated as the universal "invalid" by osnoise_read_ll_config().
>
> FWIW using "init val" to mean "invalid" rather than "default" is a bit
> unintuitive.
>
The idea behind *_INIT_VAL is to re-use a value that is invalid on the
osnoise tracer side to mean "not (read from tracer and) set yet (on
the RTLA side)". OSNOISE_TIME_INIT_VAL is used for values where 0 is
invalid (e.g. period, runtime), OSNOISE_OPTION_INIT_VAL is used for
those where 0 is valid. I believe the distinction comes from the fact
that some of the osnoise options are unsigned on the kernel side, and
-1 (= 18446744073709551615) is actually a valid value there. E.g.:
[root@cs9 osnoise]# cat period_us
1000000
[root@cs9 osnoise]# echo 18446744073709551615 > period_us
[root@cs9 osnoise]# cat period_us
18446744073709551615
There are values where both 0 and -1 are valid (stop_tracing_us,
stop_tracing_total_us, timerlat_align_us), and they use
OSNOISE_OPTION_INIT_VAL. Those cannot use OSNOISE_TIME_INIT_VAL as 0
is a meaningful and common value for them (disabled for
stop_us/stop_total_us, zero alignment for timerlat_align_us). Those
will break if pre-set to 18446744073709551615.
Merging OSNOISE_OPTION_INIT_VAL and OSNOISE_TIME_INIT_VAL would
introduce this breakage to options that are using
OSNOISE_TIME_INIT_VAL now. IMHO the cleanest solution is dropping the
entire complex logic around checking if the value was read from the
kernel already, as there is no user: RTLA always saves the value once,
then restores it at the end.
> > OSNOISE_FLAG_OPTION is similar, but instead of path, it takes the option
> > string inside /sys/kernel/tracing/osnoise/options (opt_string), and no
> > init_val, as it is purely boolean (0 or 1).
> >
> > Previously, for options timerlat_align and osnoise_workload, the return
> > value of osnoise_set_<opt>() distinguished between -2 (option cannot be
> > set) and -1 (option not present). This distinction is expanded for all
> > options for consistency; for most options, it is currently not used,
> > only osnoise_workload is implemented to avoid error on -1 on older RTLA
> > versions.
>
> "on -1 on"?
On older RTLA versions, osnoise_workload distinguishes between -1 and
-2 to avoid error when the kernel doesn't support the feature.
>
> > The change overall has two main benefits: it makes it much simpler to
> > add a new option, as well as to change existing logic consistently for
> > all of them. It also makes the code shorter by a bit over 500 lines.
> >
> > There is no intentional user-visible change coming from the refactoring.
> > osnoise_restore_<opt>() for flag options now sets <opt> instead of
> > orig_<opt>. As the latter is also set by osnoise_put_<opt>(), plus long
> > long options set <opt> in both the old and new implementation, the old
> > behavior was likely a mistake, and should not matter for now, as the
> > options are only restored once at the end of tracing and neither <opt>
> > nor orig_<opt> field is read again.
> >
> > Assisted-by: Claude:claude-opus-4-6
> > Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> > ---
> > tools/tracing/rtla/src/common.h | 79 +--
> > tools/tracing/rtla/src/osnoise.c | 836 ++++++-------------------------
> > tools/tracing/rtla/src/osnoise.h | 22 -
> > 3 files changed, 188 insertions(+), 749 deletions(-)
>
> While we're at it, can we move this code to common.c, and drop
> "osnoise" from the names, to move closer to using that only for the
> actual osnoise mode?
>
> Or if we really want to namespace things that are specific to the
> osnoise subsystem (i.e. everything implemented in trace_osnoise.c) but
> not specific with respect to the osnoise/timerlat split, I'd suggest
> something different like "osn_".
>
They are called "osnoise options" in the interface (although they are
shared with the timerlat tracer), which cannot be changed. I don't
like using an esoteric prefix like "osn".
> > + * Long long option get/set/restore/put functions, generated from OSNOISE_LL_OPTIONS.
> > + */
> > +#define OSNOISE_LL_OPTION(name, path, init_val) \
> > +static long long \
> > +osnoise_get_##name(struct osnoise_context *context) \
> > +{ \
> > + long long name; \
> > + \
> > + if (context->name != (init_val)) \
> > + return context->name; \
> > + \
> > + if (context->orig_##name != (init_val)) \
> > + return context->orig_##name; \
> > + \
> > + name = osnoise_read_ll_config(path); \
> > + if (name < 0) \
> > + return (init_val); \
> > + \
> > + context->orig_##name = name; \
> > + return name; \
> > +} \
> > + \
> > +int osnoise_set_##name(struct osnoise_context *context, long long name) \
> > +{ \
> > + long long curr = osnoise_get_##name(context); \
> > + int retval; \
> > + \
> > + if (curr == (init_val)) \
> > + return -1; \
> > + \
> > + retval = osnoise_write_ll_config(path, name); \
> > + if (retval < 0) \
> > + return -2; \
> > + \
> > + context->name = name; \
> > + return 0; \
> > +} \
>
> Using "name" for the value is confusing... "val" would be better.
>
But it's the *name* of the option/field/argument here, not the value.
If you use "value" you'll get:
#define OSNOISE_LL_OPTION(value, path, init_val)
which is incorrect. Would making the macro options in capital letters
(i.e. NAME) make it more clear?
> > + \
> > +void osnoise_restore_##name(struct osnoise_context *context) \
> > +{ \
> > + int retval; \
> > + \
> > + if (context->orig_##name == (init_val)) \
> > + return; \
> > + \
> > + if (context->orig_##name == context->name) \
> > + goto out_done_##name; \
> > + \
> > + retval = osnoise_write_ll_config(path, context->orig_##name); \
> > + if (retval < 0) \
> > + err_msg("Could not restore original " #name "\n"); \
> > + \
> > +out_done_##name: \
> > + context->name = (init_val); \
> > +} \
>
> Why does the label need to have ##name in it?
>
It doesn't. Code assistants are just not smart enough to distinguish
labels from global symbols yet, it seems...
> > + \
> > +static void osnoise_put_##name(struct osnoise_context *context) \
> > +{ \
> > + osnoise_restore_##name(context); \
> > + \
> > + if (context->orig_##name == (init_val)) \
> > + return; \
> > + \
> > + context->orig_##name = (init_val); \
> > +}
> [snip]
> > +/*
> > + * Flag option get/set/restore/put functions, generated from OSNOISE_FLAG_OPTIONS.
> > + */
> > +#define OSNOISE_FLAG_OPTION(name, option_str) \
> > +static int osnoise_get_##name(struct osnoise_context *context) \
> > +{ \
> > + if (context->opt_##name != OSNOISE_OPTION_INIT_VAL) \
> > + return context->opt_##name; \
> > + \
> > + if (context->orig_opt_##name != OSNOISE_OPTION_INIT_VAL) \
> > + return context->orig_opt_##name; \
> > + \
> > + context->orig_opt_##name = osnoise_options_get_option(option_str); \
> > + return context->orig_opt_##name; \
> > +} \
> > + \
> > +int osnoise_set_##name(struct osnoise_context *context, bool onoff) \
> > +{ \
> > + int val = osnoise_get_##name(context); \
> > + int retval; \
> > + \
> > + if (val == OSNOISE_OPTION_INIT_VAL) \
> > + return -1; \
> > + \
> > + if (val == onoff) \
> > + return 0; \
> > + \
> > + retval = osnoise_options_set_option(option_str, onoff); \
> > + if (retval < 0) \
> > + return -2; \
> > + \
> > + context->opt_##name = onoff; \
> > + return 0; \
> > +} \
> > + \
> > +void osnoise_restore_##name(struct osnoise_context *context) \
> > +{ \
> > + int retval; \
> > + \
> > + if (context->orig_opt_##name == OSNOISE_OPTION_INIT_VAL) \
> > + return; \
> > + \
> > + if (context->orig_opt_##name == context->opt_##name) \
> > + goto out_done_##name; \
> > + \
> > + retval = osnoise_options_set_option(option_str, context->orig_opt_##name); \
> > + if (retval < 0) \
> > + err_msg("Could not restore original " option_str " option\n"); \
> > + \
> > +out_done_##name: \
> > + context->opt_##name = OSNOISE_OPTION_INIT_VAL; \
> > +} \
> > + \
> > +static void osnoise_put_##name(struct osnoise_context *context) \
> > +{ \
> > + osnoise_restore_##name(context); \
> > + \
> > + if (context->orig_opt_##name == OSNOISE_OPTION_INIT_VAL) \
> > + return; \
> > + \
> > + context->orig_opt_##name = OSNOISE_OPTION_INIT_VAL; \
> > +}
>
> Can we reduce the amount of code we put in macros by moving some of the
> logic to osnoise_read/write_ll_config() and osnoise_get/set_optino()?
> Or a non-macro wrapper around them if there are other callers that need
> the current behavior.
>
> Something like (assuming universal -1 invalid):
>
> static int osn_read_ll_config(const char *rel_path, long long *val, long long *orig)
> static int osn_write_ll_config(const char *rel_path, long long *val, long long *orig)
> static int osn_get_option(const char *name, int *val, int *orig)
> static int osn_set_option(const char *name, int *val, int *orig)
>
Yeah I think that could work. The get/set functions call each other,
but they can just pass the pointers instead.
> -Crystal (who wishes we were using a modern language that didn't require all
> this macro stuff)
>
Tomas
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rtla: Simplify osnoise tracer option setting code
2026-07-03 12:11 ` Tomas Glozar
@ 2026-07-03 14:09 ` Tomas Glozar
2026-08-18 22:31 ` Crystal Wood
1 sibling, 0 replies; 5+ messages in thread
From: Tomas Glozar @ 2026-07-03 14:09 UTC (permalink / raw)
To: Crystal Wood
Cc: Steven Rostedt, John Kacur, Luis Goncalves, Costa Shulyupin,
Wander Lairson Costa, LKML, linux-trace-kernel
pá 3. 7. 2026 v 14:11 odesílatel Tomas Glozar <tglozar@redhat.com> napsal:
>
> pá 12. 6. 2026 v 19:55 Crystal Wood <crwood@redhat.com> napsala:
>
> > > OSNOISE_LL_OPTIONS takes three options: name - struct osnoise_context
> > > field name (written "<opt>" above), path - filename inside
> > > /sys/kernel/tracing/osnoise passed to libtracefs, and init_val - initial
> > > value of struct fields, corresponding to an otherwise invalid option
> > > (some options use OSNOISE_OPTION_INIT_VAL = -1, some use
> > > OSNOISE_TIME_INIT_VAL = 0).
> >
> > Can we simplify by always using -1? Especially since that's already
> > treated as the universal "invalid" by osnoise_read_ll_config().
> >
> > FWIW using "init val" to mean "invalid" rather than "default" is a bit
> > unintuitive.
> >
>
> The idea behind *_INIT_VAL is to re-use a value that is invalid on the
> osnoise tracer side to mean "not (read from tracer and) set yet (on
> the RTLA side)". OSNOISE_TIME_INIT_VAL is used for values where 0 is
> invalid (e.g. period, runtime), OSNOISE_OPTION_INIT_VAL is used for
> those where 0 is valid. I believe the distinction comes from the fact
> that some of the osnoise options are unsigned on the kernel side, and
> -1 (= 18446744073709551615) is actually a valid value there. E.g.:
>
> [root@cs9 osnoise]# cat period_us
> 1000000
> [root@cs9 osnoise]# echo 18446744073709551615 > period_us
> [root@cs9 osnoise]# cat period_us
> 18446744073709551615
>
> There are values where both 0 and -1 are valid (stop_tracing_us,
> stop_tracing_total_us, timerlat_align_us), and they use
> OSNOISE_OPTION_INIT_VAL. Those cannot use OSNOISE_TIME_INIT_VAL as 0
> is a meaningful and common value for them (disabled for
> stop_us/stop_total_us, zero alignment for timerlat_align_us). Those
> will break if pre-set to 18446744073709551615.
>
> Merging OSNOISE_OPTION_INIT_VAL and OSNOISE_TIME_INIT_VAL would
> introduce this breakage to options that are using
> OSNOISE_TIME_INIT_VAL now. IMHO the cleanest solution is dropping the
> entire complex logic around checking if the value was read from the
> kernel already, as there is no user: RTLA always saves the value once,
> then restores it at the end.
>
Ah sorry, I missed that we use get_llong_from_str() for everything,
which already treats all values as (signed) long long, despite period
and runtime ("OSNOISE_OPTION_INIT_VAL" options) being declared
unsigned. We can just drop the INIT_VALs then and just use -1.
Tomas
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rtla: Simplify osnoise tracer option setting code
2026-07-03 12:11 ` Tomas Glozar
2026-07-03 14:09 ` Tomas Glozar
@ 2026-08-18 22:31 ` Crystal Wood
1 sibling, 0 replies; 5+ messages in thread
From: Crystal Wood @ 2026-08-18 22:31 UTC (permalink / raw)
To: Tomas Glozar
Cc: Steven Rostedt, John Kacur, Luis Goncalves, Costa Shulyupin,
Wander Lairson Costa, LKML, linux-trace-kernel
On Fri, 2026-07-03 at 14:11 +0200, Tomas Glozar wrote:
> pá 12. 6. 2026 v 19:55 Crystal Wood <crwood@redhat.com> napsala:
>
> > > OSNOISE_LL_OPTIONS takes three options: name - struct osnoise_context
> > > field name (written "<opt>" above), path - filename inside
> > > /sys/kernel/tracing/osnoise passed to libtracefs, and init_val - initial
> > > value of struct fields, corresponding to an otherwise invalid option
> > > (some options use OSNOISE_OPTION_INIT_VAL = -1, some use
> > > OSNOISE_TIME_INIT_VAL = 0).
> >
> > Can we simplify by always using -1? Especially since that's already
> > treated as the universal "invalid" by osnoise_read_ll_config().
> >
> > FWIW using "init val" to mean "invalid" rather than "default" is a bit
> > unintuitive.
> >
>
> The idea behind *_INIT_VAL is to re-use a value that is invalid on the
> osnoise tracer side to mean "not (read from tracer and) set yet (on
> the RTLA side)". OSNOISE_TIME_INIT_VAL is used for values where 0 is
> invalid (e.g. period, runtime), OSNOISE_OPTION_INIT_VAL is used for
> those where 0 is valid.
>
Right, it just hurts seeing something called "INIT_VAL" that isn't the
initial value. :-/
> > While we're at it, can we move this code to common.c, and drop
> > "osnoise" from the names, to move closer to using that only for the
> > actual osnoise mode?
> >
> > Or if we really want to namespace things that are specific to the
> > osnoise subsystem (i.e. everything implemented in trace_osnoise.c) but
> > not specific with respect to the osnoise/timerlat split, I'd suggest
> > something different like "osn_".
> >
>
> They are called "osnoise options" in the interface (although they are
> shared with the timerlat tracer), which cannot be changed. I don't
> like using an esoteric prefix like "osn".
I'm not suggesting changing the kernel interface, just having some
convention within rtla (and within trace_osnoise.c) to make it clearer
which things apply to osnoise-the-subsystem versus osnoise-the-specific-
tracer. It might be "esoteric" which one gets the abbreviation, but as
long as it's consistent I think it'd be an improvement over the current
jumble.
>
> > > + * Long long option get/set/restore/put functions, generated from OSNOISE_LL_OPTIONS.
> > > + */
> > > +#define OSNOISE_LL_OPTION(name, path, init_val) \
> > > +static long long \
> > > +osnoise_get_##name(struct osnoise_context *context) \
> > > +{ \
> > > + long long name; \
> > > + \
> > > + if (context->name != (init_val)) \
> > > + return context->name; \
> > > + \
> > > + if (context->orig_##name != (init_val)) \
> > > + return context->orig_##name; \
> > > + \
> > > + name = osnoise_read_ll_config(path); \
> > > + if (name < 0) \
> > > + return (init_val); \
> > > + \
> > > + context->orig_##name = name; \
> > > + return name; \
> > > +} \
> > > + \
> > > +int osnoise_set_##name(struct osnoise_context *context, long long name) \
> > > +{ \
> > > + long long curr = osnoise_get_##name(context); \
> > > + int retval; \
> > > + \
> > > + if (curr == (init_val)) \
> > > + return -1; \
> > > + \
> > > + retval = osnoise_write_ll_config(path, name); \
> > > + if (retval < 0) \
> > > + return -2; \
> > > + \
> > > + context->name = name; \
> > > + return 0; \
> > > +} \
> >
> > Using "name" for the value is confusing... "val" would be better.
> >
>
> But it's the *name* of the option/field/argument here, not the value.
> If you use "value" you'll get:
>
> #define OSNOISE_LL_OPTION(value, path, init_val)
>
> which is incorrect. Would making the macro options in capital letters
> (i.e. NAME) make it more clear?
That's not what I meant. I was suggesting this:
int osnoise_set_##name(struct osnoise_context *context, long long val)
-Crystal
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-08-18 22:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-12 11:51 [PATCH] rtla: Simplify osnoise tracer option setting code Tomas Glozar
2026-06-12 17:54 ` Crystal Wood
2026-07-03 12:11 ` Tomas Glozar
2026-07-03 14:09 ` Tomas Glozar
2026-08-18 22:31 ` Crystal Wood
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox