pá 12. 6. 2026 v 19:55 Crystal Wood <[email protected]> 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 <[email protected]> > > --- > > 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
