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
