Re: [PATCH] rtla: Simplify osnoise tracer option setting code

[Crystal Wood <crwood@redhat.com>]

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)