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

On Fri, 2026-07-03 at 14:11 +0200, Tomas Glozar wrote:
> 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.
> 

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